gnodet-bot commented on code in PR #1154: URL: https://github.com/apache/maven-compiler-plugin/pull/1154#discussion_r4181217826
########## 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: 🔴 **Still not addressed (previous review):** This sets `incrementalStrategy` to `abi`, but `AbstractCompilerMojo.compile()` (line 1427) only checks for `"graph"`. This IT silently falls through to the timestamp strategy and never exercises the dependency-graph incremental code path. ```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: ⚠️ **Still not addressed (previous review):** 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 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()) { Review Comment: 💡 **Still not addressed (previous review):** `digest.isEmpty()` is never true — line 1043 always appends `"opts:"` + options + `";"`, so the `StringBuilder` is never empty. This early-return is dead code. Not a bug (the hash is still computed correctly), but the dead branch is misleading. ########## 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: 💡 **Still not addressed (previous review):** `compileWithAbiAnalyzer` implies bytecode analysis happens during compilation, but this method doesn't perform analysis — it just drives the compilation loop. The actual bytecode analysis happens post-compilation in `processCompiledClasses()`. Consider `compileForGraphIncremental` or `compileIncrementalRound` to better convey the method's role — and to be consistent with the `"graph"` strategy name. ########## 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: 🔴 **Still not addressed (previous review):** Same issue — `abi` doesn't match the `"graph"` dispatch in `AbstractCompilerMojo`. This IT won't test the graph strategy. ```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)."); Review Comment: 💡 **New finding:** These log messages say "ABI" but the user-facing parameter is `incrementalStrategy=graph`. Line 964 and 1033 correctly say "graph strategy", but lines 971-972 say "(ABI: full build)" / "(ABI: incremental)" and line 1015 says "ABI cascade". This inconsistency will confuse users trying to correlate log output with their configuration. Consider using "graph" consistently: ```suggestion ? "Compiling " + toCompile.size() + " source file(s) (graph: full build)." : "Compiling " + toCompile.size() + " source file(s) (graph: incremental)."); ``` -- 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]
