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

[MSHARED-817] Change eclipse aether dependency scope to provided #2

Merged
merged 6 commits into from Oct 25, 2019

Conversation

belingueres
Copy link
Contributor

  • Changed eclipse aether dependency to provided scope.
  • Removed shading of aether classes.
  • Added method to import aether library inside TransferUtils to use from
    a plugin.
  • Modified ProjectDeployerMojo and ProjectInstallerMojo plugins used for
    ITs.

@rfscholte
Copy link
Contributor

This commit looks kind of dangerous to me. The integration goals show how a any plugin should use this library, the classRealm is for Maven internal only and is public due to design restrictions.

@rfscholte rfscholte self-requested a review May 1, 2019 15:41
@belingueres
Copy link
Contributor Author

@rfscholte Would it be OK if the classRealm handling is inside the artifact-transfer library instead of inside the plugin's code?

@rfscholte
Copy link
Contributor

That would be better, bet this library should not assume it is always used within a Maven Classrealm. There should be a good fallback.

@belingueres
Copy link
Contributor Author

belingueres commented May 9, 2019

Hi @rfscholte. I tracked this issue and it is related to MNG-5901, where the package org.eclipse.aether.util was removed from the extensions.xml exports.
I've tested it adding org.eclipse.aether.util back again into extensions.xml in 3.6.2-SNAPSHOT: Core ITs show all OK, and tested with maven-project-deployer-plugin (from artifact-transfer) and it works OK without importing the package as part of the library (3.6.2-SNAPSHOT only, previous mvn versions still need the package import).

@rfscholte
Copy link
Contributor

That's great news. I guess this means that plugins/extensions trying to upload/download are forced to use maven-artifact-transfer. That's probably fine assuming this is done by our own maven-plugins

@belingueres
Copy link
Contributor Author

So should we open a new issue to revert MNG-5901?

Copy link
Contributor

@rfscholte rfscholte left a comment

Choose a reason for hiding this comment

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

Changes look interesting and would solve the shading issue that's still required. Just a small number of remarks.

- Changed eclipse aether dependency to provided scope.
- Removed shading of aether classes.
- Added method to import aether library inside TransferUtils to use from
a plugin.
- Modified ProjectDeployerMojo and ProjectInstallerMojo plugins used for
ITs.
the library.

- Moved and renamed class to
org.apache.maven.shared.transfer.project.MavenAetherUtils.
- No dependency with org.codehaus.plexus:plexus-classworlds is required.
- Added an IT.
- added optional classworlds dependency.
- Adapted MavenAetherUtils.importAether() method.
@rfscholte rfscholte merged commit 5d86480 into apache:master Oct 25, 2019
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