[MNG-8112] Honor export descriptors from the extension classpath - #13101
goutamadwant wants to merge 2 commits into
Conversation
Read package and artifact exports from maven.ext.class.path entries in both CLI implementations. Reuse descriptor discovery without moving extensions into the core realm or changing extension precedence. Cover JAR and directory entries, optional descriptors, package isolation, artifact filtering, and mixed extension loading with unit tests and Core ITs.
gnodet-bot
left a comment
There was a problem hiding this comment.
Solid fix. The approach correctly reads extension.xml descriptors from -Dmaven.ext.class.path entries and feeds their exported packages/artifacts into CoreExports, so ClassRealmManager can properly filter them when building plugin realms. Both CLI implementations (compat 3.x and impl 4.x) are semantically equivalent despite their structural differences. The ordering is correct — setupContainerRealm already adds the classpath URLs to the container realm before discoverFrom is called, so descriptor scanning works against the right classloader. The list mutation in PlexusContainerCapsuleFactory is safe because setupContainerRealm makes a defensive copy.
Test coverage is thorough: unit tests verify package export visibility, artifact filtering, optional descriptors, and multi-entry classpath merging. The integration tests exercise both classpath-only and classpath-plus-extensions.xml scenarios end-to-end.
One nit below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @Deprecated | ||
| class MavenCliExtensionClasspathTest { |
There was a problem hiding this comment.
💡 Nit: @Deprecated marks this class itself as deprecated, which is not the intent — you want to suppress deprecation warnings from using the deprecated MavenCli. The correct annotation is @SuppressWarnings("deprecation").
| class MavenCliExtensionClasspathTest { | |
| @SuppressWarnings("deprecation") |
There was a problem hiding this comment.
@gnodet-bot Replaced @Deprecated with @SuppressWarnings("deprecation"). The CLI tests and module verification pass. Thanks for catching this.
Use SuppressWarnings instead of marking the regression test itself deprecated. Keep the warning suppression scoped to the test class.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 79dfab82: the @Deprecated → @SuppressWarnings("deprecation") nit from the previous review has been addressed. No new issues introduced.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Fixes #10506.
Discover
META-INF/maven/extension.xmlfrommaven.ext.class.pathentries and include their package and artifact exports in both CLI implementations. This makes exported packages visible to plugins and filters exported artifacts from plugin dependencies.Reuse the existing descriptor reader. Keep extension classloader isolation and duplicate-extension precedence unchanged. Cover JARs, directories, multiple entries, optional descriptors, and loading alongside
.mvn/extensions.xml.Validation with JDK 21: both new Core ITs fail against the unmodified distribution and pass with the fix.
mvn -B -ntp verifyand the fullmvn -B -ntp -Prun-its -Dits.forkCount=2 verifypassed, including package-visibility and artifact-filtering regressions for both CLI implementations.Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.