-
Notifications
You must be signed in to change notification settings - Fork 179
Add a parser for module-info-patch.maven
files where `--add-exports and similar options can be specified more easily
#963
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
base: master
Are you sure you want to change the base?
Conversation
Maybe @slawekjaranowski as a reviewer here too? Like #959 the decision about accepting a Note: an email has been sent to the developer mailing list about those issues, but got no reaction yet. |
I'd like to experiment switching JLine's master branch to the new plugins, since i'm migrating it to Maven 4 with JPMS support. |
Documentation about this feature has been added in #976, in particular in the |
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.
Pull Request Overview
This PR introduces support for parsing module-info-patch.maven
files that provide a more convenient way to specify module system options like --add-exports
and --add-reads
for testing purposes. Instead of overwriting module-info.java
files in test directories or configuring verbose XML in the POM, developers can use a Maven-specific syntax that maps directly to Java compiler options.
Key changes:
- Adds a new parser for
module-info-patch.maven
files with Java-like syntax for module system options - Integrates the parser into the test compilation workflow to generate appropriate compiler flags
- Updates existing code to handle the new patch-based approach while maintaining backward compatibility
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/test/resources/org/apache/maven/plugin/compiler/module-info-patch.maven |
Example patch file showing the supported syntax and options |
src/test/java/org/apache/maven/plugin/compiler/ModuleInfoPatchTest.java |
Test class validating the parser functionality and output generation |
src/main/java/org/apache/maven/plugin/compiler/ToolExecutorForTest.java |
Integration of patch file parsing into the test compilation process |
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java |
Updated comment clarifying main code handling |
src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java |
Updated deprecation warnings to reference the new patch file approach |
src/main/java/org/apache/maven/plugin/compiler/ModuleInfoPatchException.java |
Exception class for patch file parsing errors |
src/main/java/org/apache/maven/plugin/compiler/ModuleInfoPatch.java |
Main parser implementation for module-info-patch.maven files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/main/java/org/apache/maven/plugin/compiler/ModuleInfoPatch.java
Outdated
Show resolved
Hide resolved
…` and similar options can be specified more easily.
Co-authored-by: Copilot <[email protected]>
7d9f47e
to
653b2fd
Compare
This is a variation of the practice consisting in overwriting the
module-info.java
of the main code with anothermodule-info.java
defined in the test directory. Instead, themodule-info.java
file formerly defined in the test would be replaced by amodule-info-patch.maven
file in the same directory. The latter is Maven-specific: the content ofmodule-info-patch.maven
looks likemodule-info.java
, but is not standard Java, hence the.maven
file suffix. The principles are:module-info.java
file for testing purposes is declared inmodule-info-patch.maven
.module-info.java
is not inmodule-info-patch.maven
neither. In particular, everything that specify paths to JAR files stay in thepom.xml
file.patch-module
block of that file map directly to Java compiler or Java launcher options.The keywords are
add-modules
,limit-modules
,add-reads
,add-exports
andadd-opens
. All these keywords map tojavac
options of the same names. Note that they are options where the values are package or module names, not paths to source or binary files. Options with path values should be handled in the<sources>
and<dependencies>
elements of the POM instead. Below is an example of amodule-info-patch.maven
file content for modifying themodule-info
of a module namedmy.product.foo
:Why this approach
It is not convenient to put these options in the plugin configuration in the POM because these options need to be specified on a module-per-module (in JPMS sense) basis, while the plugin configuration in the POM applies to the whole module source hierarchy. Actually, the options of all modules end up merged together in a single set of options for
javac
, but in a more verbose form for differentiating each module. For example, if the optionadd-reads org.junit.jupiter.api
is declared inside themy.product.foo
module, it becomes--add-reads my.product.foo=org.junit.jupiter.api
in the options given tojavac
. It is easer for developers to read and maintain their code if this verbosity is managed by the compiler plugin, in addition of avoiding the XML verbosity of the POM.Another demonstration that options declared in plugin configuration are impracticable is that many developers prefer to provide a
module-info.java
in their test which override themodule-info.java
of their main code. But this approach has problems: the testmodule-info.java
must duplicate everything that is in the mainmodule-info.java
. If they diverge, the tests may not behave as the main code would do. Furthermore, this approach sometime requires hacks in the compiler plugin such as pretending that the tests are the main code and the main code is a patch applied over the tests. For avoiding those issues,javac
options such as--add-reads
should be used instead, but they are tedious to write and maintain as plugin configuration in the POM.Impact on other plugins
Other plugins do not need to parse this
module-info-patch.maven
file. Only the Maven Compiler Plugin needs to do so. The compiler plugin generates aMETA-INF/maven/module-info-patch.args
file which would be consumed by the Surefire plugin for executing the tests. Other plugins would need to care about this file only if they have something to do with tests. Note that Maven 3 already generates a file for JPMS arguments, but with a different content. So this approach is not really new.Gradual approach proposal
We could take a gradual approach by accepting this pull request with an amendment: if a
module-info-patch.maven
file is found, print a warning saying that this is an experimental feature. I think that the risk is low because most users will probably not experiment this feature until the Surefire is upgraded for consuming the above-citedMETA-INF/maven/module-info-patch.args
file. But having feedbacks from peoples willing to try on the command-line would be very valuable.