Skip to content

Conversation

@jglick
Copy link
Member

@jglick jglick commented Feb 21, 2025

Output from jenkinsci/workflow-support-plugin#305 indicate that a lot of stack frames are Closure.call. In some cases this is actually helping to structure code. In others, it is not. Two half-cheese, half-sausage pizzas are just one cheese and one sausage pizza.

CloudBees-internal issue

@jglick jglick added the bug label Feb 21, 2025
@jglick jglick requested a review from Vlatombe February 21, 2025 23:03
script.withEnv(inputEnv) {
body.call()
}
def submitted = script.input(message: input.message, id: input.id, ok: input.ok, submitter: input.submitter,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore WS)

import org.jenkinsci.plugins.workflow.cps.CpsScript;


@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in a few other plugins:

  • kubernetes
  • docker
  • docker-workflow
  • amazon-ecs

@jglick jglick requested a review from a team February 25, 2025 00:58
@jglick jglick marked this pull request as ready for review February 25, 2025 00:58
@jglick
Copy link
Member Author

jglick commented Feb 25, 2025

I presume a timeout in BasicModelDefTest.stages300 is a flake, retrying…

(Linux only; already skipped on Windows for being too slow.)

@jglick jglick enabled auto-merge February 25, 2025 15:00
…lly, but 192s in CI even when passing, close to the limit
@Issue("JENKINS-47363")
// Give this a longer timeout
@Test(timeout=5 * 60 * 1000)
@Test(timeout=10 * 60 * 1000)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now it is passing in 98s. 🤷

@jglick jglick merged commit 4ab13b8 into jenkinsci:master Feb 25, 2025
17 checks passed
@jglick jglick deleted the closuritis branch February 25, 2025 16:21
@felipecrs
Copy link

This is listed as a breaking change in the release notes, but the details are shady.

Can you please clarify whether it's safe to upgrade this plugin for users using docker-workflow or not? And what's the game plan otherwise.

@jglick
Copy link
Member Author

jglick commented Feb 25, 2025

@felipecrs if all goes well, jenkinsci/docker-workflow-plugin#337 should release shortly. Sorry for any disruption.

@jglick
Copy link
Member Author

jglick commented Feb 25, 2025

@felipecrs
Copy link

Got it, that's alright, no need to apologize.

@felipecrs
Copy link

Just as a heads up and in case some other user searches for it, this upgrade caused ongoing builds to fail abruptly with:

Caused: java.io.InvalidClassException: org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl.LabelScript; Class does not extend stream superclass

But new builds seems to be running fine.

@jglick
Copy link
Member Author

jglick commented Feb 25, 2025

Good call; the plugin includes no tests of serial form (program.dat) that I could see. (Other Pipeline plugins do test that scenario, for Scripted syntax.) I will think about whether this could be made compatible (for anyone who has not yet upgraded).

Technically, the difficulty here is WithScriptDescribable does not offer a clear way to select an alternate script to use for a new API interface; and DeclarativeAgentScript could not use the usual idiom used in Jenkins plugins of Util.isOverridden to delegate to a new method variant because it is written in Java yet the Closure is CPS-transformed and so it would not work to either .call() it or in the reverse direction to create a Closure wrapping the new method in Java code. The same difficulty forced jenkinsci/docker-workflow-plugin#337. And more generally, it is hard to preserve serial form in this plugin after changing implementations in src/main/resources/**/*.groovy since a build serialized against an old plugin version will be deserialized against a new plugin version; whereas a Groovy library (or loaded script, etc.) will be snapshotted in build.xml so that the program is loadable even if the dependencies have since changed.

@felipecrs
Copy link

@jglick thank you very much for the detailed response. I must admit I understand little of what you say, but I got the gist: it would be very hard to keep this kind of backwards compatibility, or to avoid this disruption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants