| Hey Jesse Glick, I started working on this over the weekend and achieved some preliminary success converting workflow-support to work with both Guava 11 and Guava 28.1. You can see my work-in-progress changes here. While running some integration tests with the above, I found three remaining odds and ends. In workflow-basic-steps's TimeoutStepExecution we directly call MoreExecutors.sameThreadExecutor, which was removed in Guava 21:
workflow-basic-steps-plugin/src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java
import com.google.common.util.concurrent.MoreExecutors;
[…]
// TODO would use Futures.addCallback but this is still @Beta in Guava 19 and the Pipeline copy is in workflow-support on which we have no dep
currentExecutions.addListener(new Runnable() {
@Override public void run() {
[…]
}
}, MoreExecutors.sameThreadExecutor());
In workflow-api's FlowExecutionList we call com.google.common.util.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) directly (as opposed to org.jenkinsci.plugins.workflow.support.concurrent.Futures#addCallback(ListenableFuture, FutureCallback)):
workflow-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java
import com.google.common.util.concurrent.Futures;
[…]
Futures.addCallback(e.getCurrentExecutions(false), new FutureCallback<List<StepExecution>>() {
[…]
});
com.google.common.util.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) was removed in Guava 26, yielding errors like this even with my workflow-support fixes:
java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V
at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$ItemListenerImpl.onLoaded(FlowExecutionList.java:180)
at jenkins.model.Jenkins.<init>(Jenkins.java:989)
at hudson.model.Hudson.<init>(Hudson.java:85)
Finally, workflow-step-api's StepExecution uses Futures#allAsList:
workflow-step-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/steps/StepExecution.java
import com.google.common.util.concurrent.Futures;
[…]
return Futures.allAsList(futures);
This hasn't changed in the latest version of Guava, but it makes me nervous that we are using com.google.common.util.concurrent.Futures anywhere in Pipeline rather than the safer org.jenkinsci.plugins.workflow.support.concurrent.Futures. What is to be done for these three cases? workflow-api depends on workflow-step-api and workflow-support depends on workflow-api. So one solution would be to move the org.jenkinsci.plugins.workflow.support.concurrent package out of workflow-support. But where should it go? Since it is an internal API for pipeline, I think a logical place would be a org.jenkinsci.plugins.workflow.concurrent package in workflow-api. But then workflow-step-api wouldn't have access to it. Putting the org.jenkinsci.plugins.workflow.concurrent package in workflow-step-api doesn't seem like a good place either, since it isn't clearly related to the Step API. And making a whole new plugin for this seems overkill. So I can't think of a good solution here. Maybe we should put it in workflow-api and copy and paste a private restricted copy in workflow-step-api? That isn't the cleanest solution, but it's the best compromise I can think of right now. Here's my proposed plan of action. Let me know what you think.
- Open PR ① against workflow-api to copy the org.jenkinsci.plugins.workflow.support.concurrent package from workflow-support into a org.jenkinsci.plugins.workflow.concurrent package in workflow-api.
- Open PR ② against workflow-api (downstream of PR ①) with the changes from here to make org.jenkinsci.plugins.workflow.concurrent.Futures work with newer Guava versions.
- Open PR ③ against workflow-step-api (downstream of PR ②) to make a restricted copy of the needed org.jenkinsci.plugins.workflow.concurrent classes from PR ② and consume them in StepExecution.
- Open PR ④ against workflow-support (downstream of PRs ② and ③) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and deprecate all
org.jenkinsci.plugins.workflow.support.concurrent classes.
- Open PR ⑤ against workflow-cps (downstream of PRs ②, ③, and ④) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and to update CpsThreadGroup to use org.jenkinsci.plugins.workflow.concurrent.Futures rather than com.google.common.util.concurrent.Futures.
- Open PRs ⑥, ⑦, and ⑧ against workflow-basic-steps, workflow-durable-task, and workflow-job (downstream of PRs ②, ③, ④, and ⑤) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and to start using org.jenkinsci.plugins.workflow.concurrent.Futures#addCallback in TimeoutStepExecution rather than the current code referenced above which uses MoreExecutors directly.
- Open "do not merge" PRs ⑨, ⑩, ⑪, ⑫, ⑬, ⑭, and ⑮ against workflow-api, workflow-step-api, workflow-support, workflow-cps, workflow-basic-steps, workflow-durable-task, and workflow-job (downstream of the preceding PRs above) with a Guava update to Guava 28.1 to demonstrate that all preceding work results in Pipeline being compatible with the newest version of Guava. The preceding PRs would have demonstrated that it still works with Guava 11.
This is a formidable amount of work, so before I get started I wanted to get your feedback. |
