Copilot commented on code in PR #27:
URL:
https://github.com/apache/maven-compiler-plugin/pull/27#discussion_r2680374500
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
Review Comment:
The comment is incomplete - it ends abruptly with "Patch module to add"
without finishing the sentence. This should be completed to clearly describe
what is being added to the patch module.
```suggestion
// Patch module to add main output directory, source roots,
and modularized test dependencies
```
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
+
+ compilerArgs.add(patchModuleValue.toString());
+ }
+
compilerArgs.add("--add-reads");
compilerArgs.add(mainModuleDescriptor.name() + "=ALL-UNNAMED");
} else {
modulepathElements = Collections.emptyList();
classpathElements = testPath;
}
+
+ }
+ }
+
+ /**
+ * Compiling with test classpath is not sufficient because some
dependencies <br/>
+ * may be modularized and their test class path addition would be
ignored.<p/>
+ * Example from MCOMPILER-372_modularized_test:<br/>
+ * prj2 test classes depend on prj1 classes and test classes<br/>
+ * As prj1.jar is added as a module, pjr1-tests.jar classes are ignored if
we add jar class path<br/>
+ * We have to add pjr1-tests.jar as --patch-module<p/>
+ * @param mainModuleName
+ * param mainModuleTarget
+ * @param patchModules map of module names -> list of paths to add to
--patch-module option
+ *
+ */
+ protected void patchModulesForTestDependencies(
+ String mainModuleName, String mainModuleTarget,
+ LinkedHashMap<String, List<String>> patchModules
+ ) {
+ File mainModuleDescriptorClassFile = new File(mainModuleTarget,
"module-info.class");
+
+ ResolvePathsResult<File> result;
+
+ try {
+ // Get compile path information to identify modularized compile
path elements
+ Collection<File> dependencyArtifacts =
getCompileClasspathElements(getProject());
+
+ ResolvePathsRequest<File> request =
+ ResolvePathsRequest.ofFiles(dependencyArtifacts)
+ .setMainModuleDescriptor(mainModuleDescriptorClassFile);
+
+ Toolchain toolchain = getToolchain();
+ if (toolchain instanceof DefaultJavaToolChain) {
+ request.setJdkHome(new File(((DefaultJavaToolChain)
toolchain).getJavaHome()));
+ }
+
+ result = locationManager.resolvePaths(request);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
+ // Prepare a path list containing test paths for modularized paths
+ // This path list will augment modules to be able to compile
+ List<String> listModuleTestPaths = new ArrayList<>(testPath.size());
+
+ // Browse modules
+ for (Entry<File, ModuleNameSource> pathElt :
result.getModulepathElements().entrySet()) {
+
+ File modulePathElt = pathElt.getKey();
+
+ // Get module name
+ JavaModuleDescriptor moduleDesc = result.getPathElements().get(
modulePathElt );
+ String moduleName = (moduleDesc != null) ? moduleDesc.name() :
null;
+
+ if (
+ // Is it a modularized compile elements?
+ ( modulePathElt != null ) && ( moduleName != null )
+ &&
+ // Is it different from main module name
+ !mainModuleName.equals( moduleName )
+ ) {
+
+ // Get test path element
+ File moduleTestPathElt = getModuleTestPathElt(modulePathElt);
+
+ if (moduleTestPathElt != null) {
+
listModuleTestPaths.add(moduleTestPathElt.getAbsolutePath());
+ }
+ }
+ }
+
+ // Remove main target path
+ listModuleTestPaths.remove(mainModuleTarget);
+ listModuleTestPaths.remove(outputDirectory.getAbsolutePath());
+
+ // Freeze list
+ listModuleTestPaths =
Collections.unmodifiableList(listModuleTestPaths);
+
+ if (getLog().isDebugEnabled()) {
+ getLog().debug("patchModule test paths:");
+ for (String moduleTestPath : listModuleTestPaths) {
+ getLog().debug(" " + moduleTestPath);
+ }
+
+ }
+
+ // Get modularized dependencies resolved before
+ for (Entry<File, ModuleNameSource> pathElt :
result.getModulepathElements().entrySet()) {
+ File path = pathElt.getKey();
+
+ // Get module name
+ JavaModuleDescriptor moduleDesc =
result.getPathElements().get(path);
+ String moduleName = (moduleDesc != null) ? moduleDesc.name() :
null;
+
+ if (
+ // Is it a modularized compile elements?
+ ( path != null ) && ( moduleName != null )
+ &&
+ // Not an auto-module
+ !moduleDesc.isAutomatic()
+ &&
+ // Is it different from main module name
+ !mainModuleName.equals( moduleName )
+ ) {
+
+ // Add --add-reads <moduleName>=ALL-UNNAMED
+ compilerArgs.add( "--add-reads" );
+ StringBuilder sbAddReads = new StringBuilder();
+ sbAddReads.append(moduleName).append("=ALL-UNNAMED");
+ compilerArgs.add(sbAddReads.toString());
+
+ // Add compile classpath if needed
+ if (!listModuleTestPaths.isEmpty()) {
+ // Yes, add it as patch module
+ List<String> listPath = patchModules.get(moduleName);
+ // Make sure it is initialized
+ if (listPath == null) {
+ listPath = new ArrayList<>();
+ patchModules.put(moduleName, listPath);
+ }
+
+ // Add test compile path but not main module target
+ listPath.addAll(listModuleTestPaths);
+ }
+ }
+ }
+
+ if (pathElements == null) {
+ pathElements = new
LinkedHashMap<>(result.getPathElements().size());
+ for (Entry<File, JavaModuleDescriptor> entry :
result.getPathElements().entrySet()) {
+ pathElements.put(entry.getKey().getAbsolutePath(),
entry.getValue());
+ }
}
}
+ /**
+ * Get module test path element from module path element
+ * @param modulePathElt
+ * @return
Review Comment:
The javadoc is missing a description for the @param modulePathElt parameter
and the @return value. These should be documented to explain what the parameter
represents and what is returned.
```suggestion
* Get module test path element from module path element.
*
* @param modulePathElt the module path element, typically the main
output directory of a module
* @return the corresponding test output directory for the given module
path element,
* or {@code null} if no matching test path can be resolved or
it does not exist
```
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
+
+ compilerArgs.add(patchModuleValue.toString());
+ }
+
compilerArgs.add("--add-reads");
compilerArgs.add(mainModuleDescriptor.name() + "=ALL-UNNAMED");
} else {
modulepathElements = Collections.emptyList();
classpathElements = testPath;
}
+
+ }
+ }
+
+ /**
+ * Compiling with test classpath is not sufficient because some
dependencies <br/>
+ * may be modularized and their test class path addition would be
ignored.<p/>
+ * Example from MCOMPILER-372_modularized_test:<br/>
+ * prj2 test classes depend on prj1 classes and test classes<br/>
+ * As prj1.jar is added as a module, pjr1-tests.jar classes are ignored if
we add jar class path<br/>
+ * We have to add pjr1-tests.jar as --patch-module<p/>
+ * @param mainModuleName
+ * param mainModuleTarget
Review Comment:
The @param documentation is malformed. The second parameter
"mainModuleTarget" is missing the "@" symbol and appears as "param
mainModuleTarget" instead of "@param mainModuleTarget". This will cause the
javadoc to be incorrectly formatted.
```suggestion
* @param mainModuleTarget
```
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
+
+ compilerArgs.add(patchModuleValue.toString());
+ }
+
compilerArgs.add("--add-reads");
compilerArgs.add(mainModuleDescriptor.name() + "=ALL-UNNAMED");
} else {
modulepathElements = Collections.emptyList();
classpathElements = testPath;
}
+
+ }
+ }
+
+ /**
+ * Compiling with test classpath is not sufficient because some
dependencies <br/>
+ * may be modularized and their test class path addition would be
ignored.<p/>
+ * Example from MCOMPILER-372_modularized_test:<br/>
+ * prj2 test classes depend on prj1 classes and test classes<br/>
+ * As prj1.jar is added as a module, pjr1-tests.jar classes are ignored if
we add jar class path<br/>
+ * We have to add pjr1-tests.jar as --patch-module<p/>
+ * @param mainModuleName
+ * param mainModuleTarget
+ * @param patchModules map of module names -> list of paths to add to
--patch-module option
+ *
+ */
+ protected void patchModulesForTestDependencies(
+ String mainModuleName, String mainModuleTarget,
+ LinkedHashMap<String, List<String>> patchModules
+ ) {
+ File mainModuleDescriptorClassFile = new File(mainModuleTarget,
"module-info.class");
+
+ ResolvePathsResult<File> result;
+
+ try {
+ // Get compile path information to identify modularized compile
path elements
+ Collection<File> dependencyArtifacts =
getCompileClasspathElements(getProject());
+
+ ResolvePathsRequest<File> request =
+ ResolvePathsRequest.ofFiles(dependencyArtifacts)
+ .setMainModuleDescriptor(mainModuleDescriptorClassFile);
+
+ Toolchain toolchain = getToolchain();
+ if (toolchain instanceof DefaultJavaToolChain) {
+ request.setJdkHome(new File(((DefaultJavaToolChain)
toolchain).getJavaHome()));
+ }
+
+ result = locationManager.resolvePaths(request);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
IOException is being caught and wrapped in a RuntimeException, which loses
the exception type information and converts a checked exception to an unchecked
one. This could make it harder for callers to handle specific error scenarios.
Consider wrapping it in a more specific exception type or propagating the
IOException with better context about what failed.
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
+
+ compilerArgs.add(patchModuleValue.toString());
+ }
+
compilerArgs.add("--add-reads");
compilerArgs.add(mainModuleDescriptor.name() + "=ALL-UNNAMED");
} else {
modulepathElements = Collections.emptyList();
classpathElements = testPath;
}
+
+ }
+ }
+
+ /**
+ * Compiling with test classpath is not sufficient because some
dependencies <br/>
+ * may be modularized and their test class path addition would be
ignored.<p/>
+ * Example from MCOMPILER-372_modularized_test:<br/>
+ * prj2 test classes depend on prj1 classes and test classes<br/>
+ * As prj1.jar is added as a module, pjr1-tests.jar classes are ignored if
we add jar class path<br/>
+ * We have to add pjr1-tests.jar as --patch-module<p/>
Review Comment:
There's a typo in the javadoc comment: "pjr1-tests.jar" should be
"prj1-tests.jar" to match the project naming convention used throughout the
codebase and examples.
```suggestion
* As prj1.jar is added as a module, prj1-tests.jar classes are ignored
if we add jar class path<br/>
* We have to add prj1-tests.jar as --patch-module<p/>
```
##########
src/it/MCOMPILER-372_modularized_testjar/prj0/pom.xml:
##########
@@ -0,0 +1,132 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <description>This project tests a JAXB XSD to java test compilation
</description>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>prj0-372</artifactId>
+
+ <build>
+
+ <plugins>
+
+ <!-- Needed to install test jar to repository -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <version>3.2.0</version>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+
+ <!-- generate JAXB stubs for schemas -->
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>jaxb2-maven-plugin</artifactId>
+ <version>2.5.0</version>
+ <executions>
+
+ <!-- Dummy configuration XSD -->
+ <execution>
+ <id>config</id>
+ <goals>
+ <goal>xjc</goal>
+ </goals>
+ <configuration>
+ <!-- XSD schema -->
+ <sources>
+ <source>src/main/resources/prj0/Prj0Conf.xsd</source>
+ </sources>
+ <packageName>prj0.conf</packageName>
+
<outputDirectory>${project.build.directory}/generated-sources/xjcConf</outputDirectory>
+ </configuration>
Review Comment:
The indentation uses tabs instead of spaces, which is inconsistent with the
XML file's general indentation style. This should use spaces to match the
surrounding code.
```suggestion
<source>src/main/resources/prj0/Prj0Conf.xsd</source>
</sources>
<packageName>prj0.conf</packageName>
<outputDirectory>${project.build.directory}/generated-sources/xjcConf</outputDirectory>
</configuration>
```
##########
src/it/MCOMPILER-372_modularized_testjar/prj2/pom.xml:
##########
@@ -0,0 +1,87 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>prj2-372</artifactId>
+
+ <build>
+
+ <plugins>
+
+ <!-- Needed to install test jar to repository -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <version>3.2.0</version>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+
+ </plugins>
+
+ </build>
+
+ <dependencies>
+
+ <dependency>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>prj1-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </dependency>
+
+ <!--
+ Prj 2 depends on Prj 1 test jar
+ Prj 1 is modularized.
+ This is configuration we want to test.
+ -->
+ <dependency>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>prj1-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <classifier>tests</classifier>
+ <scope>test</scope>
+ </dependency>
+
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>3.8.2</version>
+ <scope>test</scope>
Review Comment:
The indentation uses tabs instead of spaces, which is inconsistent with the
XML file's general indentation style. This dependency declaration should use
spaces to match the surrounding code.
##########
src/it/MCOMPILER-372_modularized_testjar/pom.xml:
##########
@@ -0,0 +1,56 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <packaging>pom</packaging>
+
+ <properties>
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ </properties>
+
+ <modules>
+ <module>prj0</module>
+ <module>prj1</module>
+ <module>prj2</module>
+ <module>prj3</module>
+ </modules>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <version>@project.version@</version>
+ <configuration>
+ <release>11</release>
+ <testRelease>11</testRelease>
Review Comment:
The indentation uses a tab character instead of spaces, which is
inconsistent with the rest of the XML file that uses spaces for indentation.
This should use spaces to match the surrounding code style.
```suggestion
<testRelease>11</testRelease>
```
##########
src/it/MCOMPILER-372_modularized_testjar/prj0/pom.xml:
##########
@@ -0,0 +1,132 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <description>This project tests a JAXB XSD to java test compilation
</description>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>prj0-372</artifactId>
+
+ <build>
+
+ <plugins>
+
+ <!-- Needed to install test jar to repository -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <version>3.2.0</version>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+
+ <!-- generate JAXB stubs for schemas -->
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>jaxb2-maven-plugin</artifactId>
+ <version>2.5.0</version>
+ <executions>
+
+ <!-- Dummy configuration XSD -->
+ <execution>
+ <id>config</id>
+ <goals>
+ <goal>xjc</goal>
+ </goals>
+ <configuration>
+ <!-- XSD schema -->
+ <sources>
+ <source>src/main/resources/prj0/Prj0Conf.xsd</source>
+ </sources>
+ <packageName>prj0.conf</packageName>
+
<outputDirectory>${project.build.directory}/generated-sources/xjcConf</outputDirectory>
+ </configuration>
+ </execution>
+
+ </executions>
+ </plugin>
+
+ </plugins>
+
+ </build>
+
+ <dependencies>
+
+ <!-- log4j -->
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>2.12.1</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <version>2.12.1</version>
+ </dependency>
+
+ <!-- For XML binding, removed from java 11 -->
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ <version>2.3.1</version>
+ </dependency>
+ <dependency>
+ <groupId>com.sun.xml.bind</groupId>
+ <artifactId>jaxb-core</artifactId>
+ <version>2.3.0.1</version>
+ </dependency>
+ <dependency>
+ <groupId>com.sun.xml.bind</groupId>
+ <artifactId>jaxb-impl</artifactId>
+ <version>2.3.1</version>
+ </dependency>
+
+ <dependency>
+ <groupId>javax.activation</groupId>
+ <artifactId>activation</artifactId>
+ <version>1.1.1</version>
+ </dependency>
+
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>3.8.2</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
The indentation uses tabs instead of spaces, which is inconsistent with the
XML file's general indentation style. These dependency declarations should use
spaces to match the surrounding code.
```suggestion
<dependency>
<groupId>javax.xml.bind</groupId>
<artifactId>jaxb-api</artifactId>
<version>2.3.1</version>
</dependency>
<dependency>
<groupId>com.sun.xml.bind</groupId>
<artifactId>jaxb-core</artifactId>
<version>2.3.0.1</version>
</dependency>
<dependency>
<groupId>com.sun.xml.bind</groupId>
<artifactId>jaxb-impl</artifactId>
<version>2.3.1</version>
</dependency>
<dependency>
<groupId>javax.activation</groupId>
<artifactId>activation</artifactId>
<version>1.1.1</version>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>3.8.2</version>
<scope>test</scope>
</dependency>
```
##########
src/it/MCOMPILER-372_modularized_testjar/prj3/pom.xml:
##########
@@ -0,0 +1,74 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>prj3-372</artifactId>
+
+ <dependencies>
+
+ <dependency>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>prj2-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </dependency>
+
+ <!--
+ Prj 3 depends on Prj 1 test jar
+ Prj 2 depends on Prj 1 test jar
+ Prj 1, 2, 3 are modularized.
+ This is configuration we want to test.
+ -->
+ <dependency>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>prj2-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <classifier>tests</classifier>
+ <scope>test</scope>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>prj1-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <classifier>tests</classifier>
+ <scope>test</scope>
+ </dependency>
+
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>3.8.2</version>
+ <scope>test</scope>
Review Comment:
The indentation uses tabs instead of spaces, which is inconsistent with the
XML file's general indentation style. This dependency declaration should use
spaces to match the surrounding code.
```suggestion
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>3.8.2</version>
<scope>test</scope>
```
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
Review Comment:
The patch module value will have a trailing path separator (PS) after the
loop completes. For example, if there are two paths, the result would be
"moduleName=path1:path2:" with an extra colon at the end. This trailing
separator should be removed after the loop to ensure the --patch-module
argument is correctly formatted.
```suggestion
}
if (!patchModuleEntry.getValue().isEmpty()) {
patchModuleValue.setLength(patchModuleValue.length()
- 1);
}
```
##########
src/it/MCOMPILER-372_modularized_testjar/prj1/pom.xml:
##########
@@ -0,0 +1,83 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>prj1-372</artifactId>
+
+ <build>
+
+ <plugins>
+
+ <!-- Needed to install test jar to repository -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <version>3.2.0</version>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+
+ </plugins>
+
+ </build>
+
+ <dependencies>
+
+ <!-- log4j -->
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>2.12.1</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <version>2.12.1</version>
+ </dependency>
+
+ <!-- Test dependencies -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>3.8.2</version>
+ <scope>test</scope>
Review Comment:
The indentation uses tabs instead of spaces, which is inconsistent with the
XML file's general indentation style. This dependency declaration should use
spaces to match the surrounding code.
```suggestion
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>3.8.2</version>
<scope>test</scope>
```
##########
src/it/MCOMPILER-372_modularized_testjar/prj0/pom.xml:
##########
@@ -0,0 +1,132 @@
+<?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
+ ~
+ ~ 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.
+-->
+
+<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
http://maven.apache.org/maven-v4_0_0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <description>This project tests a JAXB XSD to java test compilation
</description>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it.372</groupId>
+ <artifactId>mcompiler-372</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>prj0-372</artifactId>
+
+ <build>
+
+ <plugins>
+
+ <!-- Needed to install test jar to repository -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-jar-plugin</artifactId>
+ <version>3.2.0</version>
+ <executions>
+ <execution>
+ <goals>
+ <goal>test-jar</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+
+ <!-- generate JAXB stubs for schemas -->
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>jaxb2-maven-plugin</artifactId>
+ <version>2.5.0</version>
+ <executions>
+
+ <!-- Dummy configuration XSD -->
+ <execution>
+ <id>config</id>
+ <goals>
+ <goal>xjc</goal>
+ </goals>
+ <configuration>
+ <!-- XSD schema -->
+ <sources>
+ <source>src/main/resources/prj0/Prj0Conf.xsd</source>
+ </sources>
+ <packageName>prj0.conf</packageName>
+
<outputDirectory>${project.build.directory}/generated-sources/xjcConf</outputDirectory>
+ </configuration>
Review Comment:
The indentation uses tabs instead of spaces, which is inconsistent with the
XML file's general indentation style. This should use spaces to match the
surrounding code.
```suggestion
</sources>
<packageName>prj0.conf</packageName>
<outputDirectory>${project.build.directory}/generated-sources/xjcConf</outputDirectory>
</configuration>
```
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
+
+ compilerArgs.add(patchModuleValue.toString());
+ }
+
compilerArgs.add("--add-reads");
compilerArgs.add(mainModuleDescriptor.name() + "=ALL-UNNAMED");
} else {
modulepathElements = Collections.emptyList();
classpathElements = testPath;
}
+
+ }
+ }
+
+ /**
+ * Compiling with test classpath is not sufficient because some
dependencies <br/>
+ * may be modularized and their test class path addition would be
ignored.<p/>
+ * Example from MCOMPILER-372_modularized_test:<br/>
+ * prj2 test classes depend on prj1 classes and test classes<br/>
+ * As prj1.jar is added as a module, pjr1-tests.jar classes are ignored if
we add jar class path<br/>
+ * We have to add pjr1-tests.jar as --patch-module<p/>
+ * @param mainModuleName
+ * param mainModuleTarget
+ * @param patchModules map of module names -> list of paths to add to
--patch-module option
+ *
+ */
+ protected void patchModulesForTestDependencies(
+ String mainModuleName, String mainModuleTarget,
+ LinkedHashMap<String, List<String>> patchModules
+ ) {
+ File mainModuleDescriptorClassFile = new File(mainModuleTarget,
"module-info.class");
+
+ ResolvePathsResult<File> result;
+
+ try {
+ // Get compile path information to identify modularized compile
path elements
+ Collection<File> dependencyArtifacts =
getCompileClasspathElements(getProject());
+
+ ResolvePathsRequest<File> request =
+ ResolvePathsRequest.ofFiles(dependencyArtifacts)
+ .setMainModuleDescriptor(mainModuleDescriptorClassFile);
+
+ Toolchain toolchain = getToolchain();
+ if (toolchain instanceof DefaultJavaToolChain) {
+ request.setJdkHome(new File(((DefaultJavaToolChain)
toolchain).getJavaHome()));
+ }
+
+ result = locationManager.resolvePaths(request);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
+ // Prepare a path list containing test paths for modularized paths
+ // This path list will augment modules to be able to compile
+ List<String> listModuleTestPaths = new ArrayList<>(testPath.size());
+
+ // Browse modules
+ for (Entry<File, ModuleNameSource> pathElt :
result.getModulepathElements().entrySet()) {
+
+ File modulePathElt = pathElt.getKey();
+
+ // Get module name
+ JavaModuleDescriptor moduleDesc = result.getPathElements().get(
modulePathElt );
+ String moduleName = (moduleDesc != null) ? moduleDesc.name() :
null;
+
+ if (
+ // Is it a modularized compile elements?
+ ( modulePathElt != null ) && ( moduleName != null )
+ &&
+ // Is it different from main module name
+ !mainModuleName.equals( moduleName )
+ ) {
+
+ // Get test path element
+ File moduleTestPathElt = getModuleTestPathElt(modulePathElt);
+
+ if (moduleTestPathElt != null) {
+
listModuleTestPaths.add(moduleTestPathElt.getAbsolutePath());
+ }
+ }
+ }
+
+ // Remove main target path
+ listModuleTestPaths.remove(mainModuleTarget);
+ listModuleTestPaths.remove(outputDirectory.getAbsolutePath());
+
+ // Freeze list
+ listModuleTestPaths =
Collections.unmodifiableList(listModuleTestPaths);
+
+ if (getLog().isDebugEnabled()) {
+ getLog().debug("patchModule test paths:");
+ for (String moduleTestPath : listModuleTestPaths) {
+ getLog().debug(" " + moduleTestPath);
+ }
+
+ }
+
+ // Get modularized dependencies resolved before
+ for (Entry<File, ModuleNameSource> pathElt :
result.getModulepathElements().entrySet()) {
+ File path = pathElt.getKey();
+
+ // Get module name
+ JavaModuleDescriptor moduleDesc =
result.getPathElements().get(path);
+ String moduleName = (moduleDesc != null) ? moduleDesc.name() :
null;
+
+ if (
+ // Is it a modularized compile elements?
+ ( path != null ) && ( moduleName != null )
+ &&
+ // Not an auto-module
+ !moduleDesc.isAutomatic()
+ &&
+ // Is it different from main module name
+ !mainModuleName.equals( moduleName )
+ ) {
+
+ // Add --add-reads <moduleName>=ALL-UNNAMED
+ compilerArgs.add( "--add-reads" );
+ StringBuilder sbAddReads = new StringBuilder();
+ sbAddReads.append(moduleName).append("=ALL-UNNAMED");
+ compilerArgs.add(sbAddReads.toString());
+
+ // Add compile classpath if needed
+ if (!listModuleTestPaths.isEmpty()) {
+ // Yes, add it as patch module
+ List<String> listPath = patchModules.get(moduleName);
+ // Make sure it is initialized
+ if (listPath == null) {
+ listPath = new ArrayList<>();
+ patchModules.put(moduleName, listPath);
+ }
+
+ // Add test compile path but not main module target
+ listPath.addAll(listModuleTestPaths);
+ }
+ }
+ }
+
+ if (pathElements == null) {
+ pathElements = new
LinkedHashMap<>(result.getPathElements().size());
+ for (Entry<File, JavaModuleDescriptor> entry :
result.getPathElements().entrySet()) {
+ pathElements.put(entry.getKey().getAbsolutePath(),
entry.getValue());
+ }
}
}
+ /**
+ * Get module test path element from module path element
+ * @param modulePathElt
+ * @return
+ */
+ protected File getModuleTestPathElt(File modulePathElt) {
+ // Get test path from reactor projects
+ File result = getModuleTestPathEltFromReactorProjects(modulePathElt);
+ return result;
+
Review Comment:
This method simply calls getModuleTestPathEltFromReactorProjects and returns
its result. The intermediate variable assignment is unnecessary and the method
adds no value. Consider either removing this wrapper method and calling
getModuleTestPathEltFromReactorProjects directly, or adding logic that
justifies this layer of indirection.
```suggestion
if (modulePathElt == null) {
return null;
}
// Get test path from reactor projects
return getModuleTestPathEltFromReactorProjects(modulePathElt);
```
##########
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:
##########
@@ -350,27 +353,218 @@ protected void preparePaths(Set<File> sourceFiles) {
if (compilerArgs == null) {
compilerArgs = new ArrayList<>();
}
- compilerArgs.add("--patch-module");
-
- StringBuilder patchModuleValue = new
StringBuilder(mainModuleDescriptor.name())
- .append('=')
- .append(mainOutputDirectory)
- .append(PS);
- for (String root : compileSourceRoots) {
- patchModuleValue.append(root).append(PS);
- }
-
- compilerArgs.add(patchModuleValue.toString());
+ // Patch module to add
+ LinkedHashMap<String, List<String>> patchModules = new
LinkedHashMap<>();
+
+ List<String> mainModulePaths = new ArrayList<>();
+ // Add main output dir
+ mainModulePaths.add(mainOutputDirectory.getAbsolutePath());
+ // Add source roots
+ mainModulePaths.addAll(compileSourceRoots);
+ patchModules.put(mainModuleDescriptor.name(), mainModulePaths);
+
+ // Main module detected, modularized test dependencies must
augment patch modules
+ patchModulesForTestDependencies(
+ mainModuleDescriptor.name(),
mainOutputDirectory.getAbsolutePath(),
+ patchModules
+ );
+
+ for (Entry<String, List<String>> patchModuleEntry:
patchModules.entrySet()) {
+ compilerArgs.add("--patch-module");
+ StringBuilder patchModuleValue = new
StringBuilder(patchModuleEntry.getKey())
+ .append('=');
+
+ for (String path : patchModuleEntry.getValue()) {
+ patchModuleValue.append(path).append(PS);
+ }
+
+ compilerArgs.add(patchModuleValue.toString());
+ }
+
compilerArgs.add("--add-reads");
compilerArgs.add(mainModuleDescriptor.name() + "=ALL-UNNAMED");
} else {
modulepathElements = Collections.emptyList();
classpathElements = testPath;
}
+
+ }
+ }
+
+ /**
+ * Compiling with test classpath is not sufficient because some
dependencies <br/>
+ * may be modularized and their test class path addition would be
ignored.<p/>
+ * Example from MCOMPILER-372_modularized_test:<br/>
+ * prj2 test classes depend on prj1 classes and test classes<br/>
+ * As prj1.jar is added as a module, pjr1-tests.jar classes are ignored if
we add jar class path<br/>
+ * We have to add pjr1-tests.jar as --patch-module<p/>
Review Comment:
There's a typo in the javadoc comment: "pjr1-tests.jar" should be
"prj1-tests.jar" to match the project naming convention.
```suggestion
* As prj1.jar is added as a module, prj1-tests.jar classes are ignored
if we add jar class path<br/>
* We have to add prj1-tests.jar as --patch-module<p/>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
