Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-64846] Fix use of local declared variables inside matrix #415

Merged
merged 3 commits into from
Feb 12, 2021
Merged

[JENKINS-64846] Fix use of local declared variables inside matrix #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab6714b
[JENKINS-64846] Fix use of local declared variables inside matrix
bitwiseman Feb 11, 2021
3636ef4
Add non-splitting test to cover non-matrix scenario
bitwiseman Feb 11, 2021
e1ef424
Add profiles to run with and without script splitting
bitwiseman Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 34 additions & 34 deletions ...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/RuntimeASTTransformer.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ import static org.objectweb.asm.Opcodes.ACC_PUBLIC
class RuntimeASTTransformer {

/**
* Enables or disables the script splitting behavior in {@Wrapper} which
* mitigates "Method code too large" and "Class too large" errors.
* Enables or disables the script splitting behavior in {@Wrapper} which
* mitigates "Method code too large" and "Class too large" errors.
*/
@SuppressFBWarnings(value="MS_SHOULD_BE_FINAL", justification="For access from script console")
public static boolean SCRIPT_SPLITTING_TRANSFORMATION = SystemProperties.getBoolean(
Expand Down Expand Up @@ -1256,48 +1256,48 @@ class RuntimeASTTransformer {
@NonNull
private List<Statement> prepareClosureScopedHandles(@NonNull BlockStatement pipelineBlock) {
ArrayList<Statement> result = new ArrayList<Statement>()
if (SCRIPT_SPLITTING_TRANSFORMATION) {
ArrayList<DeclarationExpression> declarations = new ArrayList<DeclarationExpression>()
moduleNode.statementBlock.statements.each { item ->
if (item instanceof ExpressionStatement) {
ExpressionStatement es = (ExpressionStatement) item
if (es.expression instanceof DeclarationExpression) {
declarations.add((DeclarationExpression) es.expression)
}
ArrayList<DeclarationExpression> declarations = new ArrayList<DeclarationExpression>()
moduleNode.statementBlock.statements.each { item ->
if (item instanceof ExpressionStatement) {
ExpressionStatement es = (ExpressionStatement) item
if (es.expression instanceof DeclarationExpression) {
declarations.add((DeclarationExpression) es.expression)
}
}
}

if (!declarations.isEmpty()) {
if (SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES) {
result.addAll(pipelineElementHandles)
pipelineElementHandles.clear()
} else {
def declarationNames = []
declarations.each { item ->
def left = item.getLeftExpression()
if (left instanceof VariableExpression) {
declarationNames.add(((VariableExpression)left).getName())
} else if (left instanceof ArgumentListExpression) {
left.each { arg ->
if (arg instanceof VariableExpression) {
declarationNames.add(((VariableExpression) arg).getName())
} else {
declarationNames.add("Unrecognized expression: " + arg.toString())
}
// if we're not doing script splitting, keep the old behavior that preserves declared variable functionality in matrix
if (!declarations.isEmpty()) {
result.addAll(pipelineElementHandles)
pipelineElementHandles.clear()

if (SCRIPT_SPLITTING_TRANSFORMATION && !SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES) {
def declarationNames = []
declarations.each { item ->
def left = item.getLeftExpression()
if (left instanceof VariableExpression) {
declarationNames.add(((VariableExpression) left).getName())
} else if (left instanceof ArgumentListExpression) {
left.each { arg ->
if (arg instanceof VariableExpression) {
declarationNames.add(((VariableExpression) arg).getName())
} else {
declarationNames.add("Unrecognized expression: " + arg.toString())
}
} else {
declarationNames.add("Unrecognized declaration structure: " + left.toString())
}
} else {
declarationNames.add("Unrecognized declaration structure: " + left.toString())
}
throw new IllegalStateException("[JENKINS-34987] SCRIPT_SPLITTING_TRANSFORMATION is an experimental feature of Declarative Pipeline and is incompatible with local variable declarations inside a Jenkinsfile. " +
"As a temporary workaround, you can add the '@Field' annotation to these local variable declarations. " +
"However, use of Groovy variables in Declarative pipeline, with or without the '@Field' annotation, is not recommended or supported. " +
"To use less effective script splitting which allows local variable declarations without changing your pipeline code, set SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES=true . " +
"Local variable declarations found: " + declarationNames.sort().join(", ") + ". ")
}
throw new IllegalStateException("[JENKINS-34987] SCRIPT_SPLITTING_TRANSFORMATION is an experimental feature of Declarative Pipeline and is incompatible with local variable declarations inside a Jenkinsfile. " +
"As a temporary workaround, you can add the '@Field' annotation to these local variable declarations. " +
"However, use of Groovy variables in Declarative pipeline, with or without the '@Field' annotation, is not recommended or supported. " +
"To use less effective script splitting which allows local variable declarations without changing your pipeline code, set SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES=true . " +
"Local variable declarations found: " + declarationNames.sort().join(", ") + ". ")
}
}


// In a future version, it may be possible to detect closures that reference script-local variables
// and then only use closure scoped handles for those closures.
return result
Expand Down
5 changes: 0 additions & 5 deletions ...on/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,6 @@ private static String symbolFromDescriptor(Descriptor d) {
public void setUpFeatureFlags() {
defaultScriptSplitting = RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION;
defaultScriptSplittingAllowLocalVariables = RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES;

// For testing we want to default to exercising splitting
// and not allowing local variables
RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION = true;
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;
}

@After
Expand Down
20 changes: 20 additions & 0 deletions ...ition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import org.jenkinsci.plugins.workflow.pipelinegraphanalysis.GenericStatus;
import org.jenkinsci.plugins.workflow.pipelinegraphanalysis.StatusAndTiming;
import org.jenkinsci.plugins.workflow.steps.ErrorStep;
import org.junit.Assume;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
Expand All @@ -56,6 +57,7 @@
import java.util.Collection;
import java.util.List;

import static org.hamcrest.Matchers.is;
import static org.junit.Assert.*;

/**
Expand All @@ -76,6 +78,7 @@ public static void setUpAgent() throws Exception {
// Give this a longer timeout
@Test(timeout=5 * 60 * 1000)
public void stages300() throws Exception {
RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION = true;
expect("basic/stages300")
.logContains("letters1 = 'a', letters10 = 'a', letters100 = 'a'",
"letters1 = 'j', letters10 = 'j', letters100 = 'c'")
Expand All @@ -96,6 +99,20 @@ public void stages300NoSplit() throws Exception {
@Issue("JENKINS-37984")
@Test
public void stages100WithOutsideVarAndFunc() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;
expect("basic/stages100WithOutsideVarAndFunc")
.logContains("letters1 = 'a', letters10 = 'a', letters100 = 'a'",
"letters1 = 'j', letters10 = 'j', letters100 = 'a'",
"Hi there - This comes from a function")
.logNotContains("Method code too large!")
.go();
}

@Issue("JENKINS-37984")
@Test
public void stages100WithOutsideVarAndFuncNoSplitting() throws Exception {
RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION = false;
expect("basic/stages100WithOutsideVarAndFunc")
.logContains("letters1 = 'a', letters10 = 'a', letters100 = 'a'",
"letters1 = 'j', letters10 = 'j', letters100 = 'a'",
Expand All @@ -107,6 +124,9 @@ public void stages100WithOutsideVarAndFunc() throws Exception {
@Issue("JENKINS-37984")
@Test
public void stages100WithOutsideVarAndFuncNotAllowed() throws Exception {
// this test only works if script splitting is enabled
Assume.assumeThat(RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION, is(true));

RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = false;
expect(Result.FAILURE,"basic/stages100WithOutsideVarAndFunc")
.logContains("add the '@Field' annotation to these local variable declarations")
Expand Down
7 changes: 7 additions & 0 deletions ...inition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/EnvironmentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import hudson.model.Result;
import hudson.model.Slave;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import org.jenkinsci.plugins.pipeline.modeldefinition.parser.RuntimeASTTransformer;
import org.junit.BeforeClass;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
Expand Down Expand Up @@ -194,6 +195,9 @@ public void environmentWithWorkspace() throws Exception {
@Issue("JENKINS-42753")
@Test
public void stmtExprInEnvironment() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

expect("environment/stmtExprInEnvironment")
.logContains("FOO is BAR",
"LIST_EXP is [a, BAR, c]",
Expand All @@ -211,6 +215,9 @@ public void stmtExprInEnvironment() throws Exception {

@Test
public void nonLiteralEnvironment() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

initGlobalLibrary();

expect("environment/nonLiteralEnvironment")
Expand Down
4 changes: 4 additions & 0 deletions ...efinition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/LibrariesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import jenkins.plugins.git.GitSCMSource;
import org.jenkinsci.plugins.pipeline.modeldefinition.actions.ExecutionModelAction;
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStages;
import org.jenkinsci.plugins.pipeline.modeldefinition.parser.RuntimeASTTransformer;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.libs.FolderLibraries;
import org.jenkinsci.plugins.workflow.libs.GlobalLibraries;
Expand Down Expand Up @@ -130,6 +131,9 @@ public void librariesDirective() throws Exception {
@Issue("JENKINS-38110")
@Test
public void librariesDirectiveWithOutsideVarAndFunc() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

otherRepo.init();
otherRepo.write("vars/myecho.groovy", "def call() {echo 'something special'}");
otherRepo.write("vars/myecho.txt", "Says something very special!");
Expand Down
83 changes: 82 additions & 1 deletion ...l-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/MatrixTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,6 @@ public void matrixStageDirectives() throws Exception {
.go();
}


// Behavior should be identical to previous, just expressed differently
@Issue("JENKINS-41334")
@Test
Expand Down Expand Up @@ -371,6 +370,88 @@ public void matrixStageDirectivesChildStage() throws Exception {
.go();
}

@Issue("JENKINS-64846")
@Test
public void matrixStageDirectivesOutsideVarAndFunc() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

expect("matrix/matrixStageDirectivesOutsideVarAndFunc")
.logContains("[Pipeline] { (foo)",
"{ (Branch: Matrix - OS_VALUE = 'linux')",
"{ (Branch: Matrix - OS_VALUE = 'windows')",
"{ (Branch: Matrix - OS_VALUE = 'mac')",
"First stage, mac agent",
"First stage, do not override",
"First stage, overrode once and done",
"First stage, overrode twice, in first mac-os branch",
"First stage, overrode per nested, in first mac-os branch",
"First stage, declared per nested, in first mac-os branch",
"First stage, Hi there - This comes from a function",
"First stage, windows agent",
"First stage, do not override",
"First stage, overrode once and done",
"First stage, overrode twice, in first windows-os branch",
"First stage, overrode per nested, in first windows-os branch",
"First stage, declared per nested, in first windows-os branch",
"First stage, Hi there - This comes from a function",
"First stage, linux agent",
"First stage, do not override",
"First stage, overrode once and done",
"First stage, overrode twice, in first linux-os branch",
"First stage, overrode per nested, in first linux-os branch",
"First stage, declared per nested, in first linux-os branch",
"First stage, Hi there - This comes from a function",
"Apache Maven 3.0.1",
"Apache Maven 3.0.1",
"Apache Maven 3.0.1")
.logNotContains("WE SHOULD NEVER GET HERE",
"java.lang.IllegalArgumentException",
"override in matrix axis")
.go();
}

@Issue("JENKINS-64846")
@Test
public void matrixStageDirectivesOutsideVarAndFuncNoSplitting() throws Exception {
// ensure vars in matrix still works with splitting transform turned off
RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION = false;

expect("matrix/matrixStageDirectivesOutsideVarAndFunc")
.logContains("[Pipeline] { (foo)",
"{ (Branch: Matrix - OS_VALUE = 'linux')",
"{ (Branch: Matrix - OS_VALUE = 'windows')",
"{ (Branch: Matrix - OS_VALUE = 'mac')",
"First stage, mac agent",
"First stage, do not override",
"First stage, overrode once and done",
"First stage, overrode twice, in first mac-os branch",
"First stage, overrode per nested, in first mac-os branch",
"First stage, declared per nested, in first mac-os branch",
"First stage, Hi there - This comes from a function",
"First stage, windows agent",
"First stage, do not override",
"First stage, overrode once and done",
"First stage, overrode twice, in first windows-os branch",
"First stage, overrode per nested, in first windows-os branch",
"First stage, declared per nested, in first windows-os branch",
"First stage, Hi there - This comes from a function",
"First stage, linux agent",
"First stage, do not override",
"First stage, overrode once and done",
"First stage, overrode twice, in first linux-os branch",
"First stage, overrode per nested, in first linux-os branch",
"First stage, declared per nested, in first linux-os branch",
"First stage, Hi there - This comes from a function",
"Apache Maven 3.0.1",
"Apache Maven 3.0.1",
"Apache Maven 3.0.1")
.logNotContains("WE SHOULD NEVER GET HERE",
"java.lang.IllegalArgumentException",
"override in matrix axis")
.go();
}

@Issue("JENKINS-41334")
@Test
public void matrixStageDirectivesWhenBeforeAgent() throws Exception {
Expand Down
10 changes: 10 additions & 0 deletions ...-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/OptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import jenkins.model.BuildDiscarder;
import jenkins.model.BuildDiscarderProperty;
import org.jenkinsci.plugins.pipeline.modeldefinition.actions.DeclarativeJobPropertyTrackerAction;
import org.jenkinsci.plugins.pipeline.modeldefinition.parser.RuntimeASTTransformer;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
import org.jenkinsci.plugins.workflow.flow.FlowExecution;
Expand Down Expand Up @@ -179,6 +180,9 @@ public void checkoutToSubdirectory() throws Exception {
@Issue("JENKINS-44277")
@Test
public void checkoutToSubdirectoryWithOutsideVarAndFunc() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

expect("options/checkoutToSubdirectoryWithOutsideVarAndFunc")
.logContains("[Pipeline] { (foo)",
"hello")
Expand Down Expand Up @@ -484,6 +488,9 @@ public void rateLimitBuilds() throws Exception {
@Issue("JENKINS-46354")
@Test
public void topLevelRetryExecutesAllStages() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

expect("options/topLevelRetryExecutesAllStages")
.logContains("Actually executing stage Bar")
.go();
Expand All @@ -492,6 +499,9 @@ public void topLevelRetryExecutesAllStages() throws Exception {
@Issue("JENKINS-46354")
@Test
public void parentStageRetryExecutesAllChildStages() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

expect("options/parentStageRetryExecutesAllChildStages")
.logContains("Actually executing stage Bar", "Actually executing stage Baz")
.go();
Expand Down
4 changes: 4 additions & 0 deletions ...finition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ParametersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import hudson.model.ParametersDefinitionProperty;
import hudson.model.PasswordParameterDefinition;
import hudson.model.StringParameterDefinition;
import org.jenkinsci.plugins.pipeline.modeldefinition.parser.RuntimeASTTransformer;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
Expand Down Expand Up @@ -70,6 +71,9 @@ public void simpleParameters() throws Exception {

@Test
public void simpleParametersWithOutsideVarAndFunc() throws Exception {
// this should have same behavior whether script splitting is enable or not
RuntimeASTTransformer.SCRIPT_SPLITTING_ALLOW_LOCAL_VARIABLES = true;

WorkflowRun b = expect("simpleParametersWithOutsideVarAndFunc")
.logContains("[Pipeline] { (foo)", "hello true: Hi there - This comes from a function")
.logNotContains("[Pipeline] { (" + SyntheticStageNames.postBuild() + ")")
Expand Down
Loading