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

Two regressions prevent targeting anything newer than 3.3.1:

3.4.2+: timestamp range validation (via maven-archiver) rejects SOURCE_DATE_EPOCH=0 / project.build.outputTimestamp=1970-01-01T00:00:00Z, a common reproducible-builds convention used by Debian and many Apache projects (apache/maven-jar-plugin#595, still open as of 3.5.1)
3.5.0: JarToolModularJarArchiver NPE on Windows with JDK < 14 when a reproducible-builds timestamp is set (apache/maven-jar-plugin#555) — fixed in 3.5.1 via plexus-archiver 4.12.0, but the 3.4.2+ timestamp regression remains

mvnup now targets 3.3.1, the latest stable version without either regression. Will be updated to 3.5.x once #595 is fixed.

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.

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

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

Re-review of b7a5b55d84d0 ("Fix debug log messages: 'needs upgrade to' → 'needs version change to'").

Previous observation (2026-09-08) — fully addressed:

✅ Debug messages in analyzePluginsFromEffectiveModel now say "needs version change to" instead of "needs upgrade to", which is accurate for both upgrade (3.1.0 → 3.3.1) and downgrade (4.0.0-beta-1 → 3.3.1) scenarios. All three occurrences updated consistently.

Static analysis (ast-grep): All flagged items (broad-exception-catch ×4) are pre-existing patterns — nothing introduced by this commit.

All previous findings across the full PR remain 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

Comment thread ...-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java Outdated

Comment thread ...li/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/ToolchainPluginStrategyTest.java

gnodet commented Sep 8, 2026

View reviewed changes

Comment thread ...aven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java

Comment thread ...-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java Outdated

gnodet commented Sep 8, 2026

View reviewed changes

gnodet commented Sep 9, 2026

View reviewed changes

Fix debug log messages: 'needs upgrade to' -> 'needs version change to'

b7a5b55

gnodet-bot approved these changes Sep 11, 2026

View reviewed changes

spotless:apply

4363887

Navigation Menu

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 7 commits into
apache:masterfrom
gnodet:fix/mvnup-compat-issues

gnodet commented Sep 6, 2026 •

edited

Loading

Uh oh!

gnodet left a comment

Uh oh!

Uh oh!

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

Uh oh!

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!

gnodet-bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gnodet commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

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

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!

Uh oh!

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!

Uh oh!

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!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gnodet commented Sep 6, 2026 •

edited

Loading