Conversation
|
Thanks. There are a lot of export, for example in this implementation module. Are all these exported packages public API? If not, are there some exports that we could omit, or restrict to qualified exports? |
|
Claude Code on behalf of Guillaume Nodet Good point. None of the maven-impl:
Other impl modules:
Left unchanged:
Since all consumers currently use |
Add explicit module-info.java to 17 modules (11 API + 6 impl) and Automatic-Module-Name manifest entries to all 17 remaining modules. Only org.apache.maven.api.* packages are public API. All implementation packages use qualified exports restricted to internal consumers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
There was a problem hiding this comment.
Self-review notes — the JPMS module support looks correctly implemented across all 34 modules. All 17 module-info.java module names are consistent with their POM javaModuleName properties, the qualified exports in maven-impl properly restrict internal packages, and the build infrastructure changes (javadoc exclusions, legacyMode, annotationProcessorPaths, useModulePath=false) are appropriate.
Minor items to fix in the PR description
-
PR body mentions
maven-executor— Under "Modules with Automatic-Module-Name only", the description lists "Impl (4):maven-core,maven-cli,maven-executor,maven-testing". There are actually 3 impl modules in this category —maven-executordoes not exist in the project's module structure. -
Unrelated fix not in diff — The PR body mentions "AbstractUpgradeGoal: Fixed domtrip API change
children()→childElements()" under "Unrelated fix", but this change does not appear in the diff. It may have been dropped during a rebase.
Neither of these affects the code — only the PR description text.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet
left a comment
There was a problem hiding this comment.
Automated review — well-structured JPMS migration adding module-info.java to 17 modules and Automatic-Module-Name to 16 remaining modules. All CI checks pass across all platforms (Linux, macOS, Windows) and Java versions (17, 21, 25).
Strengths:
- Excellent consistency: all 33
javaModuleNamePOM properties exactly match their corresponding module-info.java module names. No split packages found. - The null-safety fixes in
DefaultModelProcessorandDefaultModelBuilderare correct and necessary. - Qualified exports in
maven-implproperly restrict all 14org.apache.maven.impl.*packages to the 5 internal consumers. - Build infrastructure changes are all well-motivated (
annotationProcessorPaths,useModulePath=falsefor surefire,sourceFileExcludesfor javadoc,legacyMode=truefor aggregate javadoc).
Minor observations (non-blocking):
- The
open moduledeclaration formaven-implgrants deep reflective access; targetedopensdirectives might reduce the surface area but may not be practical given the number of DI consumers. - The woodstox-core and stax2-api dependencies in
maven-supportare transitively available but explicitly needed for JPMSrequires— a comment noting this reasoning would help future maintainers.
No issues found that would block this PR.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
gnodet
left a comment
There was a problem hiding this comment.
Well-executed JPMS modularization with correct module descriptors, appropriate qualified exports for internal APIs, and sound design decisions for handling split-package constraints. All 23 CI checks pass green.
✅ Strengths
- Module name consistency verified: all 17 modules with
module-info.javahave theirjavaModuleNamePOM property matching the module declaration exactly. All 16Automatic-Module-Nameentries are consistent. - Split-package analysis is well-founded: 13 compat modules and 3 impl modules cannot use
module-info.javadue to 20+ split packages between impl/compat boundaries. UsingAutomatic-Module-Namefor these is correct. - Qualified exports in
maven-implare consistent — all 14 implementation packages restricted to the same 5 internal consumers (maven-core,maven-cling,maven-testing,maven-compat,maven-embedder). - The
open moduledeclaration formaven-implis justified for DI framework reflective access. - API modules appropriately use
requires transitivefor inter-API dependencies where types leak across module boundaries. - Null-safety fixes in
DefaultModelProcessorandDefaultModelBuilderare correct defensive changes. - Javadoc plugin configuration is well thought out (
sourceFileExcludesformodule-info.java,legacyMode=truefor aggregate reports).
Minor observations (non-blocking)
org.apache.maven.api.diexportsorg.apache.maven.di.tool(non-API-namespaced package in an API module) — reasonable for co-located annotation processor, but a comment explaining why could help.- New explicit deps on
woodstox-core/stax2-apiinmaven-support— previously transitive, now required for JPMSrequiresdirectives. Correct but widens declared surface.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
# Conflicts: # impl/maven-core/pom.xml
gnodet-bot
left a comment
There was a problem hiding this comment.
CI failure traced to maven-xml/module-info.java: the requires static plexus.xml directive fails compilation because plexus-xml is declared with scope=test in impl/maven-xml/pom.xml — it is therefore absent from the main compile module path.
The compiler error is:
module not found: plexus.xml
The fix is either to make plexus-xml a compile-scoped dependency, or to remove the requires static plexus.xml directive if it is not actually needed at compile time (the requires static org.eclipse.sisu.plexus on the adjacent line may already cover what you need, or neither directive may be necessary if these jars are only used by test code).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
plexus-xml 4.2.0 uses Automatic-Module-Name 'org.codehaus.plexus.util.xml', not 'plexus.xml'. Moreover, plexus-xml is test-scoped in maven-xml and no main sources use classes from it, so the requires directive is unnecessary.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after new commits.
✅ Prior finding addressed
The CHANGES_REQUESTED finding from 2026-09-19 — requires static plexus.xml in impl/maven-xml/src/main/java/module-info.java was build-breaking because plexus-xml is test-scoped and therefore absent from the main compile module path — is fixed. The new module-info.java correctly uses requires static org.eclipse.sisu.plexus instead, and the Woodstox/StAX2 dependencies required for com.ctc.wstx and org.codehaus.stax2 are now declared as explicit compile-scope entries in the POM.
Review of new changes
The full JPMS modularization across 33 modules reviewed:
- API modules (11) — correct
requires transitivebetween inter-dependent API modules;java.compilerinmaven-api-corejustified byJavaPathType.java's use ofjavax.tools.*;maven-api-modelcorrectly declaresuses ModelObjectProcessor. maven-impl—open modulefor DI reflective access; 14 implementation packages qualified to 5 internal consumers;maven-api-spiandmaven-apiviarequires transitive;annotationProcessorPathsfor DiIndexProcessor;useModulePath=falsefor surefire — all correct.maven-di— Qualifiedorg.apache.maven.di.implto the right 5 consumers; publicorg.apache.maven.diunqualified as the DI API;annotationProcessorPathsfor self-annotation processing — correct.maven-jline—opens org.apache.maven.jline to org.apache.maven.difor reflective instantiation of@Named/@Singletonbeans;annotationProcessorPaths— correct. Norequires static org.jline.terminal.ffm(correctly removed from prior version).maven-logging— No DI annotations in the module source, noannotationProcessorPathsneeded;provides org.slf4j.spi.SLF4JServiceProvidervia ServiceLoader — correct.maven-xml—requires static org.eclipse.sisu.plexusis compile-optional for Plexus XML compatibility; explicitwoodstox-coreandstax2-apicompile-scope deps in POM — correct.maven-support— Promotedwoodstox-core/stax2-apito compile scope (was transitive) — necessary and correct for JPMSrequires.- Compat modules (13) —
Automatic-Module-Nameonly; correct decision given split packages preventing named module status. - Null-safety fixes —
DefaultModelProcessorandDefaultModelBuildernull-guard on@Nullablecollections — correct. - Root POM —
maven-jar-pluginAutomatic-Module-Namevia${javaModuleName}property;sourceFileExcludesformodule-info.javain javadoc;legacyMode=truein reporting profile — all appropriate.
No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewing after 897f0e0.
Previous finding addressed ✅ — requires static plexus.xml correctly removed. The commit message explains both the wrong module name and the test-scope-only usage; the fix is clean.
New issue found ❌ — same class of bug as before, different dependency: requires plexus.sec.dispatcher in maven-impl/module-info.java (line 39). The Automatic-Module-Name in plexus-sec-dispatcher-4.2.0.jar (the version declared in the root POM) is org.codehaus.plexus.components.secdispatcher, not plexus.sec.dispatcher. This will produce the same module not found compilation error on CI.
Fix: change requires plexus.sec.dispatcher → requires org.codehaus.plexus.components.secdispatcher.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
The Automatic-Module-Name in plexus-sec-dispatcher-4.2.0.jar is org.codehaus.plexus.components.secdispatcher, not plexus.sec.dispatcher. Using the wrong name causes a 'module not found' compile error.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 3bff5fa.
✅ Prior finding addressed
The CHANGES_REQUESTED from 897f0e0 — requires plexus.sec.dispatcher in maven-impl/module-info.java — is fixed. The new commit corrects it to requires org.codehaus.plexus.components.secdispatcher, which matches the actual Automatic-Module-Name entry in plexus-sec-dispatcher-4.2.0.jar (verified from the JAR manifest).
✅ All prior findings resolved
897f0e0:requires static plexus.xmlremoved (wrong module name + test-scope-only dep) ✅3bff5fa:requires plexus.sec.dispatcher→requires org.codehaus.plexus.components.secdispatcher✅
CI is pending on the new push — but this is a single-line mechanical fix with the correct module name verified against the JAR. No other issues found in the diff.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Summary
module-info.javato 17 modules (11 API + 6 impl) with properrequires,exports,uses, andprovidesdirectivesAutomatic-Module-Namemanifest entries to all 16 remaining modules (3 impl + 13 compat) viamaven-jar-pluginconfiguration in root POMorg.apache.maven.api.*is public API--add-exportsfor plexus-interpolation)DefaultModelProcessorandDefaultModelBuilderDetails
Modules with
module-info.java(17)API modules (11):
maven-api-annotations,maven-api-di,maven-api-xml,maven-api-metadata,maven-api-model,maven-api-settings,maven-api-toolchain,maven-api-plugin,maven-api-core,maven-api-spi,maven-api-cliImpl modules (6):
maven-di,maven-xml,maven-support,maven-impl(open module),maven-jline,maven-loggingModules with
Automatic-Module-Nameonly (16)Impl (3):
maven-core,maven-cli,maven-testing— cannot usemodule-info.javadue to split packages with compat modules or complex DI/compat dependenciesCompat (13): All compat modules — split packages among themselves prevent named module status
Qualified exports for non-API packages
Only
org.apache.maven.api.*packages are Maven 4's public API. All implementation packages use qualified exports restricted to internal consumers:org.apache.maven.impl.*exports qualified tomaven-core,maven-cli,maven-testing,maven-compat,maven-embedder. Removed unusedorg.apache.maven.impl.model.profile. Onlyorg.apache.maven.api.services.modelremains unqualified (it's an API package).org.apache.maven.di.implqualified to internal consumers (keptorg.apache.maven.diunqualified as DI API)org.apache.maven.slf4jqualified tomaven-cling,maven-embedder,maven-coreorg.apache.maven.jlinequalified tomaven-logging,maven-cling,maven-embedderSince all consumers currently compile in classpath mode (no
module-info.java), the qualified exports serve as documentation of intent and will be enforced when those modules gain module descriptors.Build infrastructure changes
maven-jar-pluginin<pluginManagement>setsAutomatic-Module-Namefrom${javaModuleName}propertymaven-javadoc-pluginconfigured withsourceFileExcludesformodule-info.javato prevent javadoc tool from switching to modular modelegacyMode=truefor aggregate javadoc — forces classpath mode to avoid "named and unnamed modules" conflict--add-exports=org.codehaus.plexus.interpolation/org.codehaus.plexus.interpolation.utilfor javadoc — plexus-interpolation 1.29 does not export the.utilpackage in its module descriptorannotationProcessorPathsforDiIndexProcessor— required when compiling with module-info.java since the processor is no longer on the classpathuseModulePath=falsein surefire to preserve existing test behavior on the classpathBug fixes included
nullfor@Nullablecollections when no providers exist; added null-safe defaults (Map.of()/List.of())ProjectBuildLogAppendercreation to occur before terminal initializationrequires static org.jline.terminal.ffm— the FFM terminal provider is discovered via ServiceLoader at runtime, and therequires staticdirective fails compilation on Java 17 CI where the module doesn't existNotable design decisions
maven-implis anopen modulebecause Maven's DI framework needs reflective access to instantiate componentsmaven-cliusesAutomatic-Module-Nameinstead ofmodule-info.javadue to split packages with compat modules it depends on (e.g.,org.apache.maven.settingsin bothmaven-settingsandmaven-core)requires transitivefor inter-API dependencies since API types leak across module boundariesmaven-api-modeldeclaresuses ModelObjectProcessorfor ServiceLoader supportmaven-impldeclaresuses RootDetectorfor ServiceLoader supportTest plan
mvn verify -B— all modules compile and tests passmvn site -Preporting) passjar --describe-moduleshows correct module descriptors for all 17 module-info modulesAutomatic-Module-Namemanifest entries present in all 16 remaining module JARs🤖 Generated with Claude Code