gnodet-bot commented on code in PR #1154: URL: https://github.com/apache/maven-compiler-plugin/pull/1154#discussion_r4181131710
########## src/it/abi-incremental-basic/pom.xml: ########## @@ -0,0 +1,66 @@ +<?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/maven-v4_0_0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <groupId>org.apache.maven.plugins.compiler.it</groupId> + <artifactId>abi-incremental-basic</artifactId> + <version>1.0-SNAPSHOT</version> + + <description>IT: ABI-based incremental compilation — body-only change should only recompile the changed file.</description> + + <properties> + <maven.compiler.release>17</maven.compiler.release> + <maven.compiler.incrementalStrategy>abi</maven.compiler.incrementalStrategy> Review Comment: 🔴 **Critical:** This sets `incrementalStrategy` to `abi`, but `AbstractCompilerMojo.compile()` only checks for `"graph"` (line 1427). This IT will silently fall through to the timestamp strategy and never exercise the new dependency-graph code path. The same issue exists in `abi-incremental-cascade/pom.xml`. ```suggestion <maven.compiler.incrementalStrategy>graph</maven.compiler.incrementalStrategy> ``` ########## src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java: ########## @@ -915,6 +919,216 @@ private static boolean removeFirsts(Deque<Path> paths, Integer count) { } } + /** + * Compiles using the dependency-graph incremental strategy. This method handles the full + * lifecycle: determining what to compile, running javac, cascading on changed classes, + * and persisting state. + * + * @param compiler the compiler + * @param configuration the options to give to the Java compiler + * @param mojo the MOJO for configuration access + * @throws IOException if an error occurred while reading or writing a file + * @throws MojoException if the compilation failed + */ + void compileWithAbiIncremental(JavaCompiler compiler, final Options configuration, final AbstractCompilerMojo mojo) + throws IOException { + var abiBuild = new AbiIncrementalBuild(outputDirectory); + + // Collect annotation processor path for processor classification + var processorPaths = new ArrayList<Path>(); + for (var entry : dependencies.entrySet()) { + if (entry.getKey() instanceof JavaPathType type) { + var location = type.location(); + if (location.isPresent() + && (location.get() == StandardLocation.ANNOTATION_PROCESSOR_PATH + || location.get() == StandardLocation.ANNOTATION_PROCESSOR_MODULE_PATH)) { + processorPaths.addAll(entry.getValue()); + } + } + } + if (!processorPaths.isEmpty()) { + abiBuild.setProcessorPath(processorPaths); + } + + // Hash module-info-patch.maven files for config change detection + abiBuild.setConfigHash(computeConfigHash(configuration)); + + // Collect all source file paths + var allSourcePaths = new ArrayList<Path>(); + for (SourceFile sf : sourceFiles) { + allSourcePaths.add(sf.file); + } + + Set<Path> toCompile = abiBuild.initialize(allSourcePaths); + if (toCompile.isEmpty()) { + logger.info("Nothing to compile - all classes are up to date (graph strategy)."); + abiBuild.finish(); + return; + } + + logger.info( + abiBuild.isFullBuild() + ? "Compiling " + toCompile.size() + " source file(s) (ABI: full build)." + : "Compiling " + toCompile.size() + " source file(s) (ABI: incremental)."); + if (mojo.showCompilationChanges && abiBuild.getRebuildCause() != null) { + logger.info("Rebuild cause: " + abiBuild.getRebuildCause()); + for (Path f : toCompile) { + logger.info(" " + f); + } + } + + var originalSourceFiles = new ArrayList<>(sourceFiles); + boolean success = true; + // Safety bound: the compile set is monotonically growing (bounded by total source count). + // If a bug causes processCompiledClasses to return files already compiled, this prevents + // an infinite loop. In practice this limit should never be reached. + int maxRounds = originalSourceFiles.size() + 1; + int rounds = 0; + + try { + while (!toCompile.isEmpty()) { + if (++rounds > maxRounds) { + throw new IllegalStateException("ABI cascade loop did not converge after " + maxRounds + + " rounds — " + "possible dependency cycle or bug in processCompiledClasses()"); + } + Set<Path> compileSet = toCompile; + sourceFiles = originalSourceFiles.stream() + .filter(sf -> compileSet.contains(sf.file)) + .collect(Collectors.toList()); + + if (sourceFiles.isEmpty()) { + break; + } + + var compilerOutput = new StringWriter(); + success = compileWithAbiAnalyzer(compiler, configuration, compilerOutput, abiBuild); + String output = compilerOutput.toString(); + if (!output.isBlank()) { + logger.warn(output); + } + if (!success) { + break; + } + + toCompile = abiBuild.processCompiledClasses(compileSet); + if (!toCompile.isEmpty()) { + logger.info("ABI cascade: recompiling " + toCompile.size() + " additional file(s)."); + if (mojo.showCompilationChanges) { + for (Path f : toCompile) { + logger.info(" " + f); + } + } + } + } + } finally { + sourceFiles = originalSourceFiles; + } + + if (success) { + abiBuild.finish(); + logger.info("Compiled " + abiBuild.compiledCount() + " file(s), " + abiBuild.unchangedCount() + + " unchanged (graph strategy)."); + } else { + abiBuild.invalidate(); + throw new CompilationFailureException("Compilation failed (graph incremental strategy)."); + } + } + + private String computeConfigHash(Options configuration) { + var digest = new StringBuilder(); + + // Include compiler options in the config hash so changes to -source/-target/-release + // etc. trigger a full rebuild under the graph strategy. + String optionsRepr = String.join("|", configuration.options); + digest.append("opts:").append(optionsRepr).append(';'); + + for (SourceDirectory source : sourceDirectories) { + Path patchFile = source.root.resolve(ModuleInfoPatch.FILENAME); + if (Files.isRegularFile(patchFile)) { + try { + byte[] content = Files.readAllBytes(patchFile); + digest.append(patchFile) + .append(':') + .append(Sha256.hash(content)) + .append(';'); + } catch (IOException e) { + digest.append(patchFile).append(":unreadable;"); + } + } + } + if (digest.isEmpty()) { Review Comment: 💡 **Dead code:** `digest.isEmpty()` is never true — the method always appends `"opts:"` + options + `";"` on lines 1042–1043, so the `StringBuilder` is never empty. The early-return will never fire. Not a bug (the hash is still computed correctly), but the dead branch is misleading. ########## src/it/abi-incremental-cascade/pom.xml: ########## @@ -0,0 +1,67 @@ +<?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/maven-v4_0_0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <groupId>org.apache.maven.plugins.compiler.it</groupId> + <artifactId>abi-incremental-cascade</artifactId> + <version>1.0-SNAPSHOT</version> + + <description>IT: ABI change should cascade to consumers (recompile Service when Model API changes).</description> + + <properties> + <maven.compiler.release>17</maven.compiler.release> + <maven.compiler.incrementalStrategy>abi</maven.compiler.incrementalStrategy> Review Comment: 🔴 **Critical:** Same as above — `abi` doesn't match the `"graph"` check in `AbstractCompilerMojo`. This IT won't test the graph strategy. ```suggestion <maven.compiler.incrementalStrategy>graph</maven.compiler.incrementalStrategy> ``` ########## pom.xml: ########## @@ -85,7 +85,7 @@ under the License. <mavenVersion>4.0.0-rc-7</mavenVersion> <asmVersion>9.10.1</asmVersion> - <mockitoVersion>5.24.0</mockitoVersion> + <mockitoVersion>5.23.0</mockitoVersion> Review Comment: ⚠️ **Warning:** Mockito downgraded from 5.24.0 → 5.23.0 without explanation. Is this intentional (e.g. compatibility issue with the MRJAR test setup) or accidental? If intentional, a comment or commit message note would help. ########## src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java: ########## @@ -915,6 +919,216 @@ private static boolean removeFirsts(Deque<Path> paths, Integer count) { } } + /** + * Compiles using the dependency-graph incremental strategy. This method handles the full + * lifecycle: determining what to compile, running javac, cascading on changed classes, + * and persisting state. + * + * @param compiler the compiler + * @param configuration the options to give to the Java compiler + * @param mojo the MOJO for configuration access + * @throws IOException if an error occurred while reading or writing a file + * @throws MojoException if the compilation failed + */ + void compileWithAbiIncremental(JavaCompiler compiler, final Options configuration, final AbstractCompilerMojo mojo) + throws IOException { + var abiBuild = new AbiIncrementalBuild(outputDirectory); + + // Collect annotation processor path for processor classification + var processorPaths = new ArrayList<Path>(); + for (var entry : dependencies.entrySet()) { + if (entry.getKey() instanceof JavaPathType type) { + var location = type.location(); + if (location.isPresent() + && (location.get() == StandardLocation.ANNOTATION_PROCESSOR_PATH + || location.get() == StandardLocation.ANNOTATION_PROCESSOR_MODULE_PATH)) { + processorPaths.addAll(entry.getValue()); + } + } + } + if (!processorPaths.isEmpty()) { + abiBuild.setProcessorPath(processorPaths); + } + + // Hash module-info-patch.maven files for config change detection + abiBuild.setConfigHash(computeConfigHash(configuration)); + + // Collect all source file paths + var allSourcePaths = new ArrayList<Path>(); + for (SourceFile sf : sourceFiles) { + allSourcePaths.add(sf.file); + } + + Set<Path> toCompile = abiBuild.initialize(allSourcePaths); + if (toCompile.isEmpty()) { + logger.info("Nothing to compile - all classes are up to date (graph strategy)."); + abiBuild.finish(); + return; + } + + logger.info( + abiBuild.isFullBuild() + ? "Compiling " + toCompile.size() + " source file(s) (ABI: full build)." + : "Compiling " + toCompile.size() + " source file(s) (ABI: incremental)."); + if (mojo.showCompilationChanges && abiBuild.getRebuildCause() != null) { + logger.info("Rebuild cause: " + abiBuild.getRebuildCause()); + for (Path f : toCompile) { + logger.info(" " + f); + } + } + + var originalSourceFiles = new ArrayList<>(sourceFiles); + boolean success = true; + // Safety bound: the compile set is monotonically growing (bounded by total source count). + // If a bug causes processCompiledClasses to return files already compiled, this prevents + // an infinite loop. In practice this limit should never be reached. + int maxRounds = originalSourceFiles.size() + 1; + int rounds = 0; + + try { + while (!toCompile.isEmpty()) { + if (++rounds > maxRounds) { + throw new IllegalStateException("ABI cascade loop did not converge after " + maxRounds + + " rounds — " + "possible dependency cycle or bug in processCompiledClasses()"); + } + Set<Path> compileSet = toCompile; + sourceFiles = originalSourceFiles.stream() + .filter(sf -> compileSet.contains(sf.file)) + .collect(Collectors.toList()); + + if (sourceFiles.isEmpty()) { + break; + } + + var compilerOutput = new StringWriter(); + success = compileWithAbiAnalyzer(compiler, configuration, compilerOutput, abiBuild); + String output = compilerOutput.toString(); + if (!output.isBlank()) { + logger.warn(output); + } + if (!success) { + break; + } + + toCompile = abiBuild.processCompiledClasses(compileSet); + if (!toCompile.isEmpty()) { + logger.info("ABI cascade: recompiling " + toCompile.size() + " additional file(s)."); + if (mojo.showCompilationChanges) { + for (Path f : toCompile) { + logger.info(" " + f); + } + } + } + } + } finally { + sourceFiles = originalSourceFiles; + } + + if (success) { + abiBuild.finish(); + logger.info("Compiled " + abiBuild.compiledCount() + " file(s), " + abiBuild.unchangedCount() + + " unchanged (graph strategy)."); + } else { + abiBuild.invalidate(); + throw new CompilationFailureException("Compilation failed (graph incremental strategy)."); + } + } + + private String computeConfigHash(Options configuration) { + var digest = new StringBuilder(); + + // Include compiler options in the config hash so changes to -source/-target/-release + // etc. trigger a full rebuild under the graph strategy. + String optionsRepr = String.join("|", configuration.options); + digest.append("opts:").append(optionsRepr).append(';'); + + for (SourceDirectory source : sourceDirectories) { + Path patchFile = source.root.resolve(ModuleInfoPatch.FILENAME); + if (Files.isRegularFile(patchFile)) { + try { + byte[] content = Files.readAllBytes(patchFile); + digest.append(patchFile) + .append(':') + .append(Sha256.hash(content)) + .append(';'); + } catch (IOException e) { + digest.append(patchFile).append(":unreadable;"); + } + } + } + if (digest.isEmpty()) { + return ""; + } + return Sha256.hash(digest.toString()); + } + + /** + * Compiles sources with the ABI analyzer attached as a TaskListener. + */ + private boolean compileWithAbiAnalyzer( Review Comment: 💡 **Naming:** `compileWithAbiAnalyzer` implies bytecode analysis happens during compilation (e.g. via a `TaskListener`), but this method doesn't perform any analysis — it only calls `abiBuild.setUseModulePrefixedPaths()`. The actual bytecode analysis happens post-compilation in `processCompiledClasses`. Consider `compileForAbiIncremental` or `compileIncrementalRound` to better convey the method's role. -- 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]
