Copilot commented on code in PR #1091: URL: https://github.com/apache/maven-compiler-plugin/pull/1091#discussion_r3669119481
########## src/main/java/org/apache/maven/plugin/compiler/CompilationOutputRegistry.java: ########## @@ -0,0 +1,141 @@ +/* + * 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 org.apache.maven.plugin.compiler; + +import java.io.File; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; + +import org.codehaus.plexus.compiler.util.scan.InclusionScanException; +import org.codehaus.plexus.compiler.util.scan.mapping.SourceMapping; + +/** + * Tracks output files processed by compiler executions for the current project and Maven session. + * + * <p>Multiple compiler executions may share an output directory while using different compiler options. + * {@link org.apache.maven.shared.incremental.IncrementalBuildHelper} compares output file names before and after + * compilation, so it cannot detect an existing output overwritten by another execution. This registry lets a later + * execution detect that overlap without relying on file timestamps, whose precision varies between file systems.</p> + * + * <p>The registry is stored in Maven's plugin context, which is shared by all executions of this plugin for one + * project and discarded after the Maven session.</p> + */ +final class CompilationOutputRegistry { + private static final String KEY = CompilationOutputRegistry.class.getName() + ".compiledOutputs"; + + /** + * Prevents instantiation. + */ + private CompilationOutputRegistry() {} + + /** + * Maps sources to normalized absolute output paths. + * + * @param mapping mapping from source paths to output paths + * @param outputDirectory compiler output directory + * @param sourceRoots configured source roots + * @param sources sources selected by the compiler execution + * @return expected output paths + * @throws InclusionScanException if a source cannot be mapped + */ + static Set<Path> mapOutputs( + SourceMapping mapping, File outputDirectory, List<String> sourceRoots, Set<File> sources) + throws InclusionScanException { + Set<Path> outputs = new HashSet<>(); + for (String sourceRoot : sourceRoots) { + Path root = Paths.get(sourceRoot).toAbsolutePath().normalize(); + for (File source : sources) { + Path path = source.toPath().toAbsolutePath().normalize(); + if (path.startsWith(root)) { + for (File output : mapping.getTargetFiles( + outputDirectory, root.relativize(path).toString())) { + outputs.add(output.toPath().toAbsolutePath().normalize()); + } + } + } + } Review Comment: A source can be mapped multiple times if `sourceRoots` contains nested/overlapping roots (e.g., `src/main` and `src/main/java`). In that case the code will generate outputs for multiple different relative paths, potentially introducing false overlap detections and unnecessary recompiles. A robust fix is to map each source against exactly one root (typically the longest matching root) and only call `getTargetFiles` once per source. ########## src/it/MCOMPILER-578/verify.groovy: ########## @@ -0,0 +1,30 @@ +/* + * 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. + */ + +def exampleClass = new File( basedir, 'target/classes/org/example/Example.class' ) +assert exampleClass.isFile() +def exampleMajorVersion = exampleClass.bytes[7] & 0xFF +// major_version: 52 = Java 8, from the base-compile execution. +assert exampleMajorVersion == 52 + +def moduleInfoClass = new File( basedir, 'target/classes/module-info.class' ) +assert moduleInfoClass.isFile() +def moduleInfoMajorVersion = moduleInfoClass.bytes[7] & 0xFF +// major_version: 55 = Java 11, from the base-modules-compile execution. +assert moduleInfoMajorVersion == 55 Review Comment: Classfile `major_version` is a u2 at offsets 6..7. Reading only `bytes[7]` works for current versions but is technically incomplete. Consider decoding `(bytes[6] << 8) | bytes[7]` (with proper unsigned handling) to match the classfile spec and avoid future edge cases. ########## src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java: ########## @@ -1569,6 +1586,54 @@ private Set<File> computeStaleSources( return staleSources; } + /** + * Maps selected sources to their expected compiler outputs. + * + * @param compilerConfiguration the compiler configuration + * @param compiler the selected compiler + * @param sources sources selected by the current execution + * @return normalized absolute output paths + */ + private Set<Path> getOutputPaths(CompilerConfiguration compilerConfiguration, Compiler compiler, Set<File> sources) + throws CompilerException, MojoExecutionException { + SourceMapping mapping = getSourceMapping(compilerConfiguration, compiler); + + File outputDirectory = + compiler.getCompilerOutputStyle() == CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES + ? buildDirectory + : getOutputDirectory(); + try { + return CompilationOutputRegistry.mapOutputs(mapping, outputDirectory, getCompileSourceRoots(), sources); + } catch (InclusionScanException e) { + throw new MojoExecutionException("Error mapping sources to their outputs.", e); + } + } + + /** + * Checks whether a different compiler execution last processed an expected output. + * + * @param compilerExecution the current execution + * @param outputs expected outputs of the current execution + * @return whether an output overlaps with another execution + */ + private boolean hasPreviouslyCompiledOutput(String compilerExecution, Set<Path> outputs) { + Optional<Path> output = CompilationOutputRegistry.find(getPluginContext(), compilerExecution, outputs); + if (output.isPresent() && (getLog().isDebugEnabled() || showCompilationChanges)) { + getLog().info("\tOutput from another compiler execution: " + output.get()); + } + return output.isPresent(); + } Review Comment: This logs at INFO whenever DEBUG is enabled (even if `showCompilationChanges` is false). That can unexpectedly add INFO-level noise for users running with `-X`. Consider logging at INFO only when `showCompilationChanges` is true, and at DEBUG when only `getLog().isDebugEnabled()` is true. ########## src/test/java/org/apache/maven/plugin/compiler/CompilationOutputRegistryTest.java: ########## @@ -0,0 +1,84 @@ +/* + * 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 org.apache.maven.plugin.compiler; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +class CompilationOutputRegistryTest { + private static final String FIRST_EXECUTION = "compile@first"; + + private static final String SECOND_EXECUTION = "compile@second"; + + private static final Path OUTPUT = Paths.get("target/classes/Example.class"); + + private static final Path OTHER_OUTPUT = Paths.get("target/classes/Other.class"); Review Comment: Production usage stores normalized absolute output paths (via `CompilationOutputRegistry.mapOutputs(...)`). Using relative paths in these tests can mask path-normalization/equality issues (absolute vs relative `Path` keys won’t match). Consider using `toAbsolutePath().normalize()` here (or adding a targeted test for absolute-path behavior) so the test data matches real registry keys. ########## src/it/MCOMPILER-578/verify.groovy: ########## @@ -0,0 +1,30 @@ +/* + * 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. + */ + +def exampleClass = new File( basedir, 'target/classes/org/example/Example.class' ) +assert exampleClass.isFile() +def exampleMajorVersion = exampleClass.bytes[7] & 0xFF +// major_version: 52 = Java 8, from the base-compile execution. +assert exampleMajorVersion == 52 Review Comment: Classfile `major_version` is a u2 at offsets 6..7. Reading only `bytes[7]` works for current versions but is technically incomplete. Consider decoding `(bytes[6] << 8) | bytes[7]` (with proper unsigned handling) to match the classfile spec and avoid future edge cases. -- 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]
