gnodet-bot commented on code in PR #1144: URL: https://github.com/apache/maven-compiler-plugin/pull/1144#discussion_r4176533959
########## src/main/java/org/apache/maven/plugin/compiler/incremental/AbiIncrementalBuild.java: ########## @@ -0,0 +1,898 @@ +/* + * 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.incremental; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import java.util.stream.Collectors; + +/** + * ABI-fingerprint-driven incremental build engine, designed for embedding + * in maven-compiler-plugin alongside the existing timestamp-based + * {@code IncrementalBuild}. + * + * <p>The plugin drives compilation; this class determines <em>what</em> to + * compile and performs post-compilation bytecode analysis to build the + * dependency graph and ABI fingerprints. Typical usage: + * + * <pre>{@code + * var abi = new AbiIncrementalBuild(outputDir); + * abi.setClasspathEntries(classpath); + * abi.setReactorModulePaths(reactorModules); + * abi.setProcessorPath(processorPath); + * abi.setConfigHash(configHash); + * + * Set<Path> toCompile = abi.initialize(allSourceFiles); + * + * while (!toCompile.isEmpty()) { + * compiler.compile(toCompile); // any compiler, any mode + * toCompile = abi.processCompiledClasses(toCompile); + * } + * + * abi.finish(); + * }</pre> + * + * <p>After each compilation pass, {@link #processCompiledClasses(Set)} scans + * the freshly produced {@code .class} files, updates the dependency graph and + * ABI fingerprints, and returns any additional files that must be compiled in + * the next pass (cascade due to ABI changes, or newly discovered dependencies). + * The loop converges in at most 2–3 passes in practice. + * + * <p>The engine persists its state as {@code .abi-incremental-state} in the + * {@code target/maven-status/maven-compiler-plugin/<outputDirName>/} directory (outside the class + * output directory so it is not packaged into JARs) and writes an {@link AbiManifest} + * ({@code .abi-fingerprints}) in the build directory for downstream reactor modules. + * + * @see IncrementalState + */ +public class AbiIncrementalBuild { + + /** Prefix used to distinguish module-info entries from regular type entries in the state. */ + static final String MODULE_PREFIX = "module:"; + + private final Path outputDir; + private final Path buildDir; + private final Path stateFile; + private List<Path> classpathEntries; + private Set<Path> reactorModulePaths; + private List<Path> processorPath; + private ProcessorClassification processorClassification; + + private IncrementalState previousState; + private IncrementalState state; + private Map<String, String> sourceHashes; + private Map<String, Long> sourceMtimes; + private List<Path> allSourceFiles; + private Set<String> allCompiled; + private boolean fullBuild; + private boolean useModulePrefixedPaths; + private String configHash = ""; + private String rebuildCause; + private int totalSources; + /** Source files compiled in previous rounds of the current build (for loop detection). */ + private Set<String> compiledInPreviousRounds; + /** Lazily populated on full builds; maps each output class file to its simple top-level class name. */ + private java.util.Map<Path, String> outputClassIndex; Review Comment: 💡 **Dead code:** `compiledInPreviousRounds` (lines 97, 209–210) is initialized in `processCompiledClasses()` but never read anywhere. The loop detection it was presumably intended for is instead handled by the `maxRounds` safety bound in `ToolExecutor`. Either remove the field entirely or wire it into a duplicate-detection check in `processCompiledClasses()`. If the intent was to detect when the cascade returns files already compiled in a previous round (a stronger invariant than the brute-force `maxRounds` cap), the check would look something like: ```java // After computing additionalFiles additionalFiles.removeIf(f -> compiledInPreviousRounds.contains(f.toString())); compiledInPreviousRounds.addAll(compiledSourceFiles.stream().map(Path::toString).toList()); ``` But since `allCompiled` already serves this purpose (line 273: `!allCompiled.contains(sf)`), the field is genuinely redundant. ########## src/test/java/org/apache/maven/plugin/compiler/incremental/CompilerTestHelper.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.incremental; + +import javax.tools.JavaCompiler; +import javax.tools.StandardLocation; +import javax.tools.ToolProvider; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; + +/** + * Shared helper for incremental compilation tests. + */ +class CompilerTestHelper { + + static void writeSource(Path sourceDir, String packageName, String className, String source) throws IOException { + Path packageDir = sourceDir.resolve(packageName.replace('.', '/')); + Files.createDirectories(packageDir); + Files.writeString(packageDir.resolve(className + ".java"), source); + } + + /** + * Compiles all {@code .java} files under {@code sourceDir} into {@code outputDir} + * and returns the output directory. No ABI analysis — raw javac only. + */ + static Map<String, SourceFileAnalysis> compileAndAnalyze(Path sourceDir, Path outputDir) throws IOException { + Files.createDirectories(outputDir); + List<Path> sourceFiles; + try (Stream<Path> walk = Files.walk(sourceDir)) { + sourceFiles = + walk.filter(p -> p.toString().endsWith(".java")).sorted().toList(); + } + + JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); + try (var fm = compiler.getStandardFileManager(null, null, null)) { + fm.setLocation(StandardLocation.CLASS_OUTPUT, List.of(outputDir.toFile())); + var units = fm.getJavaFileObjectsFromPaths(sourceFiles); + var task = compiler.getTask(null, fm, null, null, null, units); + + if (!task.call()) { + throw new RuntimeException("Compilation failed"); + } + + // Build a minimal SourceFileAnalysis map from bytecode (no dep graph needed here) + Map<String, SourceFileAnalysis> results = new java.util.LinkedHashMap<>(); + for (Path sf : sourceFiles) { + try (Stream<Path> walk = Files.walk(outputDir)) { + walk.filter(p -> p.toString().endsWith(".class")).forEach(cf -> { + try { + var analysis = BytecodeAnalyzer.analyze(cf); + var sfa = new SourceFileAnalysis( + analysis.className(), + sf.toString(), + analysis.signatureTypes(), + analysis.implementationTypes(), + analysis.abiFingerprint(), + analysis.abiCanonical(), + analysis.annotationTypes(), + analysis.moduleName()); + results.put(analysis.className(), sfa); + } catch (IOException e) { + // best effort + } + }); + } catch (IOException e) { + // best effort + } + } Review Comment: 💡 **Misleading source-file attribution:** The outer loop iterates over all source files, and for each one, the inner loop walks **all** `.class` files in the output directory. This means every class file gets analyzed `N` times (once per source file), and each analysis is stored with `sf.toString()` pointing to whichever source file is currently being iterated. This "works" only because `results.put(analysis.className(), sfa)` uses the class name as key — the last iteration wins, and the source file ends up being the last `sf` in the sorted list. For tests where the actual source attribution doesn't matter (the test only checks fingerprints/deps, not source file paths), this is harmless. But it's accidentally correct — if any test ever checks `sfa.sourceFile()`, it would get the wrong answer. The fix is to walk class files once and use the `SourceFile` attribute (which `BytecodeAnalyzer` already extracts) to match classes to sources: ```suggestion // Build a minimal SourceFileAnalysis map from bytecode (no dep graph needed here) Map<String, SourceFileAnalysis> results = new java.util.LinkedHashMap<>(); try (Stream<Path> walk = Files.walk(outputDir)) { walk.filter(p -> p.toString().endsWith(".class")).forEach(cf -> { try { var analysis = BytecodeAnalyzer.analyze(cf); // Match to source file by SourceFile attribute String sfName = analysis.sourceFileName(); // e.g. "Model.java" Path matchedSource = sourceFiles.stream() .filter(sf -> sf.getFileName().toString().equals(sfName)) .findFirst() .orElse(sourceFiles.get(0)); var sfa = new SourceFileAnalysis( analysis.className(), matchedSource.toString(), analysis.signatureTypes(), analysis.implementationTypes(), analysis.abiFingerprint(), analysis.abiCanonical(), analysis.annotationTypes(), analysis.moduleName()); results.put(analysis.className(), sfa); } catch (IOException e) { // best effort } }); } catch (IOException e) { // best effort } ``` -- 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]
