gnodet-bot commented on code in PR #1155:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1155#discussion_r4181322243
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1399,6 +1424,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 ("graph".equalsIgnoreCase(incrementalStrategy) &&
!Boolean.FALSE.equals(useIncrementalCompilation)) {
+ if
(!org.apache.maven.plugin.compiler.incremental.BytecodeAnalyzer.isAvailable()) {
+ logger.warn("Graph incremental strategy requires JDK 24 or
later "
+ + "(running JDK " + Runtime.version().feature() + "). "
+ + "Falling back to timestamp strategy.");
+ } else {
+ executor.compileWithAbiIncremental(compiler, configuration,
this);
+ return;
+ }
+ }
Review Comment:
🔴 **Critical:** The dispatch only matches `"graph"` but the ITs and the
strategy itself are named `"abi"`. This check needs to accept `"abi"` (or both
values).
```suggestion
if (("graph".equalsIgnoreCase(incrementalStrategy) ||
"abi".equalsIgnoreCase(incrementalStrategy))
&& !Boolean.FALSE.equals(useIncrementalCompilation)) {
if
(!org.apache.maven.plugin.compiler.incremental.BytecodeAnalyzer.isAvailable()) {
logger.warn("Graph/ABI incremental strategy requires JDK 24
or later "
+ "(running JDK " + Runtime.version().feature() +
"). "
+ "Falling back to timestamp strategy.");
} else {
executor.compileWithAbiIncremental(compiler, configuration,
this);
return;
}
}
```
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -682,6 +682,31 @@ final Charset charset() {
@Parameter(property = "maven.compiler.useIncrementalCompilation")
protected Boolean useIncrementalCompilation;
+ /**
+ * The strategy to use for incremental compilation.
+ * <ul>
+ * <li>{@code timestamp} (default) — the existing timestamp-based
strategy from
+ * {@link IncrementalBuild}. Detects changes by comparing source
file modification
+ * times and triggers full rebuilds when files are added/removed or
dependencies change.
+ * Respects {@link #incrementalCompilation} aspects, {@code
staleMillis}, and
+ * {@code incrementalExcludes}.</li>
+ * <li>{@code graph} — dependency-graph-based strategy. Tracks
class-level dependencies
+ * by analysing bytecode after each compilation pass. When a source
file changes, only
+ * the source files that transitively depend on any of its classes
are recompiled. Any
+ * change to a class (API or implementation) cascades to all its
consumers.
+ * Full JPMS support including {@code module-info.java} and
+ * {@code module-info-patch.maven} tracking.
+ * <p>Note: the graph strategy has its own change detection (content
hashing) and does
+ * not use {@code staleMillis}, {@code incrementalExcludes}, or the
+ * {@link #incrementalCompilation} aspects. Setting {@link
#useIncrementalCompilation}
+ * to {@code false} disables this strategy and forces a full
rebuild.</p></li>
+ * </ul>
+ *
+ * @since 4.0.0-beta-7
+ */
+ @Parameter(property = "maven.compiler.incrementalStrategy", defaultValue =
"timestamp")
+ protected String incrementalStrategy;
+
/**
* Returns the configuration of the incremental compilation.
* If the argument is null or blank, then this method applies
Review Comment:
🔴 **Critical (re-raised from previous review, NOT addressed):** The dispatch
in `compile()` only checks `"graph"` but this Javadoc doesn't mention `abi`,
and the ITs set `maven.compiler.incrementalStrategy=abi`. There is no code path
that handles the `"abi"` value — it silently falls through to the timestamp
strategy.
This means:
1. Users who set `incrementalStrategy=abi` (following the IT examples) get
timestamp strategy silently
2. The ITs for `abi-incremental-basic` and `abi-incremental-cascade` are not
actually testing the ABI strategy — they're running timestamp
3. The ITs pass by coincidence because they only check that compilation
works and some log messages appear
Either:
- Add `"abi"` to the dispatch check: `if
(("graph".equalsIgnoreCase(incrementalStrategy) ||
"abi".equalsIgnoreCase(incrementalStrategy)) && ...)`
- Or if `abi` is the intended user-facing name, change the dispatch to check
for `"abi"` and update the Javadoc
```suggestion
* The strategy to use for incremental compilation.
* <ul>
* <li>{@code timestamp} (default) — the existing timestamp-based
strategy from
* {@link IncrementalBuild}. Detects changes by comparing source
file modification
* times and triggers full rebuilds when files are added/removed
or dependencies change.
* Respects {@link #incrementalCompilation} aspects, {@code
staleMillis}, and
* {@code incrementalExcludes}.</li>
* <li>{@code abi} — ABI-fingerprint-based strategy. Tracks
class-level dependencies
* and public API surface (ABI) fingerprints via bytecode
analysis. When a source file
* changes, only files whose ABI actually changed cascade to their
consumers; body-only
* changes recompile only the changed file.
* Full JPMS support including {@code module-info.java} and
* {@code module-info-patch.maven} tracking.
* <p>Note: the ABI strategy has its own change detection (content
hashing) and does
* not use {@code staleMillis}, {@code incrementalExcludes}, or the
* {@link #incrementalCompilation} aspects. Setting {@link
#useIncrementalCompilation}
* to {@code false} disables this strategy and forces a full
rebuild.</p></li>
* </ul>
*
* @since 4.0.0-beta-7
*/
@Parameter(property = "maven.compiler.incrementalStrategy", defaultValue
= "timestamp")
protected String incrementalStrategy;
```
##########
src/it/abi-incremental-basic/verify.groovy:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.
+ */
+
+// All class files should exist
+assert new File( basedir, 'target/classes/api/Model.class' ).exists()
+assert new File( basedir, 'target/classes/impl/Helper.class' ).exists()
+assert new File( basedir, 'target/classes/impl/Service.class' ).exists()
+
+// Graph state is stored in maven-status dir (not inside classes/ to avoid
polluting JARs)
+assert new File( basedir,
'target/maven-status/maven-compiler-plugin/classes/.incremental-state'
).exists()
+
+// Build log should show incremental messages
+def logFile = new File( basedir, 'build.log' )
+assert logFile.exists()
+def content = logFile.text
+
+// Step 1 should be a full build
+assert content.contains( 'full build' ) || content.contains( 'Compiling 3
source files' )
+
+// Step 3 should show incremental behavior (not recompiling all 3 files)
+// The Graph strategy should detect changes
Review Comment:
📝 Copy-paste: comment says "The Graph strategy" but this is the ABI strategy
IT.
##########
src/it/abi-incremental-basic/verify.groovy:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.
+ */
+
+// All class files should exist
+assert new File( basedir, 'target/classes/api/Model.class' ).exists()
+assert new File( basedir, 'target/classes/impl/Helper.class' ).exists()
+assert new File( basedir, 'target/classes/impl/Service.class' ).exists()
+
+// Graph state is stored in maven-status dir (not inside classes/ to avoid
polluting JARs)
+assert new File( basedir,
'target/maven-status/maven-compiler-plugin/classes/.incremental-state'
).exists()
+
Review Comment:
🔴 **Bug:** The ABI strategy writes its state as `.abi-incremental-state`
(see `AbiIncrementalBuild` line 1060), but this assertion checks for
`.incremental-state` (the graph strategy's filename). This assertion will fail
when the ABI strategy is actually wired up (once the dispatch bug above is
fixed).
```suggestion
assert new File( basedir,
'target/maven-status/maven-compiler-plugin/classes/.abi-incremental-state'
).exists()
```
##########
src/it/abi-incremental-cascade/verify.groovy:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.
+ */
+
+// All class files should exist
+assert new File( basedir, 'target/classes/api/Model.class' ).exists()
+assert new File( basedir, 'target/classes/impl/Service.class' ).exists()
+
+// Graph state is stored in maven-status dir (not inside classes/ to avoid
polluting JARs)
+assert new File( basedir,
'target/maven-status/maven-compiler-plugin/classes/.incremental-state'
).exists()
+
+// Build should succeed (compilation didn't fail after cascade)
Review Comment:
🔴 **Bug:** Same as `abi-incremental-basic/verify.groovy` — checks for
`.incremental-state` instead of `.abi-incremental-state`.
```suggestion
assert new File( basedir,
'target/maven-status/maven-compiler-plugin/classes/.abi-incremental-state'
).exists()
```
--
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]
