Skip to content
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

Rewrite the plugin using standard javax.tools API instead of Plexus. #271

Merged
merged 6 commits into from
Nov 14, 2024

Conversation

desruisseaux
Copy link
Contributor

This is a proposed rewriting of the Maven compiler plugin. This work is for Maven 4 only, the plugin for Maven 3 will stay unaffected. A major goal of this work is to improve the support of Java Module System in Maven.

Incompatible changes

This section describes behaviors of the Maven compiler plugin 4 that changed in incompatibles way compared to versions 3. Some projects building successfully with version 3 of maven-compiler-plugin may fail with version 4 because of those changes. This section describes corrections that can be applied to projects for fixing those build failures.

Cannot use both --release and --source

As a general rule, this new Maven plugin does not modify the compiler options specified in the configuration. This policy may break projects that specify the <release> option together with <source> and <target>. The previous version of the plugin applied some heuristic rules for deciding which options to pass to the Java compiler. The new version passes all options verbatim, which may result in the following message from the javac compiler:

option --source cannot be used together with --release

Correction if the build fails

Removes the <source> and <target> configuration parameters. They may be specified indirectly through the maven.compiler.source and maven.compiler.target properties. If those configuration parameters or properties are inherited from a super-POM that cannot be modified, clear the <source> and <target> parameters as below:

<project>
  <build>
    <plugins>
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-compiler-plugin</artifactId>
        <configuration>
          <source/>
          <target/>
          <release>17</release>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

Use of dependencies with automatic names

If a dependency does not declare explicitly whether the JAR file should be placed on the class-path or on the module-path, the Maven plugin uses heuristic rules for making its own decision. Those rules are not obvious, and the selected option is not always appropriate for the project. The results may sometime be different between this version and the previous version of maven-compiler-plugin. Some JAR files may placed on the class-path when they were previously on the module-path, or conversely.

By default, a dependency is placed on --module-path if the project is itself modularized (i.e., contains a module-info.java) and one of the following conditions is true:

  • The JAR contains a module-info.class entry.
  • The JAR contains a META-INF/MANIFEST.MF entry with an Automatic-Module-Name attribute.

In all other cases, the dependency is placed on --class-path. This default behavior may be adjusted in future Maven plugin versions depending on experience.

Correction if the build fails

Developers are strongly encouraged to specify explicitly where they want their non-modular JAR files to appear in a modular project. It can be done by setting the type of a dependency to modular-jar or classpath-jar. Example:

<project>
  <dependencies>
    <dependency>
      <groupId>org.codehaus.plexus</groupId>
      <artifactId>plexus-utils</artifactId>
      <version>3.0.24</version>
      <type>modular-jar</type>
    </dependency>
  </dependencies>
</project>

Note that specifying the dependency type is usually not needed in other cases (e.g., non-modular project, or dependencies that are already modular). It should not be needed neither for dependencies with the runtime scope. However, it may be a good practice to be explicit anyway for more guarantees.

Guideline for choosing the dependency type

If a non-modular dependency is forced to the modular-jar type, Java will automatically infer a module name from the file name. That name can be shown by the jar --describe-module command. For example, the following command shows the name of above plexus-utils dependency on a Unix system:

jar --describe-module --file ~/.m2/repository/org/codehaus/plexus/plexus-utils/3.0.24/plexus-utils-3.0.24.jar

The command output contains plexus.utils@3.0.24 automatic. That module name must be declared in the module-info.java file.

module my.module {
    requires plexus.utils;
}

Conversely, if a dependency is forced to the classpath-jar type, then the module-info.java is not modified. Instead, the pom.xml file should have the following configuration (replace my.module by the actual project module name):

<project>
  <build>
    <plugins>
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-compiler-plugin</artifactId>
        <configuration>
          <compilerArgs>
            <arg>--add-reads</arg>
            <arg>my.module=ALL-UNNAMED</arg>
          </compilerArgs>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

jpms.args generation

The META-INF/jpms.args file is no longer generated.

Changes in compiler parameters

In its current version, the new plugin does not remove any parameters that exist in version 3 of maven-compiler-plugin. However, the behavior of some parameters has been modified and some parameters are deprecated. This section lists the changes.

Deprecated but still working parameters

The following parameters are deprecated but are still working:

  • <compilerArgument>: already replaced by <compilerArgs> since Maven 3.1. The replacement uses a list of strings instead of a single string.
  • <testCompilerArgument>: replaced by <testCompilerArgs> for the same reason as above. Note that the latter is a new parameter introduced in Maven 4, see next point.
  • <testCompilerArguments>: replaced by <testCompilerArgs> for consistency with the main plugin. The former parameter was inconsistent in the name ("Arguments" suffix instead of "Args"), but also in the value type (Map<String,String> instead of List<String>).
  • <annotationProcessorPaths>: replaced by ordinary dependencies with <type>proc</type>. The latter is a new artifact type introduced in Maven 4-alpha13.
  • <useModulePath>: replaced by <type>classpath-jar</type> declarations in dependencies.

Deprecated parameters that are no-op

The following parameters are marked as deprecated for removal. They all became no-op in the new plugin:

  • <forceJavacCompilerUse>: the documentation is not really explicit, but this parameters seems to be about forcing the use of java.lang.Compiler API instead of javax.tools.JavaCompiler. The former class was deprecated since Java 9 and no longer exists in Java 21.
  • <compilerVersion>: this parameter was passed to org.codehaus.plexus.compiler.CompilerConfiguration, but it is unclear how the Plexus compiler used it. We see no obvious mapping in javax.tools or java.lang.Process API.
  • <compilerReuseStrategy>: does not apply well to the javax.tools.JavaCompiler API that the new plugin is using.
  • <skipMultiThreadWarning>: deprecated as a consequence of <compilerReuseStrategy> deprecation.
  • <optimize>: was already deprecated in Maven 3, reported here for completness.
  • <outputFileName>: producing the final JAR file can be considered as the task of separated plugins, for example the JAR plugin. Furthermore, this parameter does not work well in the context of Module Source Hierarchy, because the output is not a single JAR file.
  • <outputTimestamp>: not really applicable to the Maven compiler plugin. It was used only as an heuristic rules for guessing if the developer intended to have a reproducible build.

Changes in default values

The Maven compiler plugin has numerous default values, which sometime differ from the standard (JDK) default values. For example, the default value of -source and -target options was hard-coded to "1.8" in Maven 3, while the standard (JDK) default is the version of the JDK used for compiling the code. The following parameters had their default value modified in the new plugin:

  • <source> and <target>: removed the default value and added a Javadoc saying "As of Java 9, the --release option is preferred."
  • <release>: no default value. Therefor, the standard (JDK) default applies.
    • If both <release> and <target> are unspecified, a warning is logged.
  • <debugLevel>: omitting this parameter now means "JDK default debug info" instead of "all debug info".
    • A new all level (Maven-specific) has been added for meaning "all debug info".
  • <createMissingPackageInfoClass>: default changed from true to false,
    because this workaround is no longer necessary for the new incremental build system.
  • <showWarnings>: default to true for consistency with the JDK default value.

Default values not yet changed

The following changes are in consideration, but not yet applied. The intend for those changes would be to have the same default values as the javac tool:

  • <meminitial> and <maxmem>: if no unit is specified, change the default from 'M' to bytes as interpreted by the javac tool.

Changes in parameters validation

  • Include/exclude filters accept also glob and regex syntax (see implementation changes).
  • All parameters for memory settings accept also 'K' and 'G' suffix in addition of 'M'.
  • <debugLevel> values unknown to the plugin but known by the compiler are now accepted.

Changes in implementation

This section lists some noticeable implementation changes.

Plexus dependencies removed

The major change is the replacement of Plexus compiler API by the standard javax.tools.JavaCompiler API introduced in Java 6. This changes required an almost full rewrite of the compiler plugin. Other Plexus dependencies such as StringUtils could also be removed because they now have replacements in the standard Java API.

