Skip to content

Fix #12530: add mvnup upgrade strategies for Maven 4 known compatibility issues#12532

Open
gnodet wants to merge 4 commits into
masterfrom
fix-12530-add-mvnup-upgrade-strategies-for-maven
Open

Fix #12530: add mvnup upgrade strategies for Maven 4 known compatibility issues#12532
gnodet wants to merge 4 commits into
masterfrom
fix-12530-add-mvnup-upgrade-strategies-for-maven

Conversation

@gnodet

@gnodet gnodet commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds 4 new mvnup upgrade strategies to address Maven 4 known compatibility issues from the maven4-testing project:

  • old-compiler-plugin-errorprone: Update maven-compiler-plugin minimum to 3.11.0 (older versions can't find ErrorProne plug-in under Maven 4 classloading)
  • beta-plugin-api-break: Detect and upgrade Maven 4 pre-release plugin versions (4.0.0-beta-1, 4.0.0-alpha-*, etc.) that use API methods renamed/removed before RC
  • old-scala-tools-plugin: Migrate org.scala-tools:maven-scala-pluginnet.alchim31.maven:scala-maven-plugin:4.9.5 (unmaintained since 2011)
  • duplicated-pom-tag: New DuplicateElementStrategy that removes duplicate XML elements (<artifactId>, <properties>, etc.) that Maven 4's stricter POM parser rejects

The remaining 4 strategies from the issue already existed:

  • scala-maven-plugin#911 → already in PLUGIN_UPGRADES at 4.9.5
  • ancient-enforcer-plugin → already in PLUGIN_UPGRADES at 3.5.0
  • exec-plugin-getcontainer → already in PLUGIN_UPGRADES at 3.5.0
  • duplicate-dependency-declarations → already in DeduplicateDependenciesStrategy

Changes

File Change
PluginUpgradeStrategy.java Updated compiler min to 3.11.0, added isMaven4PreRelease() detection, added PluginMigration support with migratePlugin() method
PluginMigration.java New record for plugin groupId/artifactId migrations
DuplicateElementStrategy.java New strategy (priority 21) that recursively removes duplicate XML elements using last-wins semantics, skipping list containers
PluginUpgradeStrategyTest.java 13 new tests: 9 for pre-release detection, 4 for plugin migration
DuplicateElementStrategyTest.java 18 new tests covering applicability, element removal, nesting, list container skipping

Test plan

  • All 476 mvnup tests pass (0 failures)
  • Format check passes (-Psourcecheck validate)
  • CI build passes

🤖 Generated with Claude Code

…ity issues

Add 4 new upgrade strategies (completing the 8 proposed in the issue):

1. Update maven-compiler-plugin minimum to 3.11.0 (ErrorProne classloading)
2. Detect and upgrade Maven 4 pre-release plugin versions (4.0.0-beta-1,
   4.0.0-alpha-*, etc.) that use removed API methods renamed before RC
3. Migrate org.scala-tools:maven-scala-plugin to
   net.alchim31.maven:scala-maven-plugin:4.9.5 (unmaintained since 2011)
4. Remove duplicate XML elements (DuplicateElementStrategy) that Maven 4's
   stricter POM parser rejects but Maven 3 silently accepted

The remaining 4 strategies already existed:
- scala-maven-plugin >= 4.9.5 (in PLUGIN_UPGRADES)
- maven-enforcer-plugin >= 3.0.0 (in PLUGIN_UPGRADES at 3.5.0)
- exec-maven-plugin >= 3.2.0 (in PLUGIN_UPGRADES at 3.5.0)
- Duplicate dependency declarations (DeduplicateDependenciesStrategy)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gnodet
gnodet marked this pull request as ready for review July 24, 2026 16:00
gnodet and others added 2 commits July 24, 2026 18:51
No RC versions exist for these plugins, so the RC exclusion was
dead code. Simplify isMaven4PreRelease() to catch any 4.0.0-*
qualifier.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously, plugins at 4.0.0-beta-1 were downgraded to the 3.x
minVersion floor. Now they are upgraded to the latest available
4.x pre-release instead:

- maven-compiler-plugin: 4.0.0-beta-4
- maven-resources-plugin: 4.0.0-beta-1 (already latest)
- maven-jar-plugin: 4.0.0-beta-1 (already latest)
- maven-install-plugin: 4.0.0-beta-2
- maven-deploy-plugin: 4.0.0-beta-2
- maven-clean-plugin: 4.0.0-beta-2

Adds latestPreRelease field to PluginUpgrade/PluginUpgradeInfo.
Deduplicates getPluginUpgradesMap() to derive from PLUGIN_UPGRADES.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gnodet
gnodet requested review from Bukama and cstamas July 24, 2026 21:11

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Well-implemented PR that adds four new mvnup upgrade strategies for Maven 4 compatibility. The pre-release detection logic is correct, the DuplicateElementStrategy handles list containers properly, and the test coverage is solid with 31 new tests. All 8 strategies from issue #12530 are accounted for (4 new, 4 already existing).

Three minor notes (non-blocking):

  • buildElementPath(Element parent, Element child) — the child parameter is never used; the method only walks up from parent. Either remove the parameter or append the child's name to the path for a more complete log message.
  • getPluginMigrationsMap() and getPluginUpgradesMap() rebuild a new HashMap on every call. With only one migration entry this is negligible, but could be hoisted to static fields for consistency.
  • The deduplication of getPluginUpgradesMap() (now derived from the PLUGIN_UPGRADES list via stream) is a good cleanup that eliminates a previous source of inconsistency.

LGTM.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

- Remove unused child parameter from buildElementPath()
- Cache pluginMigrationsMap as a static field

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gnodet

gnodet commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Addressed all three points:

  1. buildElementPath unused child parameter — Removed the unused parameter. Method now takes only the parent element. (80b938e)

  2. getPluginMigrationsMap() rebuilt on every call — Hoisted to a static final field initialized once from PLUGIN_MIGRATIONS. (80b938e)

  3. getPluginUpgradesMap() dedup — Already done in c88b27e (derives from PLUGIN_UPGRADES list via stream, eliminating the previous hand-maintained duplicate map).

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review (Re-review)

All three items from the previous review have been addressed in commit 80b938e:

  1. Unused child parameter in buildElementPath() — ✅ Removed.
  2. PLUGIN_MIGRATIONS_MAP rebuilt on every call — ✅ Hoisted to a static final field.
  3. getPluginUpgradesMap() deduplication — ✅ Already derived from PLUGIN_UPGRADES list, eliminating the hand-maintained duplicate map.

One remaining minor note (non-blocking):

  • getPluginUpgradesMap() still rebuilds a HashMap from PLUGIN_UPGRADES.stream() on every call (once per POM). Negligible with ~20 entries, but hoisting to a static final would be consistent with how PLUGIN_MIGRATIONS_MAP was handled.

The review-feedback commit is minimal and scoped precisely to the requested changes. LGTM.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant