bmarwell commented on code in PR #987:
URL:
https://github.com/apache/maven-compiler-plugin/pull/987#discussion_r2465495047
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1559,11 +1568,6 @@ final DependencyResolverResult
resolveDependencies(boolean hasModuleDeclaration)
* Adds paths to the annotation processor dependencies. Paths are added to
the list associated
* to the {@link JavaPathType#PROCESSOR_CLASSES} entry of given map, which
should be modifiable.
*
- * <h4>Implementation note</h4>
- * We rely on the fact that {@link
org.apache.maven.impl.DefaultDependencyResolverResult} creates
- * modifiable instances of map and lists. This is a fragile assumption,
but this method is deprecated anyway
- * and may be removed in a future version.
- *
Review Comment:
Well spotted!
##########
src/it/processor-type/annotation-user/pom.xml:
##########
@@ -0,0 +1,62 @@
+<?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/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.maven.plugins.compiler.it</groupId>
+ <artifactId>processor-type</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>annotation-user</artifactId>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.maven.plugins.compiler.it</groupId>
+ <artifactId>annotation-processor</artifactId>
+ <version>1.0-SNAPSHOT</version>
+ <type>processor</type>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins.compiler.it</groupId>
+ <artifactId>annotation-verify-plugin</artifactId>
+ <version>1.0.0-SNAPSHOT</version>
+ <executions>
Review Comment:
I have never seen this before...
##########
src/it/README.md:
##########
@@ -0,0 +1,77 @@
+<!---
+ 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.
+-->
+
+This page contains notes about the integration tests in Maven Compiler Plugin
4.
+These tests can be executed with `mvn -P run-its verify`.
+
+# Changes compared to Maven Compiler Plugin 3
+This section describes noticeable changes in the tests of Maven Compiler
Plugin 4
+compared to the tests of Maven Compiler Plugin 3.
+The purpose of this section is to help the port of new tests added to the
Maven Compiler Plugin 3
+by documenting which tests to ignore and what to modify in new 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 in a modular project,
+ `<type>modular-jar</type>` has been added in the dependency declaration.
+
+## Changes to met new plugin 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](https://jira.codehaus.org/browse/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 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.
+* `MCOMPILER-190`, which has been replaced by `MCOMPILER-609`.
+ They are compilation tests using the Eclipse compiler, but the former test
depended on Nexus.
+ It has been replaced by a test that depends on `javax.tools`.
+
+
+## Removed 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`.
Review Comment:
I think no one will care in the future about this. This is appropriate for
this PR / issue to mention once, but for future use it is probably
uninteresting.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1610,38 +1614,26 @@ final void resolveProcessorPathEntries(Map<PathType,
List<Path>> addTo) throws M
* {@return whether an annotation processor seems to be present}
* This method is invoked if the user did not specified explicit
incremental compilation options.
*
+ * @param dependencyTypes the type of dependencies, for checking if any of
them is a processor path
* @param strict whether to be conservative if the current Java version is
older than 23
Review Comment:
Maybe not part of this PR, but we should really clarify what "strict" means
and what conservative will change in the build.
##########
src/it/README.md:
##########
@@ -0,0 +1,77 @@
+<!---
+ 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.
+-->
+
+This page contains notes about the integration tests in Maven Compiler Plugin
4.
+These tests can be executed with `mvn -P run-its verify`.
+
+# Changes compared to Maven Compiler Plugin 3
+This section describes noticeable changes in the tests of Maven Compiler
Plugin 4
+compared to the tests of Maven Compiler Plugin 3.
+The purpose of this section is to help the port of new tests added to the
Maven Compiler Plugin 3
+by documenting which tests to ignore and what to modify in new 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 in a modular project,
+ `<type>modular-jar</type>` has been added in the dependency declaration.
+
+## Changes to met new plugin 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](https://jira.codehaus.org/browse/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 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.
+* `MCOMPILER-190`, which has been replaced by `MCOMPILER-609`.
+ They are compilation tests using the Eclipse compiler, but the former test
depended on Nexus.
+ It has been replaced by a test that depends on `javax.tools`.
Review Comment:
I think no one will care in the future about this. This is appropriate for
this PR / issue to mention once, but for future use it is probably
uninteresting.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1610,38 +1614,26 @@ final void resolveProcessorPathEntries(Map<PathType,
List<Path>> addTo) throws M
* {@return whether an annotation processor seems to be present}
* This method is invoked if the user did not specified explicit
incremental compilation options.
*
+ * @param dependencyTypes the type of dependencies, for checking if any of
them is a processor path
* @param strict whether to be conservative if the current Java version is
older than 23
*
* @see #incrementalCompilation
*/
- private boolean hasAnnotationProcessor(final boolean strict) {
+ private boolean hasAnnotationProcessor(final Set<PathType>
dependencyTypes, final boolean strict) {
if ("none".equalsIgnoreCase(proc)) {
return false;
}
if (proc == null || proc.isBlank()) {
- if (strict && !isVersionEqualOrNewer("RELEASE_23")) {
- return true; // Before Java 23, default value of `-proc` was
`full`.
- }
/*
* If the `proc` parameter was not specified, its default value
depends on the Java version.
* It was "full" prior Java 23 and become "none if no other
processor option" since Java 23.
*/
- if (annotationProcessors == null || annotationProcessors.length ==
0) {
- if (annotationProcessorPaths == null ||
annotationProcessorPaths.isEmpty()) {
- DependencyResolver resolver =
session.getService(DependencyResolver.class);
- if (resolver == null) { // Null value happen during tests,
depending on the mock used.
- return false;
+ if (!strict || isVersionEqualOrNewer("RELEASE_23")) {
+ if (annotationProcessors == null ||
annotationProcessors.length == 0) {
+ if (annotationProcessorPaths == null ||
annotationProcessorPaths.isEmpty()) {
+ return
dependencyTypes.contains(JavaPathType.PROCESSOR_CLASSES)
+ ||
dependencyTypes.contains(JavaPathType.PROCESSOR_MODULES);
}
Review Comment:
I know it has been there in a similar way before, but this is really hard to
read. And instead of just adding comments, I (personally) would create at most
one or two boolean methods where the method name alone explains what this
if/if/if is all about.
--
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]
