[MCOMPILER-374] Unable to compile MR-JAR code against older directories when module-info.java is present - #1093
Conversation
|
Assuming that this issue is specific to 3.x, since 4.x should handle multi-release automatically. |
elharo
changed the title
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
Let's try to avoid abbreviation in documentation. Specifically MR-JAR --> multirelease JAR. I had to read quite a bit to figure out what MR-JAR meant.
d49f6b1 to
2b263a2
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Updates MR-JAR handling so that when module-info.java is involved, earlier MR output directories are included consistently in both classpath and --patch-module, and adds coverage to validate module descriptor selection and a dedicated integration test.
Changes:
- Build an ordered “visible output directories” list for MR-JARs and reuse it for classpath/module patching.
- Extend
--patch-moduleconstruction to include earlier MR-JAR output layers when compiling as a named module. - Add unit + integration tests for layered MR-JAR compilation involving
module-info.java.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java | Adds shared helpers to enumerate MR-JAR output directories and parse Java major versions. |
| src/main/java/org/apache/maven/plugin/compiler/CompilerMojo.java | Reuses ordered output directories for classpath/patch-module when compiling named modules with MR output. |
| src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java | Selects the applicable module-info.class across MR layers and adjusts patch-module behavior for test compilation. |
| src/test/java/org/apache/maven/plugin/compiler/TestCompilerMojoTest.java | Adds a unit test for selecting the most-specific existing module descriptor across MR layers. |
| src/it/MCOMPILER-374_mrjar/pom.xml | Adds an IT project exercising releases 8/9/11 with MR output and module-info at 11. |
| src/it/MCOMPILER-374_mrjar/verify.groovy | Verifies expected class outputs across MR layers and test compilation output. |
| src/it/MCOMPILER-374_mrjar/invoker.properties | Pins IT execution to Java 11+ (needed for module-info). |
| src/it/MCOMPILER-374_mrjar/src/main/java/** | Adds base release-8 sources for MR-JAR scenario. |
| src/it/MCOMPILER-374_mrjar/src/main/java9/** | Adds release-9 layer sources used by higher layers/tests. |
| src/it/MCOMPILER-374_mrjar/src/main/java11/** | Adds release-11 layer sources plus module-info.java. |
| src/it/MCOMPILER-374_mrjar/src/test/java/** | Adds test source that references both base and layered MR classes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The selected descriptor directory represents the main module; patch in the remaining layers. | ||
| if (mainOutputDirectories.size() > 1) { | ||
| addPatchModule(mainModuleDescriptor.name(), mainOutputDirectories, Collections.emptyList()); | ||
| } |
There was a problem hiding this comment.
The selected descriptor directory is included intentionally. A module-info.class encountered on a patch path cannot replace the module descriptor and is ignored by javac, so including that directory does not reintroduce a different descriptor. From JEP 261: The --patch-module option cannot be used to replace module-info.class files. If a module-info.class file is found in a module definition on a patch path then a warning will be issued and the file will be ignored.
The directory order is important for the remaining classes. Patch-path content takes precedence over both later patch-path entries and the original module contents. The selected release directory must therefore come first, followed by older versioned directories and the base output directory. Otherwise, an older patched class can incorrectly shadow the corresponding class from the selected release.
I reproduced this with a class present in both the selected Java 11 layer and the base layer. With --patch-module module=v11:base, compilation correctly sees the Java 11 class. After removing v11 from the patch path, the base class shadows it and compilation fails when a Java 11-only method is referenced.
I agree that the “remaining layers” comment is misleading; I will reword it to explain that all output layers are patched in multirelease lookup order.
2b263a2 to
acdbafa
Compare
|
I would like to move forward with this. Can the reviewing AI create a reproducer? Because mine can't. |
|
Summary
--patch-modulepath when the current compilation containsmodule-info.java.Motivation
The plugin already added earlier MR-JAR output directories to the class path. Once
module-info.javais present, however,javacresolves the sources as a named module and the class path alone does not make those earlier classes part of that module.The existing
--patch-moduleoption only contained the base output directory. The patch path now contains each existing earlier release directory in descending order, followed by the base output directory. This matches MR-JAR shadowing order and lets the modular execution see the output of earlier executions.Fixes #579
Testing
JAVA_HOME=/opt/jdks/latest-17 /opt/maven/latest/bin/mvn clean verifyJAVA_HOME=/opt/jdks/latest-17 /opt/maven/latest/bin/mvn -Prun-its clean verifyJAVA_HOME=/opt/jdks/latest-17 /opt/maven/latest/bin/mvn -Prun-its -Dinvoker.test=MCOMPILER-374_mrjar clean verifyJAVA_HOME=/opt/jdks/latest-17 /opt/maven/latest/bin/mvn -Prun-its -Dinvoker.test=MCOMPILER-373_mrjar clean verifyFollowing this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MCOMPILER-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.