gnodet-bot commented on code in PR #1161:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1161#discussion_r4218523122
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -708,26 +738,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:
⚠️ **NOT ADDRESSED: Behavioral regression for timestamp strategy with
`proc=only` / `proc=full`**
The previous review flagged this and the new commits do not change
`amendincrementalCompilation()`.
**What this PR does:** Removes the `else` branch (`aspects.clear()` +
`NONE`) and unconditionally adds `REBUILD_ON_ADD + REBUILD_ON_CHANGE` for all
processor configurations.
**Why this is a regression for timestamp-strategy users:**
- Old behavior (proc=only or proc=full, or JDK >= 23 with detected
processor): `NONE` → force full recompile every invocation, ensuring annotation
processors run even if no source changed.
- New behavior: `REBUILD_ON_ADD + REBUILD_ON_CHANGE` → skip compilation if
no source changed. Processors do **not** run on unchanged sources.
For the **graph/ABI strategy**, this is correct — `ProcessorClassification`
handles processor lifecycle. But `amendincrementalCompilation()` is called from
`ToolExecutor` constructor before the strategy is checked in `compile()`. The
`incrementalBuildConfig` it populates is only consumed by
`applyIncrementalBuild()` (timestamp path). So timestamp-strategy users with
explicit `proc=only`/`proc=full` **now get processors skipped on
unchanged-source builds**.
The PR description's rationale ("let the active strategy handle the
details") only holds for graph/ABI. The timestamp strategy doesn't inspect
`proc` at all — it just uses the aspects.
Either:
1. Pass the `incrementalStrategy` to `amendincrementalCompilation()` and
restore `NONE` for timestamp users with explicit proc modes, or
2. Document explicitly that `proc=only`/`proc=full` no longer guarantees
processor execution on unchanged sources when using the timestamp strategy.
##########
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.");
}
- /**
- * Tests that annotation processing runs again when {@code proc} is {@code
only}, even if the sources are unchanged.
- */
- @Test
- @Basedir("${basedir}/target/test-classes/unit/compiler-proc-only-test")
- public void testCompilerProcOnlyRunsWhenSourcesAreUnchanged(
- @InjectMojo(goal = "compile", pom = "plugin-config.xml")
CompilerMojo compileMojo) {
- Log log = mock(Log.class);
- compileMojo.logger = log;
- compileMojo.execute();
-
- clearInvocations(log);
- compileMojo.execute();
-
- verify(log, never()).info("Nothing to compile - all classes are up to
date.");
- assertCompilerStubOutputFileExists(compileMojo);
- }
-
- /**
- * Tests that full annotation processing runs again when sources are
unchanged.
- */
- @Test
- @Basedir("${basedir}/target/test-classes/unit/compiler-proc-full-test")
- public void testCompilerProcFullRunsWhenSourcesAreUnchanged(
- @InjectMojo(goal = "compile", pom = "plugin-config.xml")
CompilerMojo compileMojo) {
- Log log = mock(Log.class);
- compileMojo.logger = log;
- compileMojo.execute();
-
- clearInvocations(log);
- compileMojo.execute();
-
- verify(log, never()).info("Nothing to compile - all classes are up to
date.");
- assertCompilerStubOutputFileExists(compileMojo);
- }
-
- /**
- * Tests that an explicitly configured incremental compilation value takes
precedence over annotation processing.
- */
- @Test
- public void testCompilerProcOnlyRespectsExplicitIncrementalCompilation() {
- CompilerMojo compileMojo = new CompilerMojo();
- compileMojo.incrementalCompilation = "classes";
- EnumSet<IncrementalBuild.Aspect> aspects =
EnumSet.of(IncrementalBuild.Aspect.CLASSES);
- compileMojo.amendincrementalCompilation(aspects,
Set.of(JavaPathType.PROCESSOR_CLASSES));
- assertEquals(EnumSet.of(IncrementalBuild.Aspect.CLASSES), aspects);
- }
-
/**
Review Comment:
⚠️ **NOT ADDRESSED: Proc behavior change has no test coverage**
The PR removes `testCompilerProcOnlyRunsWhenSourcesAreUnchanged` and
`testCompilerProcFullRunsWhenSourcesAreUnchanged` (verified the old `NONE`
behavior), and also removes
`testCompilerProcOnlyRespectsExplicitIncrementalCompilation` — the PR
description's claim that the last test is retained is incorrect; it is deleted
at diff line 5039.
No new test is added for the new behavior (processors now only run when
sources change). This is not a matter of style — the behavior changed, the
tests need to change too:
- The old tests `verify(log, never()).info("Nothing to compile - all classes
are up to date.")` verified that processors always forced a rebuild. Under the
new code, these would now **fail** (processors no longer force a rebuild on
unchanged sources with the timestamp strategy).
- Add a test that verifies the new intended behavior: with `proc=only` and
unchanged sources, compilation is skipped (i.e., `"Nothing to compile"` is
logged). This documents and validates the intentional behavior change.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1398,7 +1417,18 @@ public Options parseParameters(final OptionChecker
compiler) {
*/
@SuppressWarnings("UseSpecificCatch")
private void compile(final JavaCompiler compiler, final Options
configuration) throws IOException {
- final ToolExecutor executor = createExecutor(null);
+ var executor = createExecutor(null);
+ if (("graph".equalsIgnoreCase(incrementalStrategy) ||
"abi".equalsIgnoreCase(incrementalStrategy))
Review Comment:
⚠️ **No validation for unrecognized `incrementalStrategy` values**
This is a new public `@Parameter(since = "4.0.0-beta-7")` that users will
configure in their POMs. The compile-time check is:
```java
if ("graph".equalsIgnoreCase(incrementalStrategy) ||
"abi".equalsIgnoreCase(incrementalStrategy)) {
```
If a user sets `<incrementalStrategy>typo</incrementalStrategy>` or any
other unrecognized value, the code silently falls through to the timestamp
strategy with no diagnostic. This makes typos invisible and will be very hard
to debug.
Add a warning for unrecognized values:
```java
} else if (!"timestamp".equalsIgnoreCase(incrementalStrategy)) {
logger.warn("Unrecognized incrementalStrategy value: '" +
incrementalStrategy
+ "'. Valid values are: timestamp, graph, abi. Falling back to
timestamp.");
}
```
--
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]
