Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -144,24 +144,49 @@ default Optional<Version> targetVersion() {

/**
* {@return an explicit target path, overriding the default value}
* <p>
* The returned path is typically <strong>relative to the output directory</strong>
* (e.g., {@code "custom-dir"} or {@code "META-INF/resources"}), but may be absolute
* if explicitly specified as such in the project configuration.
* </p>
* <p>
* When a target path is explicitly specified, the values of the {@link #module()} and {@link #targetVersion()}
* elements are not used for inferring the path (they are still used as compiler options however).
* It means that for scripts and resources, the files below the path specified by {@link #directory()}
* are copied to the path specified by {@code targetPath()} with the exact same directory structure.
* </p>
* <p>
* To obtain the fully resolved absolute path, use {@link #targetPath(Project)} instead.
* </p>
*
* @see #targetPath(Project)
*/
default Optional<Path> targetPath() {
return Optional.empty();
}

/**
* {@return the explicit target path resolved against the default target path}
* {@return the fully resolved absolute target path}
* <p>
* This method returns the absolute path where files from {@link #directory()} should be copied.
* Invoking this method is equivalent to getting the default output directory
* by a call to {@code project.getOutputDirectory(scope())}, then resolving the
* {@linkplain #targetPath() target path} (if present) against that default directory.
* Note that if the target path is absolute, the result is that target path unchanged.
* </p>
* <p>
* If {@link #targetPath()} returns:
* </p>
* <ul>
* <li>An empty {@code Optional}: returns the default output directory
* (e.g., {@code /path/to/project/target/classes})</li>
* <li>A relative path (e.g., {@code "custom-dir"}): returns the path resolved against
* the output directory (e.g., {@code /path/to/project/target/classes/custom-dir})</li>
* <li>An absolute path: returns that absolute path unchanged</li>
* </ul>
*
* @param project the project to use for getting default directories
*
* @see #targetPath()
* @see Project#getOutputDirectory(ProjectScope)
*/
@Nonnull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
* A Resource wrapper that maintains a connection to the underlying project model.
* When includes/excludes are modified, the changes are propagated back to the project's SourceRoots.
*/
@SuppressWarnings("deprecation")
class ConnectedResource extends Resource {
private final SourceRoot originalSourceRoot;
private final ProjectScope scope;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ void setUp() {
// Set a dummy pom file to establish the base directory
project.setFile(new java.io.File("./pom.xml"));

// Set build output directories
project.getBuild().setOutputDirectory("target/classes");
project.getBuild().setTestOutputDirectory("target/test-classes");

// Add a resource source root to the project
project.addSourceRoot(
new DefaultSourceRoot(ProjectScope.MAIN, Language.RESOURCES, Path.of("src/main/resources")));
Expand Down Expand Up @@ -199,7 +203,7 @@ void testTargetPathPreservedWithConnectedResource() {
resourceWithTarget.setDirectory("src/main/custom");
resourceWithTarget.setTargetPath("custom-output");

// Convert through DefaultSourceRoot to ensure targetPath extraction works
// Convert through DefaultSourceRoot to ensure targetPath is preserved
DefaultSourceRoot sourceRootFromResource =
new DefaultSourceRoot(project.getBaseDirectory(), ProjectScope.MAIN, resourceWithTarget.getDelegate());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,7 @@ public static DefaultSourceRoot fromModel(
source.getIncludes(),
source.getExcludes(),
source.isStringFiltering(),
nonBlank(source.getTargetPath())
.map((targetPath) ->
baseDir.resolve(outputDir.apply(scope)).resolve(targetPath))
.orElse(null),
nonBlank(source.getTargetPath()).map(Path::of).orElse(null),
source.isEnabled());
}

Expand All @@ -169,7 +166,7 @@ public DefaultSourceRoot(final Path baseDir, ProjectScope scope, Resource resour
resource.getIncludes(),
resource.getExcludes(),
Boolean.parseBoolean(resource.getFiltering()),
nonBlank(resource.getTargetPath()).map(baseDir::resolve).orElse(null),
nonBlank(resource.getTargetPath()).map(Path::of).orElse(null),
Copy link
Member

Choose a reason for hiding this comment

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

This is CWD relative?

Copy link
Contributor

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.ConnectedResource line 124.
  • org.apache.maven.project.DefaultProjectBuilder lines 704 and 707.
  • org.apache.maven.project.MavenProject line 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.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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 either project.build.outputDirectory or project.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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

true);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.eq;

@SuppressWarnings("deprecation")
@ExtendWith(MockitoExtension.class)
public class DefaultSourceRootTest {

Expand Down Expand Up @@ -148,7 +149,7 @@ void testModuleTestDirectory() {
}

/**
* Tests that relative target paths are resolved against the right base directory.
* Tests that relative target paths are stored as relative paths.
*/
@Test
void testRelativeMainTargetPath() {
Expand All @@ -160,13 +161,11 @@ void testRelativeMainTargetPath() {

assertEquals(ProjectScope.MAIN, source.scope());
assertEquals(Language.JAVA_FAMILY, source.language());
assertEquals(
Path.of("myproject", "target", "classes", "user-output"),
source.targetPath().orElseThrow());
assertEquals(Path.of("user-output"), source.targetPath().orElseThrow());
}

/**
* Tests that relative target paths are resolved against the right base directory.
* Tests that relative target paths are stored as relative paths.
*/
@Test
void testRelativeTestTargetPath() {
Expand All @@ -178,15 +177,14 @@ void testRelativeTestTargetPath() {

assertEquals(ProjectScope.TEST, source.scope());
assertEquals(Language.JAVA_FAMILY, source.language());
assertEquals(
Path.of("myproject", "target", "test-classes", "user-output"),
source.targetPath().orElseThrow());
assertEquals(Path.of("user-output"), source.targetPath().orElseThrow());
}

/*MNG-11062*/
@Test
void testExtractsTargetPathFromResource() {
// Test the Resource constructor that was broken in the regression
// Test the Resource constructor with relative targetPath
// targetPath should be kept as relative path
Resource resource = Resource.newBuilder()
.directory("src/test/resources")
.targetPath("test-output")
Expand All @@ -196,7 +194,7 @@ void testExtractsTargetPathFromResource() {

Optional<Path> targetPath = sourceRoot.targetPath();
assertTrue(targetPath.isPresent(), "targetPath should be present");
assertEquals(Path.of("myproject", "test-output"), targetPath.get());
assertEquals(Path.of("test-output"), targetPath.get(), "targetPath should be relative to output directory");
assertEquals(Path.of("myproject", "src", "test", "resources"), sourceRoot.directory());
assertEquals(ProjectScope.TEST, sourceRoot.scope());
assertEquals(Language.RESOURCES, sourceRoot.language());
Expand Down Expand Up @@ -244,7 +242,10 @@ void testHandlesPropertyPlaceholderInTargetPath() {

Optional<Path> targetPath = sourceRoot.targetPath();
assertTrue(targetPath.isPresent(), "Property placeholder targetPath should be present");
assertEquals(Path.of("myproject", "${project.build.directory}/custom"), targetPath.get());
assertEquals(
Path.of("${project.build.directory}/custom"),
targetPath.get(),
"Property placeholder should be kept as-is (relative path)");
}

/*MNG-11062*/
Expand Down Expand Up @@ -276,7 +277,9 @@ void testResourceConstructorPreservesOtherProperties() {

// Verify all properties are preserved
assertEquals(
Path.of("myproject", "test-classes"), sourceRoot.targetPath().orElseThrow());
Path.of("test-classes"),
sourceRoot.targetPath().orElseThrow(),
"targetPath should be relative to output directory");
assertTrue(sourceRoot.stringFiltering(), "Filtering should be true");
assertEquals(1, sourceRoot.includes().size());
assertTrue(sourceRoot.includes().contains("*.properties"));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.maven.it;

import java.io.File;

import org.junit.jupiter.api.Test;

/**
* This is a test set for <a href="https://github.com/apache/maven/issues/11381">GH-11381</a>.
*
* Verifies that relative targetPath in resources is resolved relative to the output directory
* (target/classes) and not relative to the project base directory, maintaining Maven 3.x behavior.
*
* @since 4.0.0-rc-4
*/
class MavenITgh11381ResourceTargetPathTest extends AbstractMavenIntegrationTestCase {

/**
* Verify that resources with relative targetPath are copied to target/classes/targetPath
* and not to the project root directory.
*
* @throws Exception in case of failure
*/
@Test
void testRelativeTargetPathInResources() throws Exception {
File testDir = extractResources("/gh-11381");

Verifier verifier = newVerifier(testDir.getAbsolutePath());
verifier.setAutoclean(false);
verifier.deleteDirectory("target");
verifier.addCliArgument("process-resources");
verifier.execute();
verifier.verifyErrorFreeLog();

// Verify that resources were copied to target/classes/target-dir (Maven 3.x behavior)
verifier.verifyFilePresent("target/classes/target-dir/test.yml");
verifier.verifyFilePresent("target/classes/target-dir/subdir/another.yml");

// Verify that resources were NOT copied to the project root target-dir directory
verifier.verifyFileNotPresent("target-dir/test.yml");
verifier.verifyFileNotPresent("target-dir/subdir/another.yml");
}
}

44 changes: 44 additions & 0 deletions its/core-it-suite/src/test/resources/gh-11381/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

https://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<groupId>org.apache.maven.its.gh11381</groupId>
<artifactId>test</artifactId>
<version>1.0-SNAPSHOT</version>
<packaging>jar</packaging>

<name>Maven Integration Test :: GH-11381</name>
<description>Test for relative targetPath in resources - should be relative to output directory</description>

<build>
<resources>
<resource>
<directory>${project.basedir}/rest</directory>
<targetPath>target-dir</targetPath>
<includes>
<include>**/*.yml</include>
</includes>
</resource>
</resources>
</build>
</project>

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Another test YAML file for GH-11381
another:
test: data

4 changes: 4 additions & 0 deletions its/core-it-suite/src/test/resources/gh-11381/rest/test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Test YAML file for GH-11381
test:
key: value