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

Vendored Guava should not use reflection #117

Merged
merged 7 commits into from
Dec 15, 2021
Merged

Vendored Guava should not use reflection #117

merged 7 commits into from
Dec 15, 2021

Conversation

basil
Copy link
Member

@basil basil commented Mar 14, 2021

Simpler and reflection-free alternative to #114 (CC @timja).

org.jenkinsci.plugins.workflow.support.concurrent.ChainingListenableFuture, org.jenkinsci.plugins.workflow.support.concurrent.Futures, and org.jenkinsci.plugins.workflow.support.concurrent.ListFuture are private copies of Guava classes specifically for use by Pipeline (in particular, they are being used by workflow-cps, workflow-cps-checkpoint, workflow-job, and workflow-support). The reasoning for making private copies is given in Kohsuke's comment:

Mostly copied after Guava's Futures, because that one is still marked as beta and is subject to change.

The whole point of vendoring these classes was to insulate Pipeline from Guava API changes. The problem here is that Kohsuke just didn't go far enough with this design, since what he copied and pasted itself depended on APIs marked as beta and subject to change. This change just completes what Kohsuke started by vendoring the rest of the Guava related code in the call hierarchy. I also cherry-picked some fixes and updates from upstream to get the vendored version up-to-date.

This is all consistent with the original design of vendoring the relevant Futures functionality for use in Pipeline. But one might also ask if this design still makes sense in a Guava 30+ world where these APIs are no longer beta. Of course it doesn't, so once Jenkins core is on a newer version of Guava and the baseline has been updated in all the Pipeline plugins, these vendored classes can be removed. Accordingly I've marked the existing ones as @Deprecated and the new ones as @Restricted(DoNotUse.class) in anticipation of this eventual removal.

I'll test this with workflow-cps and workflow-job which use the vendored classes. It would be great if a CloudBees employee could test this change with workflow-cps-checkpoint which I don't have access to.

basil added 6 commits March 14, 2021 07:36
Pull in updates from:

    MOE_MIGRATED_REVID=27031992
Pull in updates from:

    MOE_MIGRATED_REVID=31833262
Pull in updates from:

    MOE_MIGRATED_REVID=71753284
    MOE_MIGRATED_REVID=72669239
@@ -30,6 +29,7 @@
* in a {@code UndeclaredThrowableException} so that this class can get
* access to the cause.
*/
@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is only used from workflow-support. Deprecating it so that workflow-support can prefer the upstream version once core is on a recent version of Guava and workflow-support's baseline has been updated.

@@ -208,7 +208,7 @@ public void run() {
ChainingListenableFuture.this.outputFuture = null;
}
}
}, Futures.newExecutorService());
}, MoreExecutors.directExecutor());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undoing a change made in #114 in favor of the upstream version. We should generally avoid changing vendored upstream code.

* execute}.
*/
@GwtCompatible
@Restricted(NoExternalUse.class)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken without change from Guava 30.1. Used only by the vendored code. Marked as restricted to prevent any further usages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be added as a comment in the source?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the commit message should cover the provenance well enough:

commit ec98115 Add MoreExecutors#directExecutor() implementation from Guava 30.1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though git blame will not show that if this PR is squash-merged, as @car-roll normally seems to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i noticed that line too. I was going to make a special exception :)

@@ -45,6 +41,7 @@
*
* @author Guava
*/
@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is used from workflow-cps, workflow-cps-checkpoint, workflow-job, and workflow-support. Deprecating it so that these plugins can prefer the upstream version once core is on a recent version of Guava and the plugins' baselines have been updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which could now be whenever we like, right?

Also some of these methods could be deprecated in favor of Java Platform equivalents, like CompletableFuture.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we okay with doing that in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

* @since 3.0
*/
@GwtCompatible(emulated = true)
@Restricted(NoExternalUse.class)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken without change from Guava 30.1. Used only by the vendored code. Marked as restricted to prevent any further usages.

@@ -37,6 +36,7 @@
* each component future to fill out the value in the List when that future
* completes.
*/
@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is only used from workflow-support. Deprecating it so that workflow-support can prefer the upstream version once core is on a recent version of Guava and workflow-support's baseline has been updated.

@car-roll car-roll merged commit 20f48f1 into jenkinsci:master Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants