Fix mvnup compatibility: jar-plugin 3.4.2, exec-plugin submodule coverage, toolchain warning by gnodet · Pull Request #13059 · apache/maven

gnodet · 2026-09-06T22:17:18Z

Summary

Three fixes for mvnup issues found during maven4-testing compatibility run:

1. maven-jar-plugin upgrade target: 3.5.0 → 3.4.2

maven-jar-plugin 3.5.0 includes a plexus-archiver regression where JarToolModularJarArchiver fails with "Could not create modular JAR file" for projects that repackage JARs (e.g., HBase's hbase-shaded-client-fixed via atlas). The fix is in plexus-archiver 4.12.0, not yet released as jar-plugin 3.5.1.

mvnup now targets 3.4.2 which is the latest stable version without this regression. Will be reverted to 3.5.0+ once jar-plugin 3.5.1 ships.

2. exec-maven-plugin submodule test coverage

Verified that #12200 (exec-maven-plugin target bump to 3.5.0) correctly covers submodule-level explicit version declarations. Added a multi-module integration test that simulates the hbase-assembly scenario: parent POM + child module declaring exec-maven-plugin:3.1.0 explicitly. The test confirms the upgrade to 3.5.0 works correctly in this scenario.

3. Toolchain JDK availability warning

When ToolchainPluginStrategy adds the maven-toolchains-plugin with select-jdk-toolchain goal, it now emits a warning that the required JDK must be installed and discoverable. This prevents silent build failures like netbeans-html4j where the toolchain request can't be satisfied because no compatible JDK is available.

Testing

All 749 tests in impl/maven-cli pass
Added 3 new unit tests + 1 new integration test
spotless clean

AI agent (Hermes on behalf of gnodet)

…ule test, toolchain warning - maven-jar-plugin: change upgrade target from 3.5.0 to 3.4.2 to avoid plexus-archiver regression (JarToolModularJarArchiver fails with 'Could not create modular JAR file' for projects that repackage JARs). Revert to 3.5.0 once jar-plugin 3.5.1 ships with plexus-archiver 4.12.0. - exec-maven-plugin: add test confirming apache#12200 fix covers submodule-level explicit version declarations (multi-module scenario with parent+child POMs on disk). - ToolchainPluginStrategy: emit warning when adding select-jdk-toolchain goal that a compatible JDK must be installed and discoverable, with link to plugin docs. Prevents silent build failures like netbeans-html4j where the required toolchain JDK is not available. Signed-off-by: Guillaume Nodet <gnodet@gmail.com>

gnodet

Two minor test-quality findings. The production changes (version bump + warning) are clean and well-tested.

Category: bug (workaround for jar-plugin regression) + enhancement (toolchain warning)
Milestone: 4.1.0 ✅ already set
Backport: backport-to-4.0.x label ✅ already set

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet · 2026-09-06T23:12:58Z

+                Files.walk(tempDir)
+                        .sorted(java.util.Comparator.reverseOrder())
+                        .map(Path::toFile)
+                        .forEach(java.io.File::delete);
+            }


⚠️ Resource leak: Files.walk() returns a Stream that wraps a DirectoryStream — it must be closed. Without try-with-resources, the file handle leaks if an exception is thrown during traversal.

The existing tests (line ~956) use the correct pattern. This should be consistent:

Suggested change

Files.walk(tempDir)

.sorted(java.util.Comparator.reverseOrder())

.map(Path::toFile)

.forEach(java.io.File::delete);

}

try (var walk = Files.walk(tempDir)) {

walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> {

try {

Files.delete(p);

} catch (IOException ignored) {

}

});

}

Also note the existing pattern uses Files.delete(p) directly instead of Path::toFile + File::delete, which gives better error diagnostics.

gnodet · 2026-09-06T23:12:58Z

+            Document doc = Document.of(pomXml);
+            UpgradeContext context = TestUtils.createMockContext();
+
+            UpgradeResult result = strategy.doApply(context, Map.of(POM_PATH, doc));
+
+            assertEquals(1, result.modifiedPoms().size());
+            assertTrue(strategy.hasToolchainsPluginWithSelectGoal(doc));
+
+            // The output should contain the toolchains plugin and version constraint
+            String xml = doc.toXml();
+            assertTrue(xml.contains("select-jdk-toolchain"), "POM should contain select-jdk-toolchain goal");
+            // The warning is emitted through the context logger — verified by integration tests
        }

        @Test


💡 Observation: This test is named shouldEmitJdkAvailabilityWarning but doesn't actually verify the warning was emitted — it only checks POM modification (same assertions as the test above). The mock context doesn't capture warning() calls.

The comment on line 469 acknowledges this ("verified by integration tests"), so this is more of a naming/clarity nit — but if you wanted to verify it here, you could use Mockito.verify(logger).warn(contains("must be installed")) on the mock logger.

- PluginUpgradeStrategyTest: wrap Files.walk() in try-with-resources and use Files.delete() instead of Path::toFile + File::delete, consistent with other cleanup blocks in the same file. - ToolchainPluginStrategyTest: verify warning emission via Mockito.verify(context.logger).warn(contains("must be installed")) instead of just checking POM modification. Signed-off-by: Guillaume Nodet <gnodet@gmail.com>

gnodet

Re-review after d19e12de22 ("Address review findings").

Both previous findings fully addressed:

✅ Files.walk() resource leak — now properly wrapped in try-with-resources, using Files.delete() for better error diagnostics, consistent with the existing cleanup pattern in the file.
✅ shouldEmitJdkAvailabilityWarning() now actually verifies the warning via verify(context.logger).warn(contains("must be installed")). The mock chain in TestUtils.createMockContext() correctly sets context.logger to a Mockito mock (via parserRequest.logger() in LookupContext), and UpgradeContext.warning() delegates to logger.warn(...), so the verify will fire correctly.

Static analysis (ast-grep, semgrep): all flagged items (broad-exception-catch, mutable-collection-return, empty-catch-block) are pre-existing patterns in the surrounding code — nothing introduced by this PR.

Production changes (jar-plugin target → 3.4.2, toolchain warning) remain clean and unchanged. No new issues. Ready to merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

…ects When a plugin version is a property reference like ${exec.maven.version}, upgradePropertyVersion now searches all POMs in the project (not just the current POM) for the property definition. This fixes the case where a submodule declares a plugin with a property-based version and the property is defined in a parent POM (e.g., the hbase pattern where root POM defines <exec.maven.version>3.1.0</exec.maven.version> and hbase-assembly uses <version>${exec.maven.version}</version>). The fix extracts upgradePropertyInDocument as a reusable helper and adds a fallback loop over the pomMap when the property is not found locally.

gnodet

Re-review of commit 700038a0 (third commit: property-based version resolution across multi-module projects).

Previous findings (from reviews on 2026-09-06 and 2026-09-08) are not affected — the Files.walk fix and toolchain warning verification remain intact.

The new multi-POM property search logic has one semantic bug that will produce spurious warnings for users.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet · 2026-09-08T06:33:53Z

+        // First, try the current POM's properties
+        if (upgradePropertyInDocument(pomDocument, propertyName, upgrade, sectionName, context)) {
+            return true;
+        }
+
+        // Property not found or not upgradable in current POM — search other POMs in the project
+        for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
+            Document otherDoc = entry.getValue();
+            if (otherDoc == pomDocument) {
+                continue; // Skip the current POM, already checked
+            }
+            if (upgradePropertyInDocument(otherDoc, propertyName, upgrade, sectionName, context)) {
+                return true;
+            }
+        }
+
+        // Property not found anywhere in the project
+        context.warning("Property " + propertyName + " not found in any project POM properties");


⚠️ Spurious warning when property is already at target version.

upgradePropertyInDocument() returns false for two distinct cases:

Property element not found → fall-through to search other POMs is correct

Property found but already at/above minimum → fall-through is wrong

In case 2, the loop finds nothing in sibling POMs (the property is in the current POM, not there), falls through to this line, and emits "Property X not found in any project POM properties" — which is factually wrong. The property was found; it just didn't need upgrading.

Concrete scenario: Root POM has <exec.maven.version>3.5.0</exec.maven.version> (already at target). Assembly submodule uses <version>${exec.maven.version}</version>. User runs mvnup. upgradePropertyInDocument on the current POM returns false (already at min), the pomMap loop finds nothing, warning fires. User sees a confusing "not found" message for a property that is perfectly defined.

Fix: track whether the property was found (regardless of upgrade outcome) to suppress the false warning. Add an existence check before the search loop:

Suggested change

// First, try the current POM's properties

if (upgradePropertyInDocument(pomDocument, propertyName, upgrade, sectionName, context)) {

return true;

}

// Property not found or not upgradable in current POM — search other POMs in the project

for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {

Document otherDoc = entry.getValue();

if (otherDoc == pomDocument) {

continue; // Skip the current POM, already checked

}

if (upgradePropertyInDocument(otherDoc, propertyName, upgrade, sectionName, context)) {

return true;

}

}

// Property not found anywhere in the project

context.warning("Property " + propertyName + " not found in any project POM properties");

// First, try the current POM's properties

if (upgradePropertyInDocument(pomDocument, propertyName, upgrade, sectionName, context)) {

return true;

}

// Check if property exists in the current POM but is already at/above min (no upgrade needed).

// In that case, skip the cross-POM search and the warning — the property IS defined.

Element currentRoot = pomDocument.root();

Element currentProps = currentRoot.childElement(PROPERTIES).orElse(null);

if (currentProps != null && currentProps.childElement(propertyName).isPresent()) {

return false; // Found in current POM, no upgrade needed

}

// Property not in current POM — search other POMs in the project (e.g., parent POM)

for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {

Document otherDoc = entry.getValue();

if (otherDoc == pomDocument) {

continue; // Skip the current POM, already checked

}

if (upgradePropertyInDocument(otherDoc, propertyName, upgrade, sectionName, context)) {

return true;

}

}

// Property not found anywhere in the project

context.warning("Property " + propertyName + " not found in any project POM properties");

return false;

Also needs a test: single POM with <exec.maven.version>3.5.0</exec.maven.version> + submodule using ${exec.maven.version} → no warning emitted, no modification.

Fixed in 63c472c. Added an existence check after upgradePropertyInDocument returns false — if the property is present in the current POM's <properties> (or any sibling POM's), we return false immediately without the cross-POM search or the spurious warning. Also added a test for the exact scenario (property already at 3.5.0 + submodule reference → no warning).

gnodet · 2026-09-08T06:33:53Z

+                Files.createDirectories(tempDir.resolve(".mvn"));
+                Path parentPomPath = tempDir.resolve("pom.xml");
+                Files.writeString(parentPomPath, parentPomXml);
+                Path assemblyDir = tempDir.resolve("assembly");
+                Files.createDirectories(assemblyDir);
+                Path submodulePomPath = assemblyDir.resolve("pom.xml");
+                Files.writeString(submodulePomPath, submodulePomXml);


💡 Nit: dead filesystem I/O in this test. doApply(context, pomMap) reads Document objects from the pomMap values — it does not read files from disk by path. The Files.createDirectories(tempDir.resolve(".mvn")) and Files.writeString(...) calls are never consumed by the strategy in a property-lookup scenario like this one. (The pre-existing tests that use a remote parent org.apache:apache:23 trigger effective-model resolution which does need the filesystem — different situation.)

These calls are harmless but add confusion. The test could be simplified: just construct Document.of(xml) and pass the in-memory map with any Path key (e.g., Paths.get("pom.xml")), same as jarPluginTargetShouldBe342() does.

Fixed in 63c472c. Removed the dead Files.createDirectories/Files.writeString calls — the tests now create a temp dir only for path structure (so relativize works in createTempProjectStructure), but skip writing the POM files since doApply handles that internally. Applied the same cleanup to shouldUpgradeExecPluginInSubmodule for consistency.

upgradePropertyVersion() emitted 'Property X not found in any project POM properties' when the property existed but was already at/above the minimum version. This happened because upgradePropertyInDocument() returns false for both 'not found' and 'already at target', causing the cross-POM search to run and the warning to fire. Add an existence check after upgradePropertyInDocument returns false: if the property is present in the current (or any) POM's <properties>, return false without searching further or warning. Also remove dead filesystem I/O from multi-POM tests — doApply handles temp directory writes internally via createTempProjectStructure.

gnodet

Re-review of 63c472cc51 ("Fix spurious warning when property already at target version").

Previous finding (2026-09-08T06:33) — fully addressed:

✅ Spurious warning when property already at target version — The existence-check guard is correctly placed in both code paths:

After upgradePropertyInDocument on the current POM: if the property element exists (regardless of upgrade outcome), return immediately without searching sibling POMs or warning.
Inside the sibling-POM loop: same guard per document, so a property found at/above minimum in any POM also suppresses the warning.

The logic is sound. upgradePropertyInDocument returns false for two distinct cases (not found / already at target), and the new guards correctly distinguish them by re-checking element presence. The test shouldNotWarnWhenPropertyAlreadyAtTargetVersion verifies the exact production scenario (parent POM has exec.maven.version=3.5.0, submodule references it via property → no warning emitted).

✅ Dead filesystem I/O in tests — The Files.createDirectories + Files.writeString calls removed from shouldUpgradePluginWithPropertyVersionInParentPom and shouldUpgradeExecPluginInSubmodule. The tests now only use Files.createTempDirectory for path key construction (harmless).

Static analysis (ast-grep): All flagged items (broad-exception-catch ×4, mutable-collection-return ×2) are pre-existing patterns in the surrounding code — nothing new introduced by this PR.

Production changes (jar-plugin target → 3.4.2, toolchain JDK availability warning, multi-POM property resolution) remain clean. All findings addressed — ready to merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Two issues found by running the full compatibility test suite: 1. maven-resources-plugin 4.0.0-beta-1 not downgraded to stable 3.3.1 The plugin upgrade entry had latestPreRelease=4.0.0-beta-1, so the pre-release handling path found the current version already >= the latest pre-release and left it unchanged. But 4.0.0-beta-1 is runtime- incompatible with Maven 4 rc-5 (NoSuchMethodError: ProjectManager. getResources). Fix: remove latestPreRelease, add new downgrade logic that detects 4.x pre-release versions with no latestPreRelease entry and downgrades them to the stable minVersion (3.3.1). 2. maven-jar-plugin target lowered from 3.4.2 to 3.3.1 jar-plugin 3.4.2 introduces two regressions observed across many projects: - Timestamp range validation rejects SOURCE_DATE_EPOCH=0 builds (1970-01-01T00:00:00Z not within valid range 1980-2099) - Stricter automatic module name validation rejects names derived from artifact IDs containing hyphens or empty strings 3.3.1 avoids both regressions. The 3.5.0 plexus-archiver regression (JarToolModularJarArchiver) is not fixed yet, so we stay at 3.3.1 until a clean 3.5.x release is available. Also extends the pre-release downgrade logic to the effective model analysis paths (build/plugins and build/pluginManagement) so that 4.x pre-release versions inherited from remote parents are also correctly identified as needing a management override.

Bukama · 2026-09-09T12:16:01Z

@gnodet One question: You linked this under "backport-to-maven-4.0" but milestone says its targets 4.1.0 only? So is it for 4.1.0 or is it a backport for 4.0.0-rc7?

gnodet · 2026-09-09T12:29:12Z

@gnodet One question: You linked this under "backport-to-maven-4.0" but milestone says its targets 4.1.0 only? So is it for 4.1.0 or is it a backport for 4.0.0-rc7?

This PR targets master, so 4.1.0. But we want to backport it, so we'll need another PR that will target 4.0.0-rc-7.
I think this looks better than having a PR targeting master with a rc-7 milestone. I used to do that, but this means changing the milestone at merge time anyway. Unless we want to keep both PRs at milestone rc-7. I'm really open to discussing that if you have a better suggestion for the workflow.

gnodet

Re-review of 05c3a06181 ("Fix mvnup regressions from compat test run on maven-4.0.x-mvnup-fixes").

Two production changes, both clean:

maven-resources-plugin: no latestPreRelease + new downgrade logic
The 4-arg PluginUpgrade constructor sets latestPreRelease=null. The new code path (isMaven4PreRelease && latestPreRelease == null) correctly downgrades to minVersion (3.3.1) instead of leaving the incompatible 4.0.0-beta-1 in place. The same logic is consistently applied in both upgradePluginVersion (direct version) and upgradePropertyInDocument (property-based version). ✅
maven-jar-plugin target: 3.4.2 → 3.3.1
Clean version bump. The 3.4.2 regressions (timestamp range validation rejecting SOURCE_DATE_EPOCH=0, stricter automatic module name checks) affect enough projects to warrant lowering the floor. 3.3.1 is safe. ✅
Effective model analysis extended for pre-release downgrade
Both build/plugins and build/pluginManagement/plugins loops now include || (isMaven4PreRelease(effectiveVersion) && upgrade.latestPreRelease() == null) — ensures 4.x pre-release versions inherited from remote parents are correctly flagged for management override. ✅

Tests:

shouldDowngradePreReleaseResourcesPluginToStable — direct version ✅
shouldDowngradePreReleaseResourcesPluginPropertyToStable — property-based version ✅
jarPluginTargetShouldBe331 (renamed from 342) — updated assertions ✅
jarPlugin342ShouldNotBeUpgraded — verifies 3.4.2 is preserved (already above 3.3.1 min) ✅
All 53 PluginUpgradeStrategyTest + 21 ToolchainPluginStrategyTest pass locally ✅

Minor observation (not blocking): The debug messages in the effective model analysis path say "needs upgrade to 3.3.1" even when the actual action is a downgrade from 4.0.0-beta-1. Since these are debug-level only, it's fine — but "needs version change to" would be more accurate if you ever touch this again.

Previous findings (Files.walk leak fix, toolchain warning verification, spurious property warning fix) remain intact and unaffected by this commit.

All findings addressed. Ready to merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet added the backport-to-4.0.x label Sep 6, 2026

gnodet marked this pull request as ready for review September 6, 2026 23:10

gnodet added this to the 4.1.0 milestone Sep 6, 2026

gnodet commented Sep 6, 2026

View reviewed changes

gnodet commented Sep 8, 2026

View reviewed changes

gnodet commented Sep 9, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix mvnup compatibility: jar-plugin 3.4.2, exec-plugin submodule coverage, toolchain warning - #13059

Fix mvnup compatibility: jar-plugin 3.4.2, exec-plugin submodule coverage, toolchain warning#13059
gnodet wants to merge 5 commits into
apache:masterfrom
gnodet:fix/mvnup-compat-issues

gnodet commented Sep 6, 2026

Uh oh!

gnodet left a comment

Uh oh!

gnodet Sep 6, 2026

Uh oh!

gnodet Sep 6, 2026

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet Sep 8, 2026

Uh oh!

gnodet Sep 8, 2026

Uh oh!

gnodet Sep 8, 2026

Uh oh!

gnodet Sep 8, 2026

Uh oh!

gnodet left a comment

Uh oh!

Bukama commented Sep 9, 2026

Uh oh!

gnodet commented Sep 9, 2026

Uh oh!

gnodet left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

gnodet commented Sep 6, 2026

Summary

1. maven-jar-plugin upgrade target: 3.5.0 → 3.4.2

2. exec-maven-plugin submodule test coverage

3. Toolchain JDK availability warning

Testing

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Bukama commented Sep 9, 2026

Uh oh!

gnodet commented Sep 9, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants