gnodet-bot commented on code in PR #1170:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1170#discussion_r4236696088
##########
src/main/java/org/apache/maven/plugin/compiler/CompilerMojo.java:
##########
@@ -398,6 +399,72 @@ private List<File>
getCompileClasspathElements(MavenProject project, List<File>
return list;
}
+ /**
+ * Replaces exploded multi-release project outputs with the versioned
directory containing their module descriptor.
+ * The Java module system does not apply multi-release JAR lookup rules to
directories, so a descriptor under
+ * {@code META-INF/versions/<N>} is otherwise invisible when another
reactor project is compiled before packaging.
+ *
+ * @param artifacts compile path entries, including this project's own
output directories
+ * @param projectOutputDirectories this project's outputs, which are
already arranged for multi-release compilation
+ */
+ private void resolveMultiReleaseDependencies(Collection<File> artifacts,
List<File> projectOutputDirectories) {
+ String release = getRelease();
+ if (release == null) {
+ release = getSource();
+ }
+ if (release == null) {
+ release = getTarget();
+ }
+ int releaseVersion =
+ getJavaMajorVersion(release != null ? release :
System.getProperty("java.specification.version"));
+ List<File> resolved = new ArrayList<>(artifacts.size());
+ for (File artifact : artifacts) {
+ if (!projectOutputDirectories.contains(artifact)) {
+ File moduleDirectory = findModuleDirectory(artifact,
releaseVersion);
+ if (moduleDirectory != null) {
+ artifact = moduleDirectory;
+ }
+ }
+ resolved.add(artifact);
+ }
+ artifacts.clear();
+ artifacts.addAll(resolved);
Review Comment:
⚠️ **Critical — module path entries lose their exported classes**
`findModuleDirectory` replaces `module-a/target/classes` with
`module-a/target/classes/META-INF/versions/9/` on the module path. That
versioned directory contains only `module-info.class`; the actual package
classes (e.g. `sample/a/Api.class`) live in `target/classes/`. Once the
replacement happens, any downstream module that imports classes from `module-a`
will fail to compile — javac can resolve the module descriptor (satisfying
`requires`), but cannot see the exported types.
The IT masks this because `module-b` only has a `module-info.java` with a
bare `requires` declaration and no source file that uses `module-a`'s API. The
described fix — also adding `--patch-module
<module-name>=<original-classes-dir>` for each affected dependency — is the
correct approach, mirroring what the existing `multiReleaseOutput` logic at
line 321–328 does for the project's own output.
Without this fix, the approach is incomplete: it solves module descriptor
resolution but breaks class resolution for any real downstream consumer.
##########
src/it/MCOMPILER-1137/module-b/src/main/java9/module-info.java:
##########
@@ -0,0 +1,21 @@
+/*
+ * 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.
+ */
+module org.apache.maven.plugins.compiler.it.moduleb {
+ requires org.apache.maven.plugins.compiler.it.modulea;
+}
Review Comment:
⚠️ **IT doesn't test the critical scenario: actual class imports from the
reactor dependency**
`module-b` only declares `requires
org.apache.maven.plugins.compiler.it.modulea` but never uses any class from
`module-a` in Java source. The path-replacement bug (see main finding) would
only manifest if a downstream module compiled source files that import exported
types. Adding a `module-b/src/main/java9/UseApi.java` like:
```java
package sample.b;
import sample.a.Api;
public class UseApi { public static void demo() { Api.value(); } }
```
would turn this IT into an actual regression test for the scenario described
in the PR title.
##########
src/it/MCOMPILER-1137/invoker.properties:
##########
@@ -0,0 +1,18 @@
+# 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.
+
+invoker.goals=clean compile
Review Comment:
⚠️ **Missing `invoker.java.version` guard**
This IT exercises module-path compilation with `release=9`. Without
`invoker.java.version = 9+`, it will be attempted (and fail) on JDK 8. All
other module-path ITs in this plugin set this property — e.g.
`MCOMPILER-321_pathexceptions/invoker.properties` line 18.
```suggestion
invoker.goals=clean compile
invoker.java.version = 9+
```
##########
src/main/java/org/apache/maven/plugin/compiler/CompilerMojo.java:
##########
@@ -398,6 +399,72 @@ private List<File>
getCompileClasspathElements(MavenProject project, List<File>
return list;
}
+ /**
+ * Replaces exploded multi-release project outputs with the versioned
directory containing their module descriptor.
+ * The Java module system does not apply multi-release JAR lookup rules to
directories, so a descriptor under
+ * {@code META-INF/versions/<N>} is otherwise invisible when another
reactor project is compiled before packaging.
+ *
+ * @param artifacts compile path entries, including this project's own
output directories
+ * @param projectOutputDirectories this project's outputs, which are
already arranged for multi-release compilation
+ */
+ private void resolveMultiReleaseDependencies(Collection<File> artifacts,
List<File> projectOutputDirectories) {
Review Comment:
💡 **API design: `Collection<File>` parameter is mutated in-place**
`resolveMultiReleaseDependencies(Collection<File> artifacts, ...)` takes a
`Collection<File>` and calls `artifacts.clear(); artifacts.addAll(resolved)`.
This is only safe because `getCompileClasspathElements()` returns a fresh
`ArrayList`. Accepting `Collection<File>` on the signature implies callers can
pass any collection (e.g. an unmodifiable view), but it silently requires
mutability. Consider either accepting `List<File>` (closer to the actual
contract) or returning `List<File>` and letting the caller reassign, making the
intent explicit.
##########
src/it/MCOMPILER-1137/invoker.properties:
##########
@@ -0,0 +1,18 @@
+# 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.
+
+invoker.goals=clean compile
Review Comment:
💡 **No `verify.groovy` script**
`MCOMPILER-328_multiReleaseOutput` has a verify script that asserts expected
class files exist under `META-INF/versions/`. This IT should do the same: at
minimum verify that
`module-b/target/classes/META-INF/versions/9/module-info.class` exists and that
`module-a`'s versioned descriptors were produced. Without this, the IT only
checks that `mvn compile` exits 0 — not that the right artifacts were generated.
--
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]
