gnodet-bot commented on code in PR #1144:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1144#discussion_r4217529453
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1399,6 +1416,16 @@ public Options parseParameters(final OptionChecker
compiler) {
@SuppressWarnings("UseSpecificCatch")
private void compile(final JavaCompiler compiler, final Options
configuration) throws IOException {
final ToolExecutor executor = createExecutor(null);
+ if ("abi".equalsIgnoreCase(incrementalStrategy) &&
!Boolean.FALSE.equals(useIncrementalCompilation)) {
+ if
(!org.apache.maven.plugin.compiler.incremental.BytecodeAnalyzer.isAvailable()) {
+ logger.warn("ABI incremental strategy requires JDK 24 or later
"
+ + "(running JDK " + Runtime.version().feature() + "). "
+ + "Falling back to timestamp strategy.");
+ } else {
+ executor.compileWithAbiIncremental(compiler, configuration,
this);
Review Comment:
🔴 **Critical: method name mismatch — compilation error.** `ToolExecutor`
defines `compileWithGraphIncremental()` (not `compileWithAbiIncremental()`).
This call will fail to compile. Either rename the call site to match the
definition, or rename the method to `compileWithAbiIncremental()` (and update
`ToolExecutorForTest` accordingly).
##########
src/main/java/org/apache/maven/plugin/compiler/incremental/GraphIncrementalBuild.java:
##########
@@ -0,0 +1,947 @@
+/*
+ * 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.ArrayDeque;
+import java.util.ArrayList;
+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;
+
+/**
+ * Dependency-graph-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
+ * class-level dependency graph. Typical usage:
+ *
+ * {@snippet :
+ * var build = new GraphIncrementalBuild(outputDir);
+ * build.setProcessorPath(processorPath);
+ * build.setConfigHash(configHash);
+ *
+ * Set<Path> toCompile = build.initialize(allSourceFiles);
+ *
+ * while (!toCompile.isEmpty()) {
+ * compiler.compile(toCompile); // any compiler, any mode
+ * toCompile = build.processCompiledClasses(toCompile);
+ * }
+ *
+ * build.finish();
+ * }
+ *
+ * <p>After each compilation pass, {@link #processCompiledClasses(Set)} scans
+ * the freshly produced {@code .class} files, updates the dependency graph,
+ * and returns any additional source files that must be compiled in the next
+ * pass (cascade due to changed classes, or newly discovered dependencies).
+ * The loop converges in at most 2–3 passes in practice.
+ *
+ * <p>The engine persists its state as {@code 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).
+ *
+ * @see IncrementalState
+ */
+public class GraphIncrementalBuild {
+
+ /** Prefix used to distinguish module-info entries from regular type
entries in the state. */
+ static final String MODULE_PREFIX = BytecodeAnalyzer.MODULE_PREFIX;
+
+ private final Path outputDir;
+ private final Path buildDir;
+ private final Path stateFile;
+ private List<Path> classpathEntries;
+ private Set<Path> reactorModulePaths;
+ private boolean abiTracking;
+ private List<Path> processorPath;
+ private ProcessorClassification processorClassification;
+
+ private IncrementalState previousState;
+ private IncrementalState state;
+
+ /** Package-private accessor for tests. */
+ IncrementalState getState() {
+ return 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;
+ /** Lazily populated on full builds; maps each output class file to its
simple top-level class name. */
+ private Map<Path, String> outputClassIndex;
+
+ public GraphIncrementalBuild(Path outputDir) {
+ this.outputDir = outputDir.toAbsolutePath();
+ this.buildDir = this.outputDir.getParent() != null ?
this.outputDir.getParent() : this.outputDir;
+ // Store state outside the output directory so it is not included in
the JAR.
+ // Use the same maven-status convention as the timestamp-based
strategy.
+ // Include the output directory name (e.g. "classes", "test-classes")
to avoid
+ // collisions between compile and testCompile executions.
+ String outputDirName = outputDir.getFileName().toString();
+ Path mavenStatus =
buildDir.resolve("maven-status").resolve("maven-compiler-plugin");
+ this.stateFile =
mavenStatus.resolve(outputDirName).resolve("incremental-state");
+ }
+
+ /**
+ * Sets classpath entries for cross-module ABI tracking. Directory entries
+ * are checked for {@link AbiManifest} files; JAR entries use bytecode
+ * analysis as fallback.
+ */
+ public void setClasspathEntries(List<Path> entries) {
+ this.classpathEntries = entries;
+ }
+
+ /**
+ * Marks specific classpath entries as reactor modules. These are always
+ * checked for ABI changes (via manifest or bytecode).
+ */
+ public void setReactorModulePaths(Set<Path> paths) {
+ this.reactorModulePaths = paths;
+ }
+
+ /**
+ * Enables ABI tracking mode ({@code abi} strategy). When {@code true}, the
+ * engine computes ABI fingerprints and fine-grained {@code signatureDeps}/
+ * {@code implementationDeps} for each compiled type, and writes an
+ * {@link AbiManifest} on {@link #finish()}. When {@code false} (default,
+ * {@code graph} strategy), only class-level dependency references are
collected.
+ */
+ public void setAbiTracking(boolean abiTracking) {
+ this.abiTracking = abiTracking;
+ }
+
+ /**
+ * Sets the annotation processor classpath for processor classification.
+ * Entries are scanned for {@code
META-INF/maven/compiler/incremental.annotation.processors}
+ * and {@code META-INF/gradle/incremental.annotation.processors} to
determine
+ * whether each processor is {@link ProcessorType#ISOLATING},
+ * {@link ProcessorType#AGGREGATING}, or {@link ProcessorType#UNKNOWN}.
+ */
+ public void setProcessorPath(List<Path> processorPath) {
+ this.processorPath = processorPath;
+ this.processorClassification = new
ProcessorClassification(processorPath);
+ }
+
+ /**
+ * Indicates that class files are written under module-name subdirectories
+ * of the output directory (MODULE_SOURCE hierarchy). When set, stored
module
+ * names are used as path prefixes when deleting class files.
+ */
+ public void setUseModulePrefixedPaths(boolean useModulePrefixedPaths) {
+ this.useModulePrefixedPaths = useModulePrefixedPaths;
+ }
+
+ /**
+ * Sets a hash of compilation context configuration (e.g. compiler options
+ * and module-info-patch files). If this hash differs from the previous
+ * build, a full rebuild is triggered.
+ */
+ public void setConfigHash(String hash) {
+ this.configHash = hash != null ? hash : "";
+ }
+
+ /**
+ * Initializes the incremental build by scanning source files and comparing
+ * against the previous build's state.
+ *
+ * @param allSourceFiles all source files in this module
+ * @return the set of files that need compilation (may be all files for a
+ * full build, a subset for incremental, or empty if up-to-date)
+ */
+ public Set<Path> initialize(List<Path> allSourceFiles) throws IOException {
+ Files.createDirectories(outputDir);
+
+ this.allSourceFiles = allSourceFiles;
+ totalSources = allSourceFiles.size();
+ allCompiled = new TreeSet<>();
+ previousState = IncrementalState.load(stateFile);
+ sourceMtimes = new LinkedHashMap<>();
+ sourceHashes = hashSourceFiles(allSourceFiles, previousState,
sourceMtimes);
+
+ if (previousState == null) {
+ rebuildCause = "no previous build state";
+ return initFullBuild(allSourceFiles);
+ } else if (!configHash.equals(previousState.getConfigHash())) {
+ rebuildCause = "compilation configuration changed
(module-info-patch.maven or compiler options)";
+ return initFullBuild(allSourceFiles);
+ } else {
+ return initIncrementalBuild(allSourceFiles);
+ }
+ }
+
+ /**
+ * Processes the {@code .class} files produced by the last compilation
pass.
+ * Scans each class file to extract the dependency graph, records changes,
+ * and returns any additional source files that must be compiled in the
next pass.
+ *
+ * <p>The cascade logic: any compiled class whose content changed (new or
modified)
+ * triggers recompilation of all source files that depend on it (signature
or
+ * implementation consumers).
+ *
+ * @param compiledSourceFiles the source files that were passed to the
compiler in this round
+ * @return additional source files to compile (may be empty when fixpoint
is reached)
+ * @throws IOException if reading {@code .class} files fails
+ */
+ public Set<Path> processCompiledClasses(Set<Path> compiledSourceFiles)
throws IOException {
+ // Reset the class index so it is rebuilt fresh for each compilation
round
+ outputClassIndex = null;
+
+ // Scan .class files for the types produced from the compiled source
files
+ var results = new LinkedHashMap<String, SourceFileAnalysis>();
+ for (Path sourceFile : compiledSourceFiles) {
+ collectClassAnalyses(sourceFile, results);
+ }
+
+ // All compiled types cascade to their consumers (any change triggers
recompilation)
+ var changedTypes = new TreeSet<>(results.keySet());
Review Comment:
🟠**Medium: ABI fingerprint comparison is missing — cascade is always
maximal.** `changedTypes` is initialized to `results.keySet()` (all compiled
types) unconditionally. The ABI manifest is computed and stored in
`IncrementalState.AbiTypeInfo` but is never compared against the previous
round's manifest to filter which types actually changed their public API. This
means the `abi` strategy cascades all compiled types on every incremental build
— no better than graph without ABI filtering. The filtering logic that would
compare `newAbi != oldAbi` before adding to `changedTypes` is absent.
##########
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>graph</maven.compiler.incrementalStrategy>
Review Comment:
🔴 **Critical: `incrementalStrategy=graph` has no dispatch path.** The
`compile()` method in `AbstractCompilerMojo` only checks
`"abi".equalsIgnoreCase(incrementalStrategy)` — a value of `"graph"` falls
through to the timestamp strategy. Both `verify.groovy` scripts assert `'graph:
full build'` and `'graph: incremental'`/`'graph cascade: recompiling'`, which
are emitted by `GraphIncrementalBuild` — but with `graph` strategy falling
through to timestamp, those log messages will never appear. Both ITs will fail.
Either add a `graph` dispatch branch, or change the IT strategy to `abi` if
the intent was to test ABI tracking.
--
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]
