Bukama commented on code in PR #987: URL: https://github.com/apache/maven-compiler-plugin/pull/987#discussion_r2466965682
########## src/it/processor-type/annotation-processor/src/main/java/processor/SimpleAnnotationProcessor.java: ########## @@ -0,0 +1,69 @@ +/* + * 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. + */ +package processor; + +import javax.annotation.processing.AbstractProcessor; +import javax.annotation.processing.RoundEnvironment; +import javax.annotation.processing.SupportedAnnotationTypes; +import javax.annotation.processing.SupportedSourceVersion; +import javax.lang.model.SourceVersion; +import javax.lang.model.element.Element; +import javax.lang.model.element.Name; +import javax.lang.model.element.TypeElement; +import javax.tools.FileObject; +import javax.tools.StandardLocation; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.io.Writer; +import java.util.Set; + +@SupportedSourceVersion(SourceVersion.RELEASE_17) +@SupportedAnnotationTypes("user.SimpleAnnotation") +public class SimpleAnnotationProcessor extends AbstractProcessor { + public SimpleAnnotationProcessor() {} + + @Override + public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) { + // Verifies that transitive dependencies worked. + // TODO: pending a fix in Maven core. Review Comment: Please create an issue or this PR should be changed to PR to don't miss this. ########## src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java: ########## @@ -1451,13 +1458,37 @@ private boolean isVersionEqualOrNewer(String sourceVersion) { return supportedVersion.compareTo(requested) >= 0; } + /** + * Returns whether the given collection is null or empty, ignoring spaces. + * This is a convenience for a frequent check, and also for clarity. + */ + private static boolean isNullOrEmpty(String c) { Review Comment: The method checks for `isBlank` and not `isEmpty` which is a difference ########## 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'm with @bmarwell This is very hard to read :( ########## 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 would rename the file to have the changes between v3 and v4 in name. That makes it more clear and pretends that other information are added to a "readme". ########## 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: > Actually, I'm lazily mixing two works in this same pull request. then why all those renames :D :D :D 💋 ########## src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java: ########## @@ -1608,42 +1634,25 @@ 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 strict whether to be conservative if the current Java version is older than 23 + * @param dependencyTypes the type of dependencies, for checking if any of them is a processor path * * @see #incrementalCompilation */ - private boolean hasAnnotationProcessor(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`. - } + private boolean hasAnnotationProcessor(final Set<PathType> dependencyTypes) { + if (isNullOrEmpty(proc)) { /* * 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; - } - var allowedTypes = EnumSet.of(JavaPathType.PROCESSOR_CLASSES, JavaPathType.PROCESSOR_MODULES); - DependencyResolverResult dependencies = resolver.resolve(DependencyResolverRequest.builder() - .session(session) - .project(project) - .requestType(DependencyResolverRequest.RequestType.COLLECT) - .pathScope(compileScope) - .pathTypeFilter(allowedTypes) - .build()); - - return !dependencies.getDependencies().isEmpty(); + if (isVersionEqualOrNewer("RELEASE_23")) { Review Comment: Can RELEASE_23 be replaced by a constant to avoid the literal here? (I was hoping that there are some in the JDK, but according to https://docs.oracle.com/en/java/javase/17/docs/api/constant-values.html there are not) -- 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]
