Repository navigation
Fix mvnup compatibility: jar-plugin 3.4.2, exec-plugin submodule coverage, toolchain warning - #13179
Merged
Merged
Conversation
gnodet-bot
reviewed
Sep 18, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Backport looks clean. Cross-POM property upgrade logic is correct, pre-release downgrade paths are well-tested, and the jar-plugin target version change is properly justified.
One Javadoc nit on the new IT (see inline).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet
force-pushed
the
backport/13059-to-4.0.x
branch
from
September 18, 2026 11:18
546f867 to
9b238cd
Compare
gnodet-bot
reviewed
Sep 18, 2026
…rage, toolchain warning * Fix mvnup compatibility issues: jar-plugin target, exec-plugin submodule 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> * Address review findings: fix Files.walk resource leak and verify warning - 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> * Fix property-based plugin version resolution across multi-module projects 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. * Fix spurious warning when property already at target version 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. * Fix mvnup regressions from compat test run on maven-4.0.x-mvnup-fixes 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. * Fix debug log messages: 'needs upgrade to' -> 'needs version change to' * spotless:apply * Fix mvnup jar-plugin target: 3.3.1→3.4.1, add IT for upgrade version verification maven-jar-plugin 3.3.1 was never released (the series went 3.3.0→3.4.0), so targeting it would cause resolution failures at build time. The correct target is 3.4.1, which is the last version before the two regressions introduced in 3.4.2 via maven-archiver 3.6.3: - SOURCE_DATE_EPOCH=0 / 1970-01-01 timestamps rejected with an IllegalArgumentException (apache/maven-jar-plugin#595, still open) - Derived Automatic-Module-Name containing hyphens (e.g. from artifactId 'integration-test') causes a build failure (apache/maven-jar-plugin#596) 3.4.1 uses maven-archiver 3.6.2 which has neither issue, and includes the MJAR-307 commons-io ClassNotFound fix needed for Maven 3.9.6+ / Maven 4. Add MavenITMvnupJarPluginUpgradeTest to verify the upgrade target via an actual mvnup invocation, guarding against regressions like targeting a non-existent version. * Address review: LinkedHashMap in PomDiscovery, rename IT to MavenITgh13059, add Maven 4 build step - Switch PomDiscovery from HashMap to LinkedHashMap so that cross-POM property iteration is deterministic (parent discovered before children). Fixes the non-deterministic sibling-resolution issue flagged in review. - Rename MavenITMvnupJarPluginUpgradeTest → MavenITgh13059MvnupJarPlugin- UpgradeTest to follow the GH-NNNNN naming convention used by all other recent ITs in core-it-suite. - Add a real Maven 4 build step (mvn package) after the mvnup apply in the IT, so the test verifies that maven-jar-plugin 3.4.1 actually works under Maven 4, not just that the version string is written correctly. - Add a minimal Java source (Hello.java) to the IT test project so that the package phase has something to compile and jar. * Address review: add idempotency step to MavenITgh13059 IT * Address review nit: use StandardCharsets.UTF_8 in Files.readString * Replace Unicode em dashes with ASCII hyphens in PluginUpgradeStrategy --------- Signed-off-by: Guillaume Nodet <gnodet@gmail.com>
gnodet
force-pushed
the
backport/13059-to-4.0.x
branch
from
September 18, 2026 11:51
9b238cd to
0b1dbe1
Compare
gnodet-bot
previously requested changes
Sep 18, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review on 0b1dbe1.
The sibling-guard fix in upgradePropertyVersion and the @since 4.0.0-rc-7 nit are both addressed. But the IT class introduced in this push has two compile errors against the maven-4.0.x base — it won't build on this branch.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet
dismissed
gnodet-bot’s stale review
September 18, 2026 12:50
The CHANGES_REQUESTED review was based on incorrect assumptions about the maven-4.0.x API. The IT file already compiles cleanly: ALL_MAVEN_VERSIONS exists in AbstractMavenIntegrationTestCase on this branch, extractResources() returns File (not Path), and the @SInCE 4.0.0-rc-7 nit was addressed in the same commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #13059 to
maven-4.0.x.Cherry-pick of c00bc1d, applied cleanly with no conflicts.