gnodet-bot commented on code in PR #1161:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1161#discussion_r4218087435
##########
src/test/java/org/apache/maven/plugin/compiler/CompilerMojoTestCase.java:
##########
@@ -207,54 +204,6 @@ public void testCompilerEmptySourceChangeDetection(
verify(log).info("Nothing to compile - all classes are up to date.");
}
Review Comment:
⚠️ **All proc-related test coverage removed**
The three deleted tests (`testCompilerProcOnlyRunsWhenSourcesAreUnchanged`,
`testCompilerProcFullRunsWhenSourcesAreUnchanged`, and
`testCompilerProcOnlyRespectsExplicitIncrementalCompilation`) were the only
coverage for the proc=only/proc=full incremental behavior on the timestamp
strategy. If the behavior change in `amendincrementalCompilation()` is
intentional (processors no longer run unconditionally on unchanged sources),
tests demonstrating the new correct behavior are needed. If it's unintentional,
these tests should be restored.
Also: the PR description states "The
`testCompilerProcOnlyRespectsExplicitIncrementalCompilation` test is retained"
— it is in fact deleted.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -708,26 +736,15 @@ final EnumSet<IncrementalBuild.Aspect>
incrementalCompilationConfiguration() {
}
/**
- * Amends the default configuration of incremental compilation for
annotation processing.
- * When processing is explicitly requested, incremental compilation is
disabled so that processors always run.
- * On Java versions before 23, an absent {@code proc} value only implies
the compiler's default processing mode;
- * it does not prove that a processor is present, so the traditional
rebuild-on-add/change behavior is retained.
- * This method does not amend an explicitly configured {@link
#incrementalCompilation} value.
+ * Amends the configuration of incremental compilation for the presence of
annotation processors.
*
* @param aspects the configuration to amend if an annotation processor is
found
* @param dependencyTypes the type of dependencies, for checking if any of
them is a processor path
*/
final void amendincrementalCompilation(EnumSet<IncrementalBuild.Aspect>
aspects, Set<PathType> dependencyTypes) {
if (isAbsent(incrementalCompilation) &&
hasAnnotationProcessor(dependencyTypes)) {
- if (isAbsent(proc) && !isVersionEqualOrNewer(RELEASE_23)) {
- // Case when `hasAnnotationProcessor(…)` cannot decide for
sure.
- // Apply an intermediate strategy between "no processor" and
"processor for sure".
- aspects.add(IncrementalBuild.Aspect.REBUILD_ON_ADD);
- aspects.add(IncrementalBuild.Aspect.REBUILD_ON_CHANGE);
- } else {
- aspects.clear();
- aspects.add(IncrementalBuild.Aspect.NONE);
- }
+ aspects.add(IncrementalBuild.Aspect.REBUILD_ON_ADD);
+ aspects.add(IncrementalBuild.Aspect.REBUILD_ON_CHANGE);
}
Review Comment:
⚠️ **Behavioral regression for timestamp strategy with `proc=only` /
`proc=full`**
The old code had two distinct branches:
1. `isAbsent(proc) && JDK < 23` → add `REBUILD_ON_ADD + REBUILD_ON_CHANGE`
(uncertain about processors)
2. **else** (proc explicitly set to `only`/`full`, or JDK ≥ 23 with
confirmed processors) → `aspects.clear()` + `NONE` (force unconditional
compilation)
The new code collapses both to just `REBUILD_ON_ADD + REBUILD_ON_CHANGE`.
But these are **modifiers** on top of existing `SOURCES`/`CLASSES` checks —
they escalate individual-file changes to full rebuilds, but they don't bypass
the "Nothing to compile" short-circuit in `applyIncrementalBuild()`. With
`proc=only`, the user's explicit intent is to run processors on every build
even when no source changed. The old `NONE` aspect guaranteed that. The new
code allows `applyIncrementalBuild()` to conclude "nothing to do" and skip the
entire processor run.
The PR rationale says "ProcessorClassification handles this" — but
`ProcessorClassification` is only instantiated inside `GraphIncrementalBuild`
(the ABI strategy path). The timestamp strategy calls
`amendincrementalCompilation()` → `applyIncrementalBuild()`, and that path
knows nothing about `ProcessorClassification`.
```suggestion
final void amendincrementalCompilation(EnumSet<IncrementalBuild.Aspect>
aspects, Set<PathType> dependencyTypes) {
if (isAbsent(incrementalCompilation) &&
hasAnnotationProcessor(dependencyTypes)) {
if (isAbsent(proc)) {
aspects.add(IncrementalBuild.Aspect.REBUILD_ON_ADD);
aspects.add(IncrementalBuild.Aspect.REBUILD_ON_CHANGE);
} else {
// proc=only/full/default: processors must run
unconditionally
aspects.clear();
aspects.add(IncrementalBuild.Aspect.NONE);
}
}
}
```
##########
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)) {
Review Comment:
💡 **Silent fallback for `graph` strategy users**
PR #1155 documented and accepted `graph` as a strategy name (with `abi` as
its alias). This PR drops `graph` silently: a user who configured
`incrementalStrategy=graph` on an intermediate snapshot will now silently fall
back to the timestamp strategy with no diagnostic. Since #1155 is unreleased
this is low severity, but a one-line warning log would be cheap insurance:
```java
} else if ("graph".equalsIgnoreCase(incrementalStrategy)) {
logger.warn("incrementalStrategy=graph is no longer supported; use 'abi'
instead. Falling back to timestamp strategy.");
}
```
--
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]
