Merge pull request #762 from jglick/closuritis

jglick · web-flow · commit 4ab13b8cd590 · 2025-02-25T11:16:21.000-05:00
Reduce depth of `Closure`s being `call`ed
diff --git a/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy b/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy
diff --git a/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/AnyScript.groovy b/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/AnyScript.groovy
@@ -26,21 +26,19 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
 import org.jenkinsci.plugins.pipeline.modeldefinition.agent.CheckoutScript
-import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript
+import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript2
 import org.jenkinsci.plugins.workflow.cps.CpsScript
 
-class AnyScript extends DeclarativeAgentScript<Any> {
+class AnyScript extends DeclarativeAgentScript2<Any> {
 
     AnyScript(CpsScript s, Any a) {
         super(s, a)
     }
 
     @Override
-    Closure run(Closure body) {
-        return {
-            script.node {
-                CheckoutScript.doCheckout(script, describable, null, body).call()
-            }
+    void run(Closure body) {
+        script.node {
+            CheckoutScript.doCheckout2(script, describable, null, body)
         }
     }
 }
diff --git a/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelScript.groovy b/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelScript.groovy
@@ -27,30 +27,27 @@ package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
 
 import org.jenkinsci.plugins.pipeline.modeldefinition.agent.CheckoutScript
-import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript
+import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript2
 import org.jenkinsci.plugins.workflow.cps.CpsScript
 
-class LabelScript extends DeclarativeAgentScript<Label> {
+class LabelScript extends DeclarativeAgentScript2<Label> {
 
     LabelScript(CpsScript s, Label a) {
         super(s, a)
     }
 
     @Override
-    Closure run(Closure body) {
-        Closure run = {
-            script.node(describable?.label) {
-                CheckoutScript.doCheckout(script, describable, describable.customWorkspace, body).call()
-            }
-        }
+    void run(Closure body) {
         if (describable.retries > 1) {
-            return {
-                script.retry(count: describable.retries, conditions: [script.agent(), script.nonresumable()]) {
-                    run.call()
+            script.retry(count: describable.retries, conditions: [script.agent(), script.nonresumable()]) {
+                script.node(describable?.label) {
+                    CheckoutScript.doCheckout2(script, describable, describable.customWorkspace, body)
                 }
             }
         } else {
-            run
+            script.node(describable?.label) {
+                CheckoutScript.doCheckout2(script, describable, describable.customWorkspace, body)
+            }
         }
     }
 }
diff --git a/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/NoneScript.groovy b/pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/NoneScript.groovy
@@ -25,17 +25,17 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
-import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript
+import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript2
 import org.jenkinsci.plugins.workflow.cps.CpsScript
 
-class NoneScript extends DeclarativeAgentScript<None> {
+class NoneScript extends DeclarativeAgentScript2<None> {
 
     NoneScript(CpsScript s, None a) {
         super(s, a)
     }
 
     @Override
-    Closure run(Closure body) {
-        return body
+    void run(Closure body) {
+        body.call()
     }
 }
diff --git a/pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java b/pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java
@@ -78,7 +78,7 @@ public static void setUpAgent() throws Exception {
 
     @Issue("JENKINS-47363")
     // Give this a longer timeout
-    @Test(timeout=5 * 60 * 1000)
+    @Test(timeout=10 * 60 * 1000)
     public void stages300() throws Exception {
         assumeFalse("can exceed even 5m timeout", Functions.isWindows());
         RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION = true;
diff --git a/pipeline-model-definition/src/test/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelAndOtherFieldAgentScript.groovy b/pipeline-model-definition/src/test/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelAndOtherFieldAgentScript.groovy
@@ -25,25 +25,23 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl
 
-import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript
+import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript2
 import org.jenkinsci.plugins.workflow.cps.CpsScript
 
-class LabelAndOtherFieldAgentScript extends DeclarativeAgentScript<LabelAndOtherFieldAgent> {
+class LabelAndOtherFieldAgentScript extends DeclarativeAgentScript2<LabelAndOtherFieldAgent> {
 
     LabelAndOtherFieldAgentScript(CpsScript s, LabelAndOtherFieldAgent a) {
         super(s, a)
     }
 
     @Override
-    Closure run(Closure body) {
+    void run(Closure body) {
         script.echo "Running in labelAndOtherField with otherField = ${describable.getOtherField()}"
         script.echo "And nested: ${describable.getNested()}"
         Label l = (Label) Label.DescriptorImpl.instanceForName("label", [label: describable.label])
         l.inStage = describable.inStage
         l.doCheckout = describable.doCheckout
         LabelScript labelScript = (LabelScript) l.getScript(script)
-        return labelScript.run {
-            body.call()
-        }
+        labelScript.run(body)
     }
 }
diff --git a/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgent.java b/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgent.java
@@ -28,6 +28,7 @@
 import java.util.Map;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.Extension;
+import java.util.logging.Logger;
 import org.jenkinsci.plugins.pipeline.modeldefinition.options.DeclarativeOption;
 import org.jenkinsci.plugins.pipeline.modeldefinition.withscript.WithScriptDescribable;
 import org.jenkinsci.plugins.pipeline.modeldefinition.withscript.WithScriptScript;
@@ -41,6 +42,9 @@
  * @author Andrew Bayer
  */
 public abstract class DeclarativeAgent<A extends DeclarativeAgent<A>> extends WithScriptDescribable<A> implements ExtensionPoint {
+
+    private static final Logger LOGGER = Logger.getLogger(DeclarativeAgent.class.getName());
+
     protected boolean inStage;
     protected boolean doCheckout;
     protected String subdirectory;
@@ -53,7 +57,11 @@ public WithScriptScript getScript(CpsScript cpsScript) throws Exception {
 
         cpsScript.getClass().getClassLoader().loadClass("org.jenkinsci.plugins.pipeline.modeldefinition.agent.CheckoutScript");
 
-        return super.getScript(cpsScript);
+        var script = super.getScript(cpsScript);
+        if (script instanceof DeclarativeAgentScript) {
+            LOGGER.warning(() -> script.getClass().getName() + " should implement " + DeclarativeAgentScript2.class.getName());
+        }
+        return script;
     }
 
     public void setInStage(boolean inStage) {
diff --git a/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgentScript.java b/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgentScript.java
@@ -27,7 +27,7 @@
 import org.jenkinsci.plugins.pipeline.modeldefinition.withscript.WithScriptScript;
 import org.jenkinsci.plugins.workflow.cps.CpsScript;
 
-
+@Deprecated
 public abstract class DeclarativeAgentScript<A extends DeclarativeAgent<A>> extends WithScriptScript<A> {
 
     public DeclarativeAgentScript(CpsScript s, A a) {
diff --git a/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgentScript2.java b/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgentScript2.java
@@ -0,0 +1,38 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2016, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package org.jenkinsci.plugins.pipeline.modeldefinition.agent;
+
+import groovy.lang.Closure;
+import org.jenkinsci.plugins.pipeline.modeldefinition.withscript.WithScriptScript;
+import org.jenkinsci.plugins.workflow.cps.CpsScript;
+
+public abstract class DeclarativeAgentScript2<A extends DeclarativeAgent<A>> extends WithScriptScript<A> {
+
+    public DeclarativeAgentScript2(CpsScript s, A a) {
+        super(s, a);
+    }
+
+    public abstract void run(Closure body);
+}
diff --git a/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/RetryableDeclarativeAgent.java b/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/RetryableDeclarativeAgent.java
@@ -28,21 +28,18 @@
 
 /**
  * A type of {@code agent} option that supports automatic retries.
- * Usage from your {@link DeclarativeAgentScript#run} would look something like:
+ * Usage from your {@link DeclarativeAgentScript2#run} would look something like:
  * <pre>{@code
- * Closure run = {
- *     script.node {
- *         CheckoutScript.doCheckout(script, describable, null, body).call()
- *     }
- * }
  * if (describable.retries > 1) {
- *     return {
- *         script.retry(count: describable.retries, conditions: [script.agent(), script.nonresumable()]) {
- *             run.call()
+ *     script.retry(count: describable.retries, conditions: [script.agent(), script.nonresumable()]) {
+ *         script.node {
+ *             CheckoutScript.doCheckout2(script, describable, null, body)
  *         }
  *     }
  * } else {
- *     run
+ *     script.node {
+ *         CheckoutScript.doCheckout2(script, describable, null, body)
+ *     }
  * }}</pre>
  */
 public abstract class RetryableDeclarativeAgent<A extends RetryableDeclarativeAgent<A>> extends DeclarativeAgent<A> {
diff --git a/pipeline-model-extensions/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy b/pipeline-model-extensions/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy
@@ -30,40 +30,41 @@ import org.jenkinsci.plugins.workflow.cps.CpsScript
 
 class CheckoutScript implements Serializable {
     
+    @Deprecated
     static Closure doCheckout(CpsScript script, DeclarativeAgent agent, String customWorkspace = null, Closure body) {
         return {
-            if (customWorkspace) {
-                script.ws(customWorkspace) {
-                    checkoutAndRun(script, agent, body).call()
-                }
-            } else {
-                checkoutAndRun(script, agent, body).call()
+            doCheckout2(script, agent, customWorkspace, body)
+        }
+    }
+
+    static void doCheckout2(CpsScript script, DeclarativeAgent agent, String customWorkspace = null, Closure body) {
+        if (customWorkspace) {
+            script.ws(customWorkspace) {
+                checkoutAndRun(script, agent, body)
             }
+        } else {
+            checkoutAndRun(script, agent, body)
         }
     }
-    
-    private static Closure checkoutAndRun(CpsScript script, DeclarativeAgent agent, Closure body) {
-        return {
-            def checkoutMap = [:]
 
-            if (agent.isDoCheckout() && agent.hasScmContext(script)) {
-                String subDir = agent.subdirectory
-                if (subDir != null && subDir != "") {
-                    script.dir(subDir) {
-                        checkoutMap.putAll(performCheckout(script, agent))
-                    }
-                } else {
+    private static void checkoutAndRun(CpsScript script, DeclarativeAgent agent, Closure body) {
+        def checkoutMap = [:]
+
+        if (agent.isDoCheckout() && agent.hasScmContext(script)) {
+            String subDir = agent.subdirectory
+            if (subDir != null && subDir != "") {
+                script.dir(subDir) {
                     checkoutMap.putAll(performCheckout(script, agent))
                 }
-            }
-            if (checkoutMap) {
-                script.withEnv(checkoutMap.collect { k, v -> "${k}=${v}" }) {
-                    body.call()
-                }
             } else {
-                body.call()
+                checkoutMap.putAll(performCheckout(script, agent))
             }
         }
+        if (checkoutMap) {
+            script.withEnv(checkoutMap.collect { k, v -> "${k}=${v}" }, body)
+        } else {
+            body.call()
+        }
     }
 
     private static Map performCheckout(CpsScript script, DeclarativeAgent agent) {

Original file line number	Diff line number	Diff line change
`@@ -26,21 +26,19 @@`
`26`	`26`	`package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl`
`27`	`27`
`28`	`28`	`import org.jenkinsci.plugins.pipeline.modeldefinition.agent.CheckoutScript`
`29`		`-import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript`
	`29`	`+import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript2`
`30`	`30`	`import org.jenkinsci.plugins.workflow.cps.CpsScript`
`31`	`31`
`32`		`-class AnyScript extends DeclarativeAgentScript<Any> {`
	`32`	`+class AnyScript extends DeclarativeAgentScript2<Any> {`
`33`	`33`
`34`	`34`	`AnyScript(CpsScript s, Any a) {`
`35`	`35`	`super(s, a)`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`@Override`
`39`		`- Closure run(Closure body) {`
`40`		`- return {`
`41`		`- script.node {`
`42`		`- CheckoutScript.doCheckout(script, describable, null, body).call()`
`43`		`- }`
	`39`	`+ void run(Closure body) {`
	`40`	`+ script.node {`
	`41`	`+ CheckoutScript.doCheckout2(script, describable, null, body)`
`44`	`42`	`}`
`45`	`43`	`}`
`46`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,17 +25,17 @@`
`25`	`25`
`26`	`26`	`package org.jenkinsci.plugins.pipeline.modeldefinition.agent.impl`
`27`	`27`
`28`		`-import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript`
	`28`	`+import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgentScript2`
`29`	`29`	`import org.jenkinsci.plugins.workflow.cps.CpsScript`
`30`	`30`
`31`		`-class NoneScript extends DeclarativeAgentScript<None> {`
	`31`	`+class NoneScript extends DeclarativeAgentScript2<None> {`
`32`	`32`
`33`	`33`	`NoneScript(CpsScript s, None a) {`
`34`	`34`	`super(s, a)`
`35`	`35`	`}`
`36`	`36`
`37`	`37`	`@Override`
`38`		`- Closure run(Closure body) {`
`39`		`- return body`
	`38`	`+ void run(Closure body) {`
	`39`	`+ body.call()`
`40`	`40`	`}`
`41`	`41`	`}`