ASM no longer used for generating package-info

The previous compiler plugin used ASM for generating the package-info.class files that were omitted by the Java compiler because empty. This issue is moot because the package-info.class files were generated as a workaround for the way that Maven incremental compilation worked. The new plugin no longer generate those classes by default (i.e., the <createMissingPackageInfoClass> default value has been changed to false). If empty package-info.class files are nevertheless desired, they are generated by the Java compiler itself with the -Xpkginfo:always option. If that extra option is not supported, then a warning is logged and no option is added.

Heuristic removed

The previous plugin tried to be clever by intercepting and modifying the values of user-supplied --patch-module compiler arguments, by adding --path-module and --add-reads <module>=ALL-UNNAMED arguments on its own initiative, etc. The result was saved in a META-INF/jpms.args file added to the JAR file. Above-cited heuristics have been removed from the new compiler plugin, as this plugin aims to give more configuration control to the users. For example, developers are encouraged to set a dependency type to modular-jar or classpath-jar instead of jar. Some heuristics have been kept for compatibility with current widespread practice, in particular when the dependency type is only jar. But most heuristics should not be applied when developers use the most explicit ways to configure Maven.

Include/exclude filters

The Plexus scranners used for include/exclude filters have been replaced by java.nio.file.PathMatcher. The set of allowed syntax contains at least "glob" and "regex". See FileSystem.getPathMatcher(String) Javadoc for a description of the "glob" syntax. If no syntax is specified, then the default syntax is "glob" with the following modification:

  • Unless escaped by \, all occurrences of the / character are replaced by the platform-specific separator.

The list of files to process is built by applying the path matcher on each regular (non directory) files. The walk in file trees has the following characteristics:

  • Symbolic links are followed.
  • Hidden files and hidden directories are ignored.

Incremental compilation

The code for detecting which files to recompile has been rewritten. The previous code had an issue with list of files generated and compared in one case as absolute files, and in other case as relative files. Consequently, the plugin always considered that all files changed, which may explain the performance issue reported in MCOMPILER-209.

The new implementation saves the list of source files, together with their last modification times, in a binary file with the .cache extension instead of a text file with the .lst extension. This custom binary encoding is more compact than the previous text files (because of less redundancies) and has more information. The incremental compilation behavior is modified as below:

  • Changes in compiler options cause a recompilation of all files. Those changes are detected on a "best effort" basis only.
  • By default, addition of new source files does not cause the recompilation of all files, because this is usually not necessary.
  • The modification times of source files are compared with the modification times of the same source files during the previous build (saved in above-cited binary file), instead of compared with the modification times of the class files.

The last point avoids the problem that the <createMissingPackageInfoClass> parameter tried to solve. Because the new algorithm does not depend on .class timestamps, there is no need to force their creation.

If the <useIncrementalCompilation> parameter is set to false, the plugin does not use the binary cache file and compare modification times of source files with modification times of class files. This approach reproduces the previous behavior.

Detection of changes in dependencies

When checking if a dependency changed, the new implementation compares the JAR timestamps against the start time of the previous build saved in the binary file instead of comparing against the value given by session.getStartTime(). The reason for this change is as below:

  • Consider a project with 3 modules named A, B and C where modules B and C depend on module A.
  • Module A is modified and mvn install is executed on the command-line.
  • A.jar get a modification time which is after the build start time. Module B and C detect that fact and perform a "clean and build all". This is the current behavior of Maven 3 incremental compilation.

But let assume that the compilation failed in module B because of the change in A. The developer fixes the compilarion error in B, then executes mvn install a second time:

  • Compilation of A is skipped because nothing changed. Consequently, the timestamp of A.jar is unchanged.
  • With the Maven 3 implementation, module C will not detect that module A changed because the modification time of A.jar is before the second build start time.

By using the timestamp saved in the binary file instead, the new algorithm will detect that A.jar has been updated since the last build of C.

Overwriting module-info in tests

For modular projects having a module-info.java file in their main sources, Maven 3 recommended to declare the test dependencies in another module-info.java file located in the test sources. The latter was basically a copy of the former with the addition of, for example, requires org.junit.jupiter.api statements. This approach is deprecated in Maven 4. Java does not let us overwrite module-info easily, maybe for security reasons. Maven 3 supported that approach by putting the project upside-down: the test classes were considered the project's main code (so that the test's module-info takes precedence), and the main classes were considered the patch added on top of the tests. This new compiler plugin keeps the main classes as main and the test classes as patch, and instead executes the following steps:

  1. Compile the test module-info.java file alone.
  2. Temporarily rename the main module-info.class file as module-info.class.bak.
  3. Temporarily move the test module-info.class file (compiled in step 1) in place of the main one (saved in step 2).
  4. Temporarily rename the test module-info.java file as module-info.java.bak (otherwise the compiler seems to get confused).
  5. Compile all test classes except the test module-info.java file which has been compiled at step 1.
  6. Restore all *.bak files to their original location.

A shutdown hook is registered for executing step 6 even if the user interrupted the compilation with [Ctrl-C]. Above hack works, but is obviously fragile and should be used in last resort only. The preferred approach is to use options formally supported by the JDK such as --add-reads. Their use should be easier with this new plugin than with Maven 3, since Maven 4 automatizes some of those options.

Compiler options validation

The way to handle compiler options has been modified. Previously, the Maven plugin validated some options before to pass them to the compiler. For example, if the <debuglevel> value contains anything else than lines, vars or source, the plugin raised an error. The intend was to provide more informative message. But in the javax.tools.JavaCompiler interface, there is an API telling us whether an option is supported. Therefor, the new plugin version first asks to the compiler whether the option is supported, and only if the compiler said "no", the validation is performed for producing the error message. Consequently, if the compiler claims to support the -g:foo option, then the plugin will no longer block the use of the foo value in <debuglevel> even if the plugin does not know that value.

Miscellaneous

when the plugin proposes a code snippet (e.g. for specifying the target Java release), the Maven compiler plugin version shown in the snippet is fetched from META-INF/MANIFEST.MF instead of META-INF/maven/org.apache.maven.plugins/maven-compiler-plugin/pom.properties.

Changes in tests

This section lists some noticeable changes in the tests.

Changes in POM parameters

Moved or added the following configuration parameters in the pom.xml files of some tests:

  • The <outputDirectory> and <testOutputDirectory> parameters declared under <configuration> moved to <build>, because those properties are read-only in the configuration.
  • Many <source> and <target> parameters have been either removed or replaced by <release>.
  • For some tests using a non-modular JAR is a modular project, <type>modular-jar</type> has been added in the dependency declaration.

Changes to met new assumptions

The plugin incremental compilation algorithm depends on the convention that Java source files are located in directories of the same name as their package names, with the . separator replaced by path separator (/ or \). This is a very common convention, but not strictly required by the Java compiler. For example, if the src/main/java/MyCode.java file contains the package foo statement, the compiled class will be located in target/classes/foo/MyCode.class — note the foo additional directory. In such case, the incremental build algorithm will not track correctly the changes. The following tests have been made compliant with the convention for allowing the algorithm to work:

  • mcompiler-182 in integration tests.

Note that due to MCOMPILER-209, the old algorithm was compiling everything without really detecting change. So this issue is maybe not really a regression. To reproduce the old behavior, users can just disable the incremental compilation.

Removed tests

JUnit tests

Removed the following directories and associated test methods:

  • compiler-one-output-file-test2 because it was redundant with compiler-one-output-file-test.

The only difference was the addition of include/exclude filters, but that difference had no effect because the compiler mock used in this test was ignoring all sources anyway. This test has been replaced by compiler-modular-project.

Integration tests

The tests in the following directories were already disabled and have been removed:

  • MCOMPILER-197 because it ran only on Java 8 while the build now requires Java 17.
  • groovy-project-with-new-plexus-compiler because it ran only on Java 8 and the plexus compiler has been removed.

The tests in the following directores are not supported anymore and have been removed:

  • release-without-profile because the plugin no longer try to chose automatically which parameters to use between --source and --release. This is justified by the fact that the plugin cannot run on Java 8.
  • release-without-profile-fork for the same reason as above.

Other aspects that are rewritten include incremental build mechanism
and how the overwriting of `module-info.java` in tests is handled.
For details and impact on users, see pull request description at
apache#271
@desruisseaux desruisseaux mentioned this pull request Oct 29, 2024
@gnodet gnodet self-requested a review October 31, 2024 07:06
@gnodet
Copy link
Contributor

gnodet commented Nov 4, 2024

Fwiw, the source and release config parameters are inherited from:
https://github.com/apache/maven-apache-parent/blob/master/pom.xml#L100-L101
and
https://github.com/apache/maven-apache-parent/blob/master/pom.xml#L528

One possibility would be to nullify the source and release inside the jdk9+ profile.

@desruisseaux
Copy link
Contributor Author

Would a jdk9+ profile be useful if Maven 4 cannot run on JDK 8 anyway? Someone pointed out the case of toolchain, but that case would not be covered by the profile, isn't it?

Alternatively, should the default value be simply removed? I remember a discussion years ago about having a stable default value. But since the Maven default also need to be upgraded from times to times, if a user didn't specified the target version, aren't we basically replacing his dependency to the Java version installed on his machine by a dependency to the Maven version installed on his machine?

@gnodet
Copy link
Contributor

gnodet commented Nov 4, 2024

The parent POM must not be the only location where the defaults are set, as in the case of the IT, it does not inherit from the apache POM. So either you added the null values as a safe belt to all ITs, or we need to find another source.

Would a jdk9+ profile be useful if Maven 4 cannot run on JDK 8 anyway? Someone pointed out the case of toolchain, but that case would not be covered by the profile, isn't it?

The problem is that the apache-parent POM may still be used with Maven 3.
But at some point, we'll probably upgrade it to a higher JDK or Maven 4 requirement, but not sure when...
And using toolchains, Maven 4 can still build JDK 8 projects...

Alternatively, should the default value be simply removed? I remember a discussion years ago about having a stable default value. But since the Maven default also need to be upgraded from times to times, if a user didn't specified the target version, aren't we basically replacing his dependency to the Java version installed on his machine by a dependency to the Maven version installed on his machine?

For compatibility, I think it's safer to remove those when using JDK 9+ only for now.

@gnodet
Copy link
Contributor

gnodet commented Nov 4, 2024

Fwiw, apache/maven#1865 should fix the MCOMPILER-346 IT.

@desruisseaux
Copy link
Contributor Author

So either you added the null values as a safe belt to all ITs, or we need to find another source.

I added these null values because otherwise, javac complained that both -source and -release were set. But I have not been able to identify from where the integration tests inherited these values.

Fwiw, apache/maven#1865 should fix the MCOMPILER-346 IT.

Thanks! I will test at the first opportunity, maybe tomorrow.

@desruisseaux
Copy link
Contributor Author

Thanks, all integration tests pass now (tested on Java 17 and 23). Does the plugin need to wait for a Maven 4.0.0-beta-6 release, or is it okay to go forward as things stand now?

@gnodet
Copy link
Contributor

gnodet commented Nov 5, 2024

Thanks, all integration tests pass now (tested on Java 17 and 23). Does the plugin need to wait for a Maven 4.0.0-beta-6 release, or is it okay to go forward as things stand now?

We could disable the failing IT until beta-6 is out. And raise a PR to re-establish the test to not forget it.

It will be re-enabled after Maven 4.0.0-beta-6 release.

apache/maven#1865
@desruisseaux
Copy link
Contributor Author

Done. With MCOMPILER-346 disabled, all integration tests pass (Java 17 and 23).

The pom.xml still have a dependency to version 4.0.0-beta-2-SNAPSHOT or maven-plugin-testing-harness. The project does not build with 4.0.0-beta-1.

@desruisseaux
Copy link
Contributor Author

There is a test failure on Windows in MCOMPILER-192, which I suspect come from the / in the following:

<includes>
  <include>dummy/*.java</include>
  <include>dummy/license.txt</include>
</includes>

The code was supposed to translate / to \ on Windows, but I guess that I missed something. I will check at the first opportunity.

…syntax.

It appears that the GLOB matcher expects `/` even on Windows.
@desruisseaux
Copy link
Contributor Author

Fixed (I was replacing / by \ on Windows, but it appears that we shall not do that when using java.nio.file.PathMatcher with GLOB syntax). The tests passed on all platforms.

…tead of returning the Boolean `false` value.

Adjust what is the cause and what is the suppressed exception.
@gnodet
Copy link
Contributor

gnodet commented Nov 14, 2024

@desruisseaux the plugin has been released
could you patch with the following:

diff --git a/pom.xml b/pom.xml
index 51acb63..bacbec8 100644
--- a/pom.xml
+++ b/pom.xml
@@ -92,7 +92,7 @@ under the License.
     <junit4Version>4.13.2</junit4Version>
     <junitVersion>5.10.1</junitVersion>
     <mockitoVersion>5.12.0</mockitoVersion>
-    <mavenPluginTestingHarnessVersion>4.0.0-beta-2-SNAPSHOT</mavenPluginTestingHarnessVersion>
+    <mavenPluginTestingHarnessVersion>4.0.0-beta-2</mavenPluginTestingHarnessVersion>
     <plexusCompilerVersion>2.15.0</plexusCompilerVersion>
     <plexusJavaVersion>1.2.0</plexusJavaVersion>
     <sisuPlexusVersion>0.9.0.M2</sisuPlexusVersion>

@desruisseaux
Copy link
Contributor Author

desruisseaux commented Nov 14, 2024

Done. The CI job failed, but the cause seems a temporary network problem (a "GOAWAY received"):

org.apache.maven.lifecycle.LifecycleExecutionException: (…snip…)
  Could not transfer artifact org.eclipse.sisu:org.eclipse.sisu.plexus:jar:0.9.0.M3
  from/to central (https://repo.maven.apache.org/maven2): closed
    at org.apache.maven.lifecycle.internal.LifecycleDependencyResolver.getDependencies(LifecycleDependencyResolver.java:279)
    at org.apache.maven.lifecycle.internal.LifecycleDependencyResolver.resolveProjectArtifacts(LifecycleDependencyResolver.java:208)
    (…snip…)
Caused by: java.io.IOException: /10.1.0.88:54412: GOAWAY received
    at jdk.internal.net.http.Http2Connection.handleGoAway(Http2Connection.java:1210)
    (…snip…)

I verified that the integration tests passed on my machine (Java 23, Linux Fedora).

@gnodet
Copy link
Contributor

gnodet commented Nov 14, 2024

@desruisseaux anything that needs to be done ? Or should I merge this PR now ?

@desruisseaux
Copy link
Contributor Author

I suggest to merge if you don't mind. The first failure today was a network problem, and the two other failures seem random (happening in different OS + Java combinations). I have no idea at this time what could be the cause. If it happens to be an issue in the code, I hope that feedback would allow us to fix it.

@gnodet
Copy link
Contributor

gnodet commented Nov 14, 2024

I suggest to merge if you don't mind. The first failure today was a network problem, and the two other failures seem random (happening in different OS + Java combinations). I have no idea at this time what could be the cause. If it happens to be an issue in the code, I hope that feedback would allow us to fix it.

We definitely have intermittent issues, so I really think the code is not a problem here.

@gnodet gnodet merged commit 46b207b into apache:master Nov 14, 2024
26 checks passed
@desruisseaux
Copy link
Contributor Author

Thanks!

@desruisseaux desruisseaux deleted the JPMS branch November 15, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants