-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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-60449] Fix and edge case of OptionalExtension blowing up Jenkins #4393
Conversation
* Variant plugin is a dependency of optional-depender Optional-depender contains the following class @OptionalExtension(requirePlugins = {"dependee"}) public class OptionalDependerExtension { private static void foo(Dependee d) { } }
…extension refresh
b16a88c
to
4a290e5
Compare
@jglick Updated the PR with a simpler diff (no more refactoring, and a new failing test instead of updating an existing one) |
Inspect all declared constructors/methods/fields recursively to exclude failing classes from Guice rather than have Guice explode. This also fix the failing test, but the previous commit is still better because it avoids an extra Jenkins.get().refreshExtensions() call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so over my head, so as a core pr reviewer I'm going to abstain from this one but its cool to try to figure out what's going on
+1 for tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, code looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine though it would be great to run PCT against the patched version
By the way: google/guice#1283 |
See JENKINS-60449.
Includes binaries from ikedam/dependencytest#2
If you have an extension like this
and
dependee
is not loaded initially, but you load it dynamically later, this blows up, because when Guice does a method scan and can't resolveDependee
because it is not yet in classloader at the time of scanning.This PR can be reviewed commit by commit.
Proposed changelog entries
Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@mention