-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix resource targetPath resolution to be relative to output directory (fixes #11381) #11394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix resource targetPath resolution to be relative to output directory (fixes #11381) #11394
Conversation
… output directory This commit fixes a regression in Maven 4 where resource targetPath was being resolved relative to the project base directory instead of the output directory (target/classes or target/test-classes). The issue was in the DefaultSourceRoot constructor that takes a Resource parameter. It was resolving targetPath relative to baseDir instead of the output directory. Changes made: - Added a new constructor to DefaultSourceRoot that accepts an outputDir parameter - Deprecated the old constructor to maintain backward compatibility - Updated all call sites in DefaultProjectBuilder, ConnectedResource, and MavenProject to use the new constructor with the appropriate output directory - Updated unit tests in DefaultSourceRootTest to verify correct behavior - Added integration test MavenITgh11381ResourceTargetPathTest to verify the fix This restores Maven 3.x behavior where relative targetPath values in resources are correctly resolved relative to the output directory. Fixes apache#11381
…elative targetPath This commit fixes two issues in ConnectedResource: 1. Handle null output directories: When the Build object doesn't have output directories set (e.g., in tests), use default values (target/classes for main scope, target/test-classes for test scope). 2. Compute relative targetPath: The SourceRoot stores the targetPath as a resolved path (absolute or relative to baseDir), but the Resource model expects it to be relative to the output directory. Added a helper method computeRelativeTargetPath() to extract the relative path from the resolved path by relativizing it against the output directory. Also updated ResourceIncludeTest to: - Set build output directories in setUp() method - Use the new DefaultSourceRoot constructor with output directory parameter instead of the deprecated constructor This ensures that the targetPath is correctly preserved through the conversion chain from Resource -> SourceRoot -> ConnectedResource.
|
This does not look correct to me, I need to have another look. |
The root cause of the issue was that DefaultSourceRoot was resolving the targetPath against baseDir (or baseDir + outputDir), which required complex logic in ConnectedResource to extract the relative path back out. This commit simplifies the approach by keeping the targetPath as a relative path in DefaultSourceRoot (using Path::of instead of resolving it). This eliminates the need for: - The outputDir parameter in the DefaultSourceRoot constructor - The computeRelativeTargetPath() method in ConnectedResource - Complex path resolution and relativization logic Changes: - DefaultSourceRoot: Keep targetPath as relative path using Path::of - DefaultSourceRoot: Remove the outputDir parameter from constructor - DefaultSourceRoot: Remove the deprecated constructor (both constructors are now identical) - ConnectedResource: Simplify to just convert targetPath to string - ConnectedResource: Remove outputDir parameter from updateProjectSourceRoot - DefaultSourceRootTest: Update tests to expect relative paths - ResourceIncludeTest: Remove outputDir parameter from test This ensures that the targetPath is correctly preserved through the conversion chain from Resource -> SourceRoot -> ConnectedResource.
…rojectBuilder Since the targetPath is now kept as a relative path in DefaultSourceRoot, the outputDir parameter is no longer needed when creating DefaultSourceRoot instances from Resources. Changes: - MavenProject.addResource(): Remove outputDir calculation and parameter - DefaultProjectBuilder: Remove outputDir parameter from DefaultSourceRoot calls
| resource.getExcludes(), | ||
| Boolean.parseBoolean(resource.getFiltering()), | ||
| nonBlank(resource.getTargetPath()).map(baseDir::resolve).orElse(null), | ||
| nonBlank(resource.getTargetPath()).map(Path::of).orElse(null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is CWD relative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path.of(String) creates a path relative the to current working directory, which is not the desired output directory. I think that DefaultSourceRoot is okay, and the change rather needs to be applied in the codes that call this constructor. The problem seems to be in the following classes:
org.apache.maven.project.ConnectedResourceline 124.org.apache.maven.project.DefaultProjectBuilderlines 704 and 707.org.apache.maven.project.MavenProjectline 840
The above-cited codes set the first parameter to project.getBaseDirectory() while it should be project.getBuild().getOutputDirectory().
Note: the baseDir parameter name of DefaultSourceRoot is misleading. The Javadoc said "the base directory for resolving relative paths", which can be the baseDir of the Maven project but not necessarily. It should be understood as any base directory for resolving paths. This parameter could be renamed parentDir if baseDir is too misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is maybe resource.getTargetPath() absolute? So CWD does not matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be absolute, but I'm not sure that this guaranteed. It may depend on which code path as leaded to this constructor. I don't think that there is any reason to not be robust. We just need to find all calls to this constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe rename the baseDir parameter as outputDir (but without change in the call to Path.resolve(String)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, the expectation in Maven 3 is that
Resource.getTargetPath()will be resolved against eitherproject.build.outputDirectoryorproject.build.testOutputDirectory.
Yes I agree, but the replacement of baseDir::resolve by Path::of does not fix that. The correction is rather to fix the baseDir argument given in calls to the DefaultSourceRoot constructor. Actually, the first commit of this pull request seems correct to me, but it seems to have been reverted in subsequent commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think it does not ?
It makes the path truly relative, and later resolved against the correct directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the path truly relative, and later resolved against the correct directory.
Ah, I see. The responsibility of resolving the path is shifted to the SourceRoot user instead of the SourceRoot constructor. But why not resolving at construction time? It would probably be less code to check for correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think that would be desired behavior, but then we need to make it truly relative in the ConnectedResource to not break the existing plugin.
So the behavior would be different between mvn3 and mvn4 api, but that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the behavior would be different between mvn3 and mvn4 api
If the difference would be that mvn3 would be relative to target while mvn4 would be relative to baseDir, I think that this difference was a bug partially fixed in #11322, but we forgot the case of resources in that fix.
I will prepare an alternative pull request tomorrow for illustrating the fix that I propose. The actual pull request at the end may be, possibly, a mix of the two.
Based on PR comments, the solution has been updated:
1. SourceRoot.targetPath() returns a Path relative to the project's basedir
(e.g., 'target/classes/custom-output')
- Reverted DefaultSourceRoot to resolve targetPath against baseDir
2. ConnectedResource.getTargetPath() (Maven 3 API) returns a path relative to
the output directory (e.g., 'custom-output')
- Added computeRelativeTargetPath() method to make the path relative to
the output directory when converting from SourceRoot to Resource
This maintains backward compatibility with Maven 3 API while using the new
Maven 4 API internally.
Changes:
- DefaultSourceRoot: Reverted to resolve targetPath against baseDir
- ConnectedResource: Added computeRelativeTargetPath() to convert from
basedir-relative to outputdir-relative paths
- DefaultSourceRootTest: Updated tests to expect basedir-relative paths
- ResourceIncludeTest: Tests still pass with the new conversion logic
17d51e1 to
708e059
Compare
| /** | ||
| * Computes the targetPath relative to the output directory. | ||
| * In Maven 3 API, Resource.getTargetPath() is expected to be relative to the output directory | ||
| * (e.g., "custom-output"), while SourceRoot.targetPath() is relative to the project basedir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while SourceRoot.targetPath() is relative to the project basedir
Actually it was a bug. The specification that we wrote in maven.mdo said that targetPath shall be relative to the target directory. Fixing that bug was the subject of #11322, but that correction was incomplete. We could said that this pull request #11394 completes #11322.
…directory This commit fixes the regression where resources with a relative targetPath were being copied to the project root instead of relative to the output directory (target/classes or target/test-classes). Changes: 1. DefaultSourceRoot.fromModel: Store targetPath as a relative path instead of resolving it against baseDir and outputDir. This ensures that SourceRoot.targetPath() returns a relative path as intended by the Maven 4 API javadoc. 2. ConnectedResource.computeRelativeTargetPath: Simplified to directly return the relative targetPath from SourceRoot, since it's now always stored as relative. 3. Updated tests to expect relative paths from SourceRoot.targetPath(). Maven 4 API Conformance: - SourceRoot.targetPath() returns an Optional<Path> containing the explicit target path, which should be relative to the output directory (or absolute if explicitly specified as absolute). - SourceRoot.targetPath(Project) resolves this relative path against the project's output directory to produce an absolute path. Maven 3 Compatibility: - Resource.getTargetPath() in Maven 3 was always relative to the output directory. This behavior is preserved by storing targetPath as relative in SourceRoot and converting it back to relative for the Resource API via ConnectedResource. Example: With <targetPath>custom-dir</targetPath>: - Maven 3: Resources copied to target/classes/custom-dir - Maven 4 (before fix): Resources copied to project-root/custom-dir - Maven 4 (after fix): Resources copied to target/classes/custom-dir Fixes apache#11381
Implementation DetailsI've implemented a fix for this issue that ensures Root CauseThe regression occurred because Maven 4 API DesignThe Maven 4 API defines two methods for
This two-method design clearly indicates that ImplementationFixed // Before (incorrect):
nonBlank(source.getTargetPath())
.map((targetPath) -> baseDir.resolve(outputDir.apply(scope)).resolve(targetPath))
.orElse(null)
// After (correct):
nonBlank(source.getTargetPath()).map(Path::of).orElse(null)Now Maven 3 CompatibilityIn Maven 3,
ExampleWith
TestingAll existing tests pass, including:
The fix is minimal, focused, and aligns with the Maven 4 API design while maintaining backward compatibility. |
Enhanced the javadoc for SourceRoot.targetPath() and targetPath(Project) to make it explicit that: 1. targetPath() returns a path that is typically relative to the output directory (e.g., 'custom-dir' or 'META-INF/resources'), but may be absolute if explicitly specified. 2. targetPath(Project) returns the fully resolved absolute path where files should be copied, with clear examples of how it handles: - Empty Optional: returns the default output directory - Relative path: resolves against the output directory - Absolute path: returns unchanged These clarifications help implementers understand the intended behavior and prevent regressions like apacheGH-11381 where targetPath was incorrectly stored as an absolute path.
…ctory()
This commit improves the documentation for resource targetPath handling to clarify
the Maven 4 API semantics while preserving Maven 3 backward compatibility.
Key documentation improvements:
1. SourceRoot.targetPath():
- Clarifies that relative paths are resolved relative to the output directory
(target/classes for MAIN scope, target/test-classes for TEST scope)
- Documents that absolute paths are used as-is
- Explicitly states Maven 3 compatibility is maintained
- Adds concrete examples showing path resolution
2. SourceRoot.targetPath(Project):
- Documents the complete resolution algorithm step-by-step
- Provides detailed examples with actual paths
- Cross-references Project.getOutputDirectory()
3. Project.getOutputDirectory():
- Clarifies that it returns the directory for both compiled classes AND resources
- Documents its role in SourceRoot targetPath resolution
- Explicitly states Maven 3 compatibility semantics
4. DefaultSourceRoot constructor from Resource:
- Documents that targetPath is stored as-is (not resolved against basedir)
- Clarifies that baseDir parameter is only used for source directory resolution
- Explains how this preserves Maven 3.x behavior
Implementation details:
- DefaultSourceRoot stores targetPath as provided (relative or absolute)
- ConnectedResource extracts targetPath as-is for maven-resources-plugin
- Resolution to absolute paths happens via SourceRoot.targetPath(Project)
- This maintains Maven 3 behavior where resource targetPath is relative to
the output directory, not the project base directory
Related to apache#11381
Latest Update: Enhanced Javadoc for API ClarityI've just pushed an update that significantly improves the documentation to clarify the Maven 4 API semantics while making the Maven 3 backward compatibility guarantees explicit. What ChangedEnhanced javadoc for three key methods:
Why This MattersThe enhanced documentation makes it crystal clear that:
Implementation VerificationThe documentation now matches the actual implementation: ✅ ✅ ✅ maven-resources-plugin receives the relative path and resolves it (Maven 3 behavior) ✅ Maven 4 API consumers can use See the detailed explanation in this comment on #11381 for the complete design rationale. |
This commit significantly enhances the javadoc documentation to make the
targetPath handling semantics even more explicit and clear for all audiences:
API consumers, plugin developers, and implementers.
Key improvements:
1. SourceRoot.targetPath():
- Explicitly states this returns the path AS CONFIGURED (no resolution)
- Clearly distinguishes between storage (this method) and resolution (targetPath(Project))
- Provides detailed "Return Value Semantics" section explaining empty/relative/absolute cases
- Adds "Usage Guidance" section for different audiences:
* Maven 4 API consumers: use targetPath(Project)
* Maven 3 compatibility: use this method for legacy plugins
* Implementers: store path as-is, don't resolve at storage time
- Emphasizes Maven 3 compatibility with explicit reference to
project.build.outputDirectory and project.build.testOutputDirectory
2. SourceRoot.targetPath(Project):
- Adds "Purpose" section explaining this is THE resolution method
- Provides detailed step-by-step resolution algorithm with all cases
- Includes comprehensive table with concrete examples showing:
* Configuration input
* Output directory
* Final resolved result
* Explanation of each case
- Adds "Relationship to targetPath()" section explaining storage vs resolution
- Includes implementation note with equivalent code for clarity
3. Project.getOutputDirectory():
- Adds comprehensive table showing scope-to-directory mapping
- Includes "Role in SourceRoot Path Resolution" section with concrete examples
- Expands Maven 3 compatibility section with actual XML configuration example
- Shows how maven-resources-plugin uses this in Maven 3
- Provides code examples demonstrating usage
4. HTML5 Compliance:
- Replaced deprecated table attributes (cellpadding, cellspacing, border)
- Uses modern HTML5 table styling with class="striped"
The refined documentation makes it crystal clear that:
- targetPath() is for STORAGE (returns path as configured)
- targetPath(Project) is for RESOLUTION (returns absolute path)
- getOutputDirectory() provides the base for resolution
- Maven 3 compatibility is maintained by storing relative paths
- Different audiences have clear guidance on which method to use
Related to apache#11381
Further Javadoc Refinements for Maximum ClarityI've pushed another update that significantly refines the Maven 4 API javadoc to make it even more explicit and clear. The documentation now leaves no ambiguity about how Key Enhancements1. SourceRoot.targetPath() - The Storage MethodNew sections added:
Why this matters: Makes it crystal clear that this method is for STORAGE, not RESOLUTION. 2. SourceRoot.targetPath(Project) - The Resolution MethodNew sections added:
Example from the table:
Why this matters: Developers can see exactly how each case is handled with concrete examples. 3. Project.getOutputDirectory() - The Base Directory ProviderNew sections added:
Why this matters: Makes the connection between this method and HTML5 ComplianceFixed javadoc generation errors by replacing deprecated HTML attributes:
Javadoc now builds cleanly without errors. Documentation PhilosophyThe refined documentation follows a clear pattern:
Verification✅ Javadoc builds successfully without errors ✅ All three methods have coherent, cross-referenced documentation ✅ Maven 3 compatibility guarantees are explicit ✅ Maven 4 API semantics are clear The documentation now provides complete clarity on the targetPath handling design, making it easy for developers to understand and use the API correctly. |
Commit Description for MergeHere's a comprehensive commit message explaining the chosen fix: Maven 3 behavior: Files copied to target/classes/target-dir ✓ Root CauseThe DefaultSourceRoot constructor that creates SourceRoot instances from Resource Solution DesignThe fix implements a clear separation of concerns between storage and resolution: 1. Storage Layer (SourceRoot.targetPath())This method returns the targetPath exactly as specified in the configuration:
No resolution happens at this layer. The path is stored as-is. 2. Resolution Layer (SourceRoot.targetPath(Project))This method performs the actual path resolution: Algorithm:
This matches Maven 3 behavior exactly. 3. Implementation ChangesDefaultSourceRoot.java:
ConnectedResource.java:
Project.getOutputDirectory():
4. Maven 4 API DocumentationExtensive javadoc enhancements make the design explicit: SourceRoot.targetPath():
SourceRoot.targetPath(Project):
Project.getOutputDirectory():
Why This ApproachThis design was chosen because:
TestingIntegration test MavenITgh11381ResourceTargetPathTest verifies:
ImpactThis fix: Fixes #11381 |
…fixes apache#11381) (apache#11394) This commit fixes the regression where resources with a relative targetPath were being copied to the project root instead of relative to the output directory (target/classes or target/test-classes). Changes: 1. DefaultSourceRoot.fromModel: Store targetPath as a relative path instead of resolving it against baseDir and outputDir. This ensures that SourceRoot.targetPath() returns a relative path as intended by the Maven 4 API javadoc. 2. ConnectedResource.computeRelativeTargetPath: Simplified to directly return the relative targetPath from SourceRoot, since it's now always stored as relative. 3. Updated tests to expect relative paths from SourceRoot.targetPath(). Maven 4 API Conformance: - SourceRoot.targetPath() returns an Optional<Path> containing the explicit target path, which should be relative to the output directory (or absolute if explicitly specified as absolute). - SourceRoot.targetPath(Project) resolves this relative path against the project's output directory to produce an absolute path. Maven 3 Compatibility: - Resource.getTargetPath() in Maven 3 was always relative to the output directory. This behavior is preserved by storing targetPath as relative in SourceRoot and converting it back to relative for the Resource API via ConnectedResource. Example: With <targetPath>custom-dir</targetPath>: - Maven 3: Resources copied to target/classes/custom-dir - Maven 4 (before fix): Resources copied to project-root/custom-dir - Maven 4 (after fix): Resources copied to target/classes/custom-dir Fixes apache#11381 (cherry picked from commit 9b95526)
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…fixes apache#11381) (apache#11394) This commit fixes the regression where resources with a relative targetPath were being copied to the project root instead of relative to the output directory (target/classes or target/test-classes). Changes: 1. DefaultSourceRoot.fromModel: Store targetPath as a relative path instead of resolving it against baseDir and outputDir. This ensures that SourceRoot.targetPath() returns a relative path as intended by the Maven 4 API javadoc. 2. ConnectedResource.computeRelativeTargetPath: Simplified to directly return the relative targetPath from SourceRoot, since it's now always stored as relative. 3. Updated tests to expect relative paths from SourceRoot.targetPath(). Maven 4 API Conformance: - SourceRoot.targetPath() returns an Optional<Path> containing the explicit target path, which should be relative to the output directory (or absolute if explicitly specified as absolute). - SourceRoot.targetPath(Project) resolves this relative path against the project's output directory to produce an absolute path. Maven 3 Compatibility: - Resource.getTargetPath() in Maven 3 was always relative to the output directory. This behavior is preserved by storing targetPath as relative in SourceRoot and converting it back to relative for the Resource API via ConnectedResource. Example: With <targetPath>custom-dir</targetPath>: - Maven 3: Resources copied to target/classes/custom-dir - Maven 4 (before fix): Resources copied to project-root/custom-dir - Maven 4 (after fix): Resources copied to target/classes/custom-dir Fixes apache#11381 (cherry picked from commit 9b95526)
…fixes #11381) (#11394) (#11406) This commit fixes the regression where resources with a relative targetPath were being copied to the project root instead of relative to the output directory (target/classes or target/test-classes). Changes: 1. DefaultSourceRoot.fromModel: Store targetPath as a relative path instead of resolving it against baseDir and outputDir. This ensures that SourceRoot.targetPath() returns a relative path as intended by the Maven 4 API javadoc. 2. ConnectedResource.computeRelativeTargetPath: Simplified to directly return the relative targetPath from SourceRoot, since it's now always stored as relative. 3. Updated tests to expect relative paths from SourceRoot.targetPath(). Maven 4 API Conformance: - SourceRoot.targetPath() returns an Optional<Path> containing the explicit target path, which should be relative to the output directory (or absolute if explicitly specified as absolute). - SourceRoot.targetPath(Project) resolves this relative path against the project's output directory to produce an absolute path. Maven 3 Compatibility: - Resource.getTargetPath() in Maven 3 was always relative to the output directory. This behavior is preserved by storing targetPath as relative in SourceRoot and converting it back to relative for the Resource API via ConnectedResource. Example: With <targetPath>custom-dir</targetPath>: - Maven 3: Resources copied to target/classes/custom-dir - Maven 4 (before fix): Resources copied to project-root/custom-dir - Maven 4 (after fix): Resources copied to target/classes/custom-dir Fixes #11381 (cherry picked from commit 9b95526)
This PR fixes a regression in Maven 4 where resource
targetPathwas being resolved relative to the project base directory instead of the output directory (target/classesortarget/test-classes).Problem
In Maven 3.x, when a resource has a relative
targetPath, it's resolved relative to the output directory. For example:In Maven 3.x, files would be copied to
target/classes/target-dir/, but in Maven 4, they were being copied totarget-dir/(relative to project root).Root Cause
The bug was in the
DefaultSourceRootconstructor that takes aResourceparameter. On line 172, the targetPath was being resolved as:This resolves the targetPath relative to the base directory, not the output directory.
Solution
DefaultSourceRoot.javathat accepts anoutputDirparameter and correctly resolves targetPath relative to the output directoryDefaultProjectBuilder.javaConnectedResource.javaMavenProject.javaDefaultSourceRootTest.javato verify the correct behaviorMavenITgh11381ResourceTargetPathTest.javato verify the fix works end-to-endTesting
The integration test verifies that:
targetPathare copied to the correct location under the output directoryTest results:
Fixes #11381
Pull Request opened by Augment Code with guidance from the PR author