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

Restrict classloader for Maven 4 plugins #1336

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Dec 7, 2023

No description provided.

@@ -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>
Copy link
Contributor

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

Copy link
Contributor Author

@gnodet gnodet Dec 7, 2023

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants