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
Move from javax.inject
to jakarta.inject
#8065
Conversation
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.
Oh, that's nice
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.
LGTM. Should we leave a javax.inject test in ExtensionFinderTest to ensure we don't accidentally break this?
/label ready-for-merge This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback. |
Please take a moment and address the merge conflicts of your pull request. Thanks! |
import java.util.List; | ||
import javax.inject.Provider; |
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.
FYI this is a breaking API change because the public extension point uses Provider
in its API.
public abstract InstallState getNextInstallState(InstallState current, Provider<InstallState> proceed);
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.
Sorry, I had not noticed that before, but you are right. Fortunately I could not find any affected open source plugins, and only a small handful of CloudBees plugins appear to be affected: bluesteel
, cloudbees-license-plugin
, and cloudbees-assurance-plugin
. Since the migration is so easy (just change the imports) I am inclined to leave this as-is in both weeklies and LTS. Since we have to switch to Jakarta injection imports eventually to get to Guice 7, I believe that the benefits of releasing a version with full support for Jakarta injection imports (allowing us to eventually get to Guice 7) outweigh the small cost of adapting to this minor method signature change in a small handful of proprietary plugins. Feel free to let me know if this isn't feasible for some reason.
jenkinsci/jenkins#8065 was first included in 2.408 and was not backported to any of the 2.401.x releases. jenkinsci/jenkins#8121 was backported to 2.401.2.
jenkinsci/jenkins#8065 was first included in 2.408 and was not backported to any of the 2.401.x releases. jenkinsci/jenkins#8121 was backported to 2.401.2.
Without dropping support for
javax.inject
annotations, preferjakarta.inject
annotations in our own code wherever possible to facilitate eventually removing support forjavax.inject
when we move to Guice 7.Testing done
mvn clean verify -Dtest=hudson.ExtensionFinderTest,jenkins.bugs.Jenkins19124Test,jenkins.install.InstallUtilTest,lib.form.TextAreaTest
Proposed changelog entries
Add support for
jakarta.inject
annotations.Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).