-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Restrict classloader for Maven 4 plugins #1336
base: master
Are you sure you want to change the base?
Conversation
@@ -187,6 +189,7 @@ under the License. | |||
<exportedArtifact>org.apache.maven.resolver:maven-resolver-util</exportedArtifact> | |||
<exportedArtifact>org.apache.maven.resolver:maven-resolver-connector-basic</exportedArtifact> | |||
|
|||
<exportedArtifact>jakarta.inject:jakarta.inject-api</exportedArtifact> |
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.
don't think it should land there, ideally we shouldn't have it (we saw that javax was an issue already) but if imposed by some part of the stack (not sure which one since I thought we stayed on javax intentionally) we should just hide it from any consumer IMHO
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.
We can't. We need it in the new api. It's used to define scopes (those can't work without the package) and it's also used to inject various components.
See #1309
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.
Or do you mean not adding it for Maven 3.x plugins ? That would be absolutely fine with 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.
not adding at all to anything, will bite us for sure, my understanding was that the guice upgrade was done under javax umbrella and/or we facade any needed api. Ad of today using javax.inject is fine cause code is frozen but anything jakarta can break without notice and can conflict with mojo so goes against maven4 api track for 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.
The only other solution I could think about is to rewrite the whole DI injection (Sisu + Guice) because they are not pluggable at all. I'm not ready for that...
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.
🤔 I don't see why:
- injections in mojo are already bridged
- abstracting injection markers (with a maven inject annotation) is easy in guice
- abstracting the injector or a lookup (like we had/have in plexus container) is quite trivial
- abstracting a scope is not crazy (wayless than what we already have)
So overall, even if I'm not sure which case(s) you want to cover, I don't see a reason to break our original hypotheis to define a clean and maven owned API and export anything jakarta related.
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.
I've raised https://issues.apache.org/jira/browse/MNG-7954
9955de3
to
ad1094e
Compare
No description provided.