From af1995070706a86da15b53f23871839c00375b82 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 23 Oct 2020 07:02:09 -0400 Subject: [PATCH 1/3] Disposer.requiresWorkspace is final --- .../jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java index 47cf3d19..15c63945 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java @@ -351,8 +351,6 @@ public void setUp(@Nonnull Context context, @Nonnull Run build, @Nonnull T context.setDisposer(new DisposerWithoutWorkspaceRequirement()); } public static final class DisposerWithoutWorkspaceRequirement extends Disposer { - // TODO: Remove once the minimum core version for this plugin is 2.258 or newer. - public boolean requiresWorkspace() { return false; } @Override public void tearDown(@Nonnull Run build, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws IOException, InterruptedException { listener.getLogger().println("<<< workspace context not needed, but provided."); } From 666817ee912f04f4aca1980b5df66d9b64333f8a Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 23 Oct 2020 07:36:34 -0400 Subject: [PATCH 2/3] Updating baseline and removing reflection --- pom.xml | 6 +- .../plugins/workflow/steps/CoreStep.java | 32 +------- .../workflow/steps/CoreWrapperStep.java | 76 ++----------------- .../plugins/workflow/steps/CoreStepTest.java | 23 ++---- .../workflow/steps/CoreWrapperStepTest.java | 15 ++-- 5 files changed, 20 insertions(+), 132 deletions(-) diff --git a/pom.xml b/pom.xml index 18cb64a1..39aee728 100644 --- a/pom.xml +++ b/pom.xml @@ -28,7 +28,7 @@ org.jenkins-ci.plugins plugin - 4.7 + 4.12 org.jenkins-ci.plugins.workflow @@ -64,7 +64,7 @@ 2.23 -SNAPSHOT - 2.249.1 + 2.263 8 true @@ -72,7 +72,7 @@ io.jenkins.tools.bom - bom-2.249.x + bom-2.249.x 12 import pom diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreStep.java index e7e9f4f3..28546bae 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreStep.java @@ -37,8 +37,6 @@ import hudson.model.TaskListener; import hudson.tasks.Builder; import hudson.tasks.Publisher; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -85,22 +83,7 @@ private static final class Execution extends SynchronousNonBlockingStepExecution final Launcher launcher = ctx.get(Launcher.class); final TaskListener listener = Objects.requireNonNull(ctx.get(TaskListener.class)); final EnvVars env = Objects.requireNonNull(ctx.get(EnvVars.class)); - boolean workspaceRequired = true; - // In Jenkins 2.258, a SimpleBuildStep can indicate that it does not require a workspace context by - // overriding a requiresWorkspace() method to return false. So use that if it's available. - // Note: this uses getMethod() on the delegate's type and not SimpleBuildStep so that an implementation can - // get this behaviour without switching to Jenkins 2.258 itself. - // TODO: Use 'workspaceRequired = this.delegate.requiresWorkspace()' once this plugin depends on Jenkins 2.258 or later. - try { - final Method requiresWorkspace = this.delegate.getClass().getMethod("requiresWorkspace"); - workspaceRequired = (boolean) requiresWorkspace.invoke(this.delegate); - } catch(NoSuchMethodException e) { - // ok, default to true - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } - if (workspaceRequired) { + if (delegate.requiresWorkspace()) { if (workspace == null) { throw new MissingContextVariableException(FilePath.class); } @@ -115,18 +98,7 @@ private static final class Execution extends SynchronousNonBlockingStepExecution if (workspace != null && launcher != null) { delegate.perform(run, workspace, env, launcher, listener); } else { - // If we get here, workspaceRequired is false and there is no workspace context. In that case, the - // overload of perform() introduced in Jenkins 2.258 MUST exist. - // Note: this uses getMethod() on the delegate's type and not SimpleBuildStep so that an implementation - // can get this behaviour without switching to Jenkins 2.258 itself. - // TODO: Use 'this.delegate.perform(run, env, listener)' once the minimum core version for this plugin is 2.258 or newer. - final Method perform = this.delegate.getClass().getMethod("perform", Run.class, EnvVars.class, TaskListener.class); - try { - perform.invoke(this.delegate, run, env, listener); - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } + delegate.perform(run, env, listener); } return null; } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java index 7dbb6f20..4c2beb98 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java @@ -34,8 +34,6 @@ import hudson.model.TaskListener; import hudson.tasks.BuildWrapperDescriptor; import java.io.IOException; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -94,20 +92,7 @@ private static final class Execution2 extends GeneralNonBlockingStepExecution { } private void doStart() throws Exception { - SimpleBuildWrapper.Context c = null; - // In Jenkins 2.258, a createContext() method on SimpleBuildWrapper is required to ensure that a Disposer - // registered on that context inherits the wrapper's workspace requirement. Use it when available. - // TODO: Use 'c = this.delegate.createContext()' once this plugin depends on Jenkins 2.258 or later. - try { - final Method createContext = SimpleBuildWrapper.class.getMethod("createContext"); - c = (SimpleBuildWrapper.Context) createContext.invoke(this.delegate); - } - catch (NoSuchMethodException e) { - c = new SimpleBuildWrapper.Context(); - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } + SimpleBuildWrapper.Context c = delegate.createContext(); final StepContext context = getContext(); final Run run = context.get(Run.class); assert run != null; @@ -118,22 +103,7 @@ private void doStart() throws Exception { assert env != null; final FilePath workspace = context.get(FilePath.class); final Launcher launcher = context.get(Launcher.class); - boolean workspaceRequired = true; - // In Jenkins 2.258, a SimpleBuildWrapper can indicate that it does not require a workspace context by - // overriding a requiresWorkspace() method to return false. So use that if it's available. - // Note: this uses getMethod() on the delegate's type and not SimpleBuildWrapper so that an - // implementation can get this behaviour without switching to Jenkins 2.258 itself. - // TODO: Use 'workspaceRequired = this.delegate.requiresWorkspace()' once this plugin depends on Jenkins 2.258 or later. - try { - final Method requiresWorkspace = this.delegate.getClass().getMethod("requiresWorkspace"); - workspaceRequired = (boolean) requiresWorkspace.invoke(this.delegate); - } catch(NoSuchMethodException e) { - // ok, default to true - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } - if (workspaceRequired) { + if (delegate.requiresWorkspace()) { if (workspace == null) { throw new MissingContextVariableException(FilePath.class); } @@ -147,16 +117,7 @@ private void doStart() throws Exception { } else { // If we get here, workspaceRequired is false and there is no workspace context. In that case, the // overload of setUp() introduced in Jenkins 2.258 MUST exist. - // Note: this uses getMethod() on the delegate's type and not SimpleBuildWrapper so that an - // implementation can get this behaviour without switching to Jenkins 2.258 itself. - // TODO: Use 'this.delegate.setUp(c, run, listener, env)' once the minimum core version for this plugin is 2.258 or newer. - final Method perform = this.delegate.getClass().getMethod("setUp", SimpleBuildWrapper.Context.class, Run.class, TaskListener.class, EnvVars.class); - try { - perform.invoke(this.delegate, c, run, listener, env); - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } + delegate.setUp(c, run, listener, env); } } BodyInvoker bodyInvoker = context.newBodyInvoker(); @@ -219,23 +180,7 @@ private static final class Callback extends BodyExecutionCallback.TailCall { assert listener != null; final FilePath workspace = context.get(FilePath.class); final Launcher launcher = context.get(Launcher.class); - boolean workspaceRequired = true; - // In Jenkins 2.258, a Disposer has a final requiresWorkspace() method that indicates whether or not it (or - // more accurately, its associated wrapper) requires a workspace context. This is set up via the Context, as - // long as that is created via SimpleBuildWrapper.createContext(). - // Note: this uses getMethod() on the disposer's type and not SimpleBuildWrapper.Disposer so that an - // implementation can get this behaviour without switching to Jenkins 2.258 itself. - // TODO: Use 'workspaceRequired = this.disposer.requiresWorkspace()' once this plugin depends on Jenkins 2.258 or later. - try { - final Method requiresWorkspace = this.disposer.getClass().getMethod("requiresWorkspace"); - workspaceRequired = (boolean) requiresWorkspace.invoke(this.disposer); - } catch(NoSuchMethodException e) { - // ok, default to true - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } - if (workspaceRequired) { + if (disposer.requiresWorkspace()) { if (workspace == null) { throw new MissingContextVariableException(FilePath.class); } @@ -247,18 +192,7 @@ private static final class Callback extends BodyExecutionCallback.TailCall { if (workspace != null && launcher != null) { this.disposer.tearDown(run, workspace, launcher, listener); } else { - // If we get here, workspaceRequired is false and there is no workspace context. In that case, the - // overload of tearDown() introduced in Jenkins 2.258 MUST exist. - // Note: this uses getMethod() on the disposer's type and not SimpleBuildWrapper.Disposer so that an - // implementation can get this behaviour without switching to Jenkins 2.258 itself. - // TODO: Use 'this.disposer.tearDown(run, listener)' once the minimum core version for this plugin is 2.258 or newer. - final Method perform = this.disposer.getClass().getMethod("tearDown", Run.class, TaskListener.class); - try { - perform.invoke(this.disposer, run, listener); - } catch (InvocationTargetException ite) { - final Throwable realException = ite.getCause(); - throw realException instanceof Exception ? (Exception) realException : ite; - } + disposer.tearDown(run, listener); } } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreStepTest.java index 9326c7a6..7f3497ad 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreStepTest.java @@ -207,16 +207,10 @@ public static class BuilderWithWorkspaceRequirement extends Builder implements S @DataBoundConstructor public BuilderWithWorkspaceRequirement() { } - // TODO: Remove once the minimum core version for this plugin is 2.258 or newer. - public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { - fail("This method should not get called."); - } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void perform(@Nonnull Run run, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws InterruptedException, IOException { + @Override public void perform(@Nonnull Run run, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws InterruptedException, IOException { listener.getLogger().println("workspace context required, but not provided!"); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull EnvVars env, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { + @Override public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull EnvVars env, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { listener.getLogger().println("workspace context required and provided."); } // While the @TextExtension supposedly limits this descriptor to the named test method, it still gets picked up @@ -247,20 +241,13 @@ public static class BuilderWithoutWorkspaceRequirement extends Builder implement @DataBoundConstructor public BuilderWithoutWorkspaceRequirement() { } - // TODO: Remove once the minimum core version for this plugin is 2.258 or newer. - public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { - fail("This method should not get called."); - } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void perform(@Nonnull Run run, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws InterruptedException, IOException { + @Override public void perform(@Nonnull Run run, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws InterruptedException, IOException { listener.getLogger().println("workspace context not needed."); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull EnvVars env, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { + @Override public void perform(@Nonnull Run run, @Nonnull FilePath workspace, @Nonnull EnvVars env, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { listener.getLogger().println("workspace context not needed, but provided."); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public boolean requiresWorkspace() { + @Override public boolean requiresWorkspace() { return false; } // While the @TextExtension supposedly limits this descriptor to the named test method, it still gets picked up diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java index 15c63945..d230b570 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java @@ -293,8 +293,7 @@ public static final class WrapperWithWorkspaceRequirement extends SimpleBuildWra listener.getLogger().println(">>> workspace context required and provided."); context.setDisposer(new DisposerWithWorkspaceRequirement()); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void setUp(@Nonnull Context context, @Nonnull Run build, @Nonnull TaskListener listener, @Nonnull EnvVars initialEnvironment) throws IOException, InterruptedException { + @Override public void setUp(@Nonnull Context context, @Nonnull Run build, @Nonnull TaskListener listener, @Nonnull EnvVars initialEnvironment) throws IOException, InterruptedException { listener.getLogger().println(">>> workspace context required but not provided!"); context.setDisposer(new DisposerWithWorkspaceRequirement()); } @@ -302,8 +301,7 @@ public static final class DisposerWithWorkspaceRequirement extends Disposer { @Override public void tearDown(@Nonnull Run build, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws IOException, InterruptedException { listener.getLogger().println("<<< workspace context required and provided."); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void tearDown(@Nonnull Run build, @Nonnull TaskListener listener) throws IOException, InterruptedException { + @Override public void tearDown(@Nonnull Run build, @Nonnull TaskListener listener) throws IOException, InterruptedException { listener.getLogger().println("<<< workspace context required but not provided!"); } } @@ -339,14 +337,12 @@ public void wrapperWithWorkspaceRequirement() throws Exception { public static final class WrapperWithoutWorkspaceRequirement extends SimpleBuildWrapper { @DataBoundConstructor public WrapperWithoutWorkspaceRequirement() { } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public boolean requiresWorkspace() { return false; } + @Override public boolean requiresWorkspace() { return false; } @Override public void setUp(@Nonnull Context context, @Nonnull Run build, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener, @Nonnull EnvVars initialEnvironment) throws IOException, InterruptedException { listener.getLogger().println(">>> workspace context not needed, but provided."); context.setDisposer(new DisposerWithoutWorkspaceRequirement()); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void setUp(@Nonnull Context context, @Nonnull Run build, @Nonnull TaskListener listener, @Nonnull EnvVars initialEnvironment) throws IOException, InterruptedException { + @Override public void setUp(@Nonnull Context context, @Nonnull Run build, @Nonnull TaskListener listener, @Nonnull EnvVars initialEnvironment) throws IOException, InterruptedException { listener.getLogger().println(">>> workspace context not needed."); context.setDisposer(new DisposerWithoutWorkspaceRequirement()); } @@ -354,8 +350,7 @@ public static final class DisposerWithoutWorkspaceRequirement extends Disposer { @Override public void tearDown(@Nonnull Run build, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws IOException, InterruptedException { listener.getLogger().println("<<< workspace context not needed, but provided."); } - // TODO: Mark as @Override once the minimum core version for this plugin is 2.258 or newer. - public void tearDown(@Nonnull Run build, @Nonnull TaskListener listener) throws IOException, InterruptedException { + @Override public void tearDown(@Nonnull Run build, @Nonnull TaskListener listener) throws IOException, InterruptedException { listener.getLogger().println("<<< workspace context not needed."); } } From 18ce7fc6096b20bfb302ab4f0507cef67817b345 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 23 Oct 2020 09:45:05 -0400 Subject: [PATCH 3/3] Forgot to delete one obsolete comment --- .../org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java index 4c2beb98..c7e7c3d1 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java @@ -115,8 +115,6 @@ private void doStart() throws Exception { if (workspace != null && launcher != null) { this.delegate.setUp(c, run, workspace, launcher, listener, env); } else { - // If we get here, workspaceRequired is false and there is no workspace context. In that case, the - // overload of setUp() introduced in Jenkins 2.258 MUST exist. delegate.setUp(c, run, listener, env); } }