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.

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-70528] node / dir / node on same agent sets PWD to that of dir rather than @2 workspace #292

Merged
merged 6 commits into from
Feb 16, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,19 @@ public final class ExecutorStepDynamicContext implements Serializable {
final @NonNull ExecutorStepExecution.PlaceholderTask task;
final @NonNull String node;
final @NonNull String path;
final int depth;
/** Non-null after {@link #resume} if all goes well. */
private transient @Nullable Executor executor;
/** Non-null after {@link #resume} if all goes well. */
private transient @Nullable WorkspaceList.Lease lease;

ExecutorStepDynamicContext(ExecutorStepExecution.PlaceholderTask task, WorkspaceList.Lease lease, Executor executor) {
ExecutorStepDynamicContext(ExecutorStepExecution.PlaceholderTask task, WorkspaceList.Lease lease, Executor executor, int depth) {
this.task = task;
this.node = FilePathUtils.getNodeName(lease.path);
this.path = lease.path.getRemote();
this.executor = executor;
this.lease = lease;
this.depth = depth;
}

void resume(StepContext context) throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -914,11 +914,9 @@ private final class PlaceholderExecutable implements ContinuableExecutable, Acce
env.put("WORKSPACE_TMP", tempDir.getRemote()); // JENKINS-60634
}
FlowNode flowNode = context.get(FlowNode.class);
if (flowNode != null) {
flowNode.addAction(new WorkspaceActionImpl(workspace, flowNode));
}
flowNode.addAction(new WorkspaceActionImpl(workspace, flowNode));
listener.getLogger().println("Running on " + ModelHyperlinkNote.encodeTo(node) + " in " + workspace);
ExecutorStepDynamicContext state = new ExecutorStepDynamicContext(PlaceholderTask.this, lease, exec);
ExecutorStepDynamicContext state = new ExecutorStepDynamicContext(PlaceholderTask.this, lease, exec, FilePathDynamicContext.depthOf(flowNode));
withExecution(execution -> {
execution.state = state;
execution.body = context.newBodyInvoker()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.workflow.FilePathUtils;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.steps.BodyInvoker;
import org.jenkinsci.plugins.workflow.steps.DynamicContext;
import org.jenkinsci.plugins.workflow.support.pickles.FilePathPickle;
Expand All @@ -61,11 +62,13 @@
}
ExecutorStepDynamicContext esdc = context.get(ExecutorStepDynamicContext.class);
LOGGER.fine(() -> "ESDC=" + esdc + " FPR=" + r);
if (esdc != null && !esdc.node.equals(r.slave)) {
LOGGER.fine(() -> "skipping " + r.path + "@" + r.slave + " since it is on a different node than " + esdc.node);
return null;
} else if (esdc != null && !esdc.path.equals(r.path)) {
LOGGER.fine(() -> "not skipping " + r.path + "@" + r.slave + " even though it is in a different workspace than " + esdc.node + "@" + esdc.path);
if (esdc != null) {
if (esdc.depth > r.depth) {
LOGGER.fine(() -> "skipping " + r.path + "@" + r.slave + " since at depth " + r.depth + " it is shallower than " + esdc.node + "@" + esdc.path + " at depth " + esdc.depth);
return null;
} else {
LOGGER.fine(() -> "not skipping " + r.path + "@" + r.slave + " since at depth " + r.depth + " it is deeper than " + esdc.node + "@" + esdc.path + " at depth " + esdc.depth);
}
}
FilePath f = FilePathUtils.find(r.slave, r.path);
if (f != null) {
Expand All @@ -91,10 +94,22 @@
}

/**
* @see BodyInvoker#withContext
* @deprecated use {@link #createContextualObject(FilePath, FlowNode)}
*/
@Deprecated
public static Object createContextualObject(FilePath f) {
return new FilePathRepresentation(FilePathUtils.getNodeName(f), f.getRemote());
return new FilePathRepresentation(FilePathUtils.getNodeName(f), f.getRemote(), 0);
}

/**
* @see BodyInvoker#withContext
*/
public static Object createContextualObject(@NonNull FilePath f, @NonNull FlowNode n) {
return new FilePathRepresentation(FilePathUtils.getNodeName(f), f.getRemote(), depthOf(n));
}

static int depthOf(@NonNull FlowNode n) {
return n.getEnclosingBlocks().size();
}

static final class FilePathRepresentation implements Serializable {
Expand All @@ -103,10 +118,12 @@ static final class FilePathRepresentation implements Serializable {

private final String slave;
private final String path;
private final int depth;

FilePathRepresentation(String slave, String path) {
FilePathRepresentation(String slave, String path, int depth) {
this.slave = slave;
this.path = path;
this.depth = depth;
}

@Override public String toString() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import hudson.FilePath;
import hudson.model.TaskListener;
import java.util.Set;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
import org.jenkinsci.plugins.workflow.steps.Step;
import org.jenkinsci.plugins.workflow.steps.StepContext;
Expand Down Expand Up @@ -72,7 +73,7 @@ public String getPath() {
}

@Override public Set<? extends Class<?>> getRequiredContext() {
return Set.of(TaskListener.class, FilePath.class);
return Set.of(TaskListener.class, FilePath.class, FlowNode.class);
}

@Override public Set<? extends Class<?>> getProvidedContext() {
Expand All @@ -95,7 +96,7 @@ public static class Execution extends StepExecution {
FilePath dir = getContext().get(FilePath.class).child(path);
getContext().get(TaskListener.class).getLogger().println("Running in " + dir);
getContext().newBodyInvoker()
.withContext(FilePathDynamicContext.createContextualObject(dir))
.withContext(FilePathDynamicContext.createContextualObject(dir, getContext().get(FlowNode.class)))
// Could use a dedicated BodyExecutionCallback here if we wished to print a message at the end ("Returning to ${cwd}"):
.withCallback(BodyExecutionCallback.wrap(getContext()))
.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public boolean start() throws Exception {
getContext().newBodyInvoker()
.withContexts(
EnvironmentExpander.merge(getContext().get(EnvironmentExpander.class), EnvironmentExpander.constant(env)),
FilePathDynamicContext.createContextualObject(workspace))
FilePathDynamicContext.createContextualObject(workspace, flowNode))
.withCallback(new Callback(lease))
.start();
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,11 @@ public class ExecutorStepDynamicContextTest {
p.setDefinition(new CpsFlowDefinition("node('alpha') {node('beta') {echo(/here ${pwd()}!/)}}", true));
j.assertLogContains("here " + big.getWorkspaceFor(p).getRemote() + "@2!", j.buildAndAssertSuccess(p));
p.setDefinition(new CpsFlowDefinition("node('alpha') {ws('alphadir') {node('beta') {echo(/here ${pwd()}!/)}}}", true));
/* TODO does not yet work; FilePathDynamicContext from ws('alphadir') takes precedence:
j.assertLogContains("here " + big.getWorkspaceFor(p).getRemote() + "@2!", j.buildAndAssertSuccess(p));
*/
j.assertLogContains("here " + big.getRootPath().child("alphadir").getRemote() + "!", j.buildAndAssertSuccess(p));
p.setDefinition(new CpsFlowDefinition("node('alpha') {node('beta') {ws('betadir') {echo(/here ${pwd()}!/)}}}", true));
j.assertLogContains("here " + big.getRootPath().child("betadir").getRemote() + "!", j.buildAndAssertSuccess(p));
p.setDefinition(new CpsFlowDefinition("node('alpha') {dir('alphadir') {node('beta') {echo(/here ${pwd()}!/)}}}", true));
/* TODO same, with dir('alphadir'):
j.assertLogContains("here " + big.getWorkspaceFor(p).getRemote() + "@2!", j.buildAndAssertSuccess(p));
*/
j.assertLogContains("here " + big.getWorkspaceFor(p).child("alphadir").getRemote() + "!", j.buildAndAssertSuccess(p));
p.setDefinition(new CpsFlowDefinition("node('alpha') {node('beta') {dir('betadir') {echo(/here ${pwd()}!/)}}}", true));
j.assertLogContains("here " + big.getWorkspaceFor(p).sibling(big.getWorkspaceFor(p).getBaseName() + "@2").child("betadir").getRemote() + "!", j.buildAndAssertSuccess(p));
});
Expand Down