Make workflow-support compatible with Guava 21.0 and newer #114
Conversation
|
seriously -.-, old pinned version in Jenkinsfile... |
pom.xml
Outdated
| @@ -64,11 +64,12 @@ | |||
| <properties> | |||
| <revision>3.8</revision> | |||
| <changelist>-SNAPSHOT</changelist> | |||
| <jenkins.version>2.222.4</jenkins.version> | |||
| <jenkins.version>2.273-rc30689.09147672b85b</jenkins.version> | |||
There was a problem hiding this comment.
Can you please link to whatever Jenkins core PR produced this incremental so reviewers have more context about the proposed change? Thanks!
| @@ -324,7 +324,7 @@ public void run() { | |||
| public static <V> ListenableFuture<List<V>> allAsList( | |||
| ListenableFuture<? extends V>... futures) { | |||
| return new ListFuture<V>(ImmutableList.copyOf(futures), true, | |||
| MoreExecutors.sameThreadExecutor()); | |||
| MoreExecutors.newDirectExecutorService()); | |||
There was a problem hiding this comment.
Since this is since Guava 18, maybe we could use reflection to try newDirectExecutorService then fall back to sameThreadExecutor, then revert the core bump?
Or copy the impl into this plugin, keeping the Apache license header.
There was a problem hiding this comment.
nice idea, when I was working on it (and plan to get back to it, although others are welcome to take it up) I was trying to get a green bom PCT build and then look at what's required and see where changes like your suggestion could be applied.
There was a problem hiding this comment.
I have this change ready to push. Is it ok if I push to your branch @timja?
There was a problem hiding this comment.
also see jenkinsci/workflow-api-plugin#135 and jenkinsci/bom#423
There was a problem hiding this comment.
Errr, I don't have push permission here, so I've filed a PR to your forked branch: timja#1
There was a problem hiding this comment.
also see jenkinsci/workflow-api-plugin#135 and jenkinsci/bom#423
I'll have a look. Meanwhile, isn't this PR mergeable as is?
src/main/java/org/jenkinsci/plugins/workflow/support/concurrent/Futures.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/concurrent/Futures.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
dwnusbaum
changed the title
|
I updated the PR title and description to reflect the proposed change. |
src/main/java/org/jenkinsci/plugins/workflow/support/concurrent/Futures.java
Outdated
Show resolved
Hide resolved
|
@car-roll this will need a release too. (Or https://github.com/jenkinsci/incrementals-tools#automatic-deployment if you are sick of my bugging you about stuff like this 😁.) |
Not yet! 😁 But I should probably look into that stuff still... |
See jenkinsci/jenkins#5059.
workflow-supportpreviously usedMoreExecutors.sameThreadExecutorfrom Guava, which was removed in version 21.0 in favor ofMoreExecutors.newDirectExecutorService. This PR uses reflection to first try to callMoreExecutors.newDirectExecutorServicebut then callMoreExecutors.sameThreadExecutorif the first method is not available to support both older and newer versions of Guava.