gnodet-bot commented on code in PR #1179:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1179#discussion_r4351477400
##########
src/main/java/org/apache/maven/plugin/compiler/DependencyState.java:
##########
@@ -137,6 +137,15 @@ private static Path addDependencies(
* The directory digest covers each relevant file's relative path, size
and modification time.
*/
private static ModificationState modificationState(Path dependency,
ScanContext context) {
+ if (Files.notExists(dependency)) {
+ // A reactor module with no sources for this output kind (e.g. no
main sources, so
+ // `target/classes` is never created) still appears on the
classpath. Treat it like an
+ // empty directory instead of letting `readAttributes` below fail
with a spurious
+ // "I/O error reading dependency state" warning and an
"unreadable" state that never
+ // compares equal across builds, which defeated incremental
compilation (forced a
+ // recompile on every run).
+ return new ModificationState(0 + ":" +
toHexString(newDigest().digest()), null);
Review Comment:
⚠️ **Style nit:** `0 + ":"` is a redundant int-to-String promotion that
looks like a mistake. Prefer the string literal directly:
```suggestion
return new ModificationState("0:" +
toHexString(newDigest().digest()), null);
```
(Note: the existing PR #1125 uses the same pattern — worth fixing there too.)
##########
src/test/java/org/apache/maven/plugin/compiler/DependencyStateTest.java:
##########
@@ -113,6 +116,33 @@ void preservesRepeatedDependencyEntriesAndPathKinds()
throws Exception {
assertTrue(state.get(2).startsWith("modulepath:"));
}
+ @Test
+ void
treatsMissingDependencyDirectoryAsEmptyWithoutWarningOrForcedRebuild() throws
Exception {
+ Path dependency =
temporaryDirectory.resolve("classes-not-yet-compiled");
+ IncrementalBuildHelper helper = mock(IncrementalBuildHelper.class);
+
when(helper.getMojoStatusDirectory()).thenReturn(temporaryDirectory.toFile());
+ Log log = mock(Log.class);
+
+ DependencyState.Configuration configuration =
+ new DependencyState.Configuration(Collections.emptySet(),
BUILD_START, 0, log, false);
+ List<String> classpath =
Collections.singletonList(dependency.toString());
+
+ assertFalse(DependencyState.hasChanged(
+ helper,
+ temporaryDirectory.resolve("output").toFile(),
+ classpath,
+ Collections.emptyList(),
+ configuration));
+ assertFalse(DependencyState.hasChanged(
+ helper,
+ temporaryDirectory.resolve("output").toFile(),
+ classpath,
+ Collections.emptyList(),
+ configuration));
+
+ verify(log, never()).warn(contains("I/O error"));
Review Comment:
⚠️ **Incomplete test coverage:** This test only verifies the _missing → no
warning + no rebuild_ path. It never creates `classes-not-yet-compiled` and
then calls `hasChanged` again to verify it returns `true`, which is the
critical caching correctness case: once the directory appears, the compiler
must detect a change and recompile. Without this, a caching regression (stale
empty-state that prevents recompile when the directory later materialises)
could slip through undetected.
PR #1125 already covers this transition in its commit `9ecf77f`. Please
defer to that PR.
--
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]
