[Backport 4.0.x] Add mvnup SourceStrategy for migrating to <source> elements#12357
[Backport 4.0.x] Add mvnup SourceStrategy for migrating to <source> elements#12357gnodet wants to merge 4 commits into
Conversation
Adds a new mvnup upgrade strategy that migrates legacy source configuration to Maven 4.1.0+ <source> elements. Handles four migration phases: - Compiler properties (maven.compiler.release, source/target) to <source><targetVersion> - Compiler plugin configuration (<release>, <source>/<target>) to <source><targetVersion> - Custom source/test directories to <source><directory> - Resource/testResource sections to <source> with lang=resources Applies when --model-version is 4.1.0+ or --all is set. Runs at @priority(20), after all other strategies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Check groupId in findCompilerPlugin (not just artifactId) - Clean up compiler plugin config when properties already provide targetVersion (previously left stale release/source/target elements) - Clean up empty pluginManagement after plugin removal - Clean up empty build element after all children removed - Add test for pluginManagement compiler plugin migration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| } | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
It is a small detail, but the lambda below seems almost identical to the lambda above. Maybe it could be written only once in a separated function.
There was a problem hiding this comment.
Fully automatic review from Claude Code
Done — extracted a copyPatternList(source, target, containerName, elementName) static method that both includes and excludes now delegate to.
| return true; | ||
| } | ||
|
|
||
| private void copyIncludesExcludes(Element source, Element target) { |
There was a problem hiding this comment.
Maybe the method could be static (I'm not sure, I didn't opened the file in an IDE for making sure that I didn't missed a field).
There was a problem hiding this comment.
Fully automatic review from Claude Code
Good catch — copyIncludesExcludes (and the new copyPatternList) are now both static.
desruisseaux
left a comment
desruisseaux
left a comment
There was a problem hiding this comment.
I did not verified in details, but it seems nice by a quick look and the purpose is nice too.
Address review feedback from desruisseaux: extract shared logic from copyIncludesExcludes into a static copyPatternList method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Backports a new mvnup upgrade strategy to migrate legacy compiler/source/resource configuration into Maven 4.1.0+ <build><sources><source> model, gated by --model-version 4.1.0+ or --all.
Changes:
- Adds
SourceStrategy(@Priority(20)) to extract target Java version from compiler properties / maven-compiler-plugin config and migrate source/test directories and (test)resources into<source>elements. - Cleans up migrated legacy elements/containers (properties, plugin config, sourceDirectory/testSourceDirectory, resources/testResources) and removes empty
<build>when applicable. - Adds a comprehensive JUnit test suite covering applicability and all migration phases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java |
Implements the new migration strategy that writes Maven 4.1+ <source> entries and removes legacy configuration. |
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java |
Adds unit tests validating applicability and each migration phase (compiler props/plugin, directories, resources, version filtering, combined scenarios). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (targetVersion != null) { | ||
| Element sourcesElement = ensureSourcesElement(root); | ||
| Element sourceElement = DomUtils.insertNewElement("source", sourcesElement); | ||
| DomUtils.insertContentElement(sourceElement, "targetVersion", targetVersion); | ||
| context.detail("Set targetVersion: " + targetVersion); | ||
| hasChanges = true; | ||
| } |
| String targetVersion = extractTargetVersionFromProperties(context, root); | ||
|
|
||
| if (targetVersion == null) { | ||
| targetVersion = extractTargetVersionFromCompilerPlugin(context, root); | ||
| } else { | ||
| cleanupCompilerPluginConfig(context, root); | ||
| } |
Summary
Backport of #12355 to
maven-4.0.x.SourceStrategyto mvnup that migrates legacy source configuration to Maven 4.1.0+<source>elementsmaven.compiler.release,maven.compiler.source/target), compiler plugin configuration, source/test directories, and resources--model-version 4.1.0+or--allis specifiedTest plan
maven-4.0.x🤖 Generated with Claude Code