Save attached outputs for compile-only cache entries

[cowwoc]

Summary

This PR fixes incomplete cache restoration from compile-only builds by ensuring that configured outputs (e.g., classes, test-classes, module-info) are always attached and saved to the cache, regardless of the highest lifecycle phase executed.

Changes Made

1. Reset attached resource bookkeeping per save invocation

   • Clears attachedResourcesPathsById and resets attachedResourceCounter at the start of each save() call
   • Prevents stale artifacts from previous builds in multi-module projects from polluting cache entries

2. Attach outputs for all lifecycle phases

   • Moves attachGeneratedSources() and attachOutputs() outside the hasLifecyclePhase("package") conditional
   • Ensures compile-only builds persist configured output directories to enable cache hits in subsequent builds
   • Maintains consistency between save and restore operations: what gets cached can be restored

3. Add comprehensive integration test

   • Tests JPMS multi-module project with compile-only followed by verify
   • Verifies module-info.class files are restored correctly after cache hit
   • Covers the exact scenario reported in #393

Addressing Reviewer Feedback from PR #176

In PR #176#discussion_r1732039886, @AlexanderAshitkin suggested:

"More accurate implementation could support pre-compile phases - 'save' and restore generated sources and continue with compile."

This PR implements exactly that suggestion:

• Pre-compile phase support: Compile-only builds now save all configured outputs
• Generated sources: Already supported via attachGeneratedSources()
• Continue with compile: Subsequent builds restore outputs and continue seamlessly with later phases
• Save/Restore consistency: Attached outputs are saved unconditionally and restored based on configuration (controlled by isRestoreGeneratedSources())

Root Cause Analysis

Before this PR:

if (project.hasLifecyclePhase("package")) {
    attachGeneratedSources(project);  // Only for package phase
    attachOutputs(project);           // Only for package phase
    // ...
}

Problem: Compile-only builds (mvn clean compile) never called attachOutputs(), creating cache entries without critical files like module-info.class.

After this PR:

attachGeneratedSources(project);  // Always called
attachOutputs(project);           // Always called

Result: Compile-only builds save configured outputs, enabling proper restoration in subsequent builds.

Testing

Run the integration test:

./mvnw -Prun-its -Dit.test=Issue393CompileRestoreTest verify

The test validates:

1. ✅ mvn clean compile creates cache entry with module-info.class
2. ✅ mvn clean verify restores from cache including module-info.class
3. ✅ Consumer module can compile against cached JPMS module descriptors
4. ✅ Tests execute successfully with restored artifacts

Related Issues

Fixes #393 - Incomplete Cache Restoration from compile-only Builds
Fixes #259 - Cannot restore cache after mvn clean commands
Fixes #340 - Cache fails to restore compiled classes with mvn clean test

Migration Notes

No configuration changes required. Existing projects will automatically benefit from this fix on their next build.

---

Note on Implementation Philosophy: This PR prioritizes correctness and consistency over optimization. All lifecycle phases save configured outputs to ensure cache integrity. Future optimizations could selectively skip certain outputs based on phase, but this would require careful analysis to avoid reintroducing the bugs fixed here.

[cowwoc]

Updates Based on Reviewer Feedback

I've improved this PR to address the concerns raised by @AlexanderAshitkin in PR #176's review:

Code Improvements

1. Added detailed inline comments explaining:

   • Why bookkeeping reset is necessary (prevents stale artifacts in multi-module builds)
   • That outputs are attached for ALL lifecycle phases (including compile-only)
   • How this addresses the JPMS module descriptor restoration issues

2. Added debug logging to make compile-only cache saves visible in build logs

3. Enhanced PR description with:

   • Clear explanation of how this implements the "pre-compile phase" suggestion
   • Root cause analysis comparing before/after behavior
   • Explicit mapping to the reviewer's feedback

Addressing "Save/Restore Consistency"

The reviewer's concern about consistency between save and restore operations is addressed:

Save side (this PR):

• Always attaches configured outputs (classes, test-classes, etc.)
• Creates cache entries that contain all necessary artifacts for restoration

Restore side (existing code):

• Restores attached outputs based on configuration (isRestoreGeneratedSources())
• The configuration controls what gets restored, not what gets saved

This design ensures:

• ✅ Compile-only builds can be restored by subsequent builds
• ✅ Users retain control over restoration behavior via configuration
• ✅ No cache entries are created without necessary outputs

Testing

The integration test validates the complete workflow:

./mvnw -Prun-its -Dit.test=Issue393CompileRestoreTest verify

Test coverage includes:

• JPMS module descriptors (module-info.class)
• Multi-module projects with inter-module dependencies
• Compile-only → verify workflow (the reported bug scenario)

[cowwoc]

Addressing Performance Concerns

Thank you for raising the important question about performance overhead during active development. You're absolutely right that caching compile outputs on every mvn compile has costs:

• IO overhead: Zipping classes directories
• CPU overhead: Computing hashes
• Network overhead: Potentially uploading to remote cache

Solution: Configurable Compile Caching

I've added a new property maven.build.cache.cacheCompile (default: true) to give users control:

# Disable compile caching during active development:
mvn clean compile -Dmaven.build.cache.cacheCompile=false

# Enable for CI/multi-module scenarios (default):
mvn clean compile

Design Rationale

Default: true - Maintains the fix for issue #393 without breaking changes. Users experiencing compile-only cache restoration issues get the fix automatically.

Opt-out available - Developers actively editing code can disable compile caching to avoid overhead, while still benefiting from package-phase caching.

Use Cases

When to keep enabled (default):

• Large multi-module projects (editing 1 of 50 modules)
• CI/CD pipelines for quick feedback
• Branch switching scenarios
• Working on test code only

When to disable:

• Actively editing a single module
• Rapid edit-compile-test cycles
• Development environments where package phase is always run anyway

Implementation

The property gates the calls to attachGeneratedSources() and attachOutputs() in CacheControllerImpl.save():

if (cacheConfig.isCacheCompile()) {
    attachGeneratedSources(project, state, buildStartTime);
    attachOutputs(project, state, buildStartTime);
}

This means:

• When false: mvn compile creates NO cache entry (no zipping, no IO)
• When true (default): Full caching as implemented for issue #393

Testing

Added CacheCompileDisabledTest to verify:

• ✅ With property disabled: No cache entries created, no restoration on second compile
• ✅ With property enabled: Cache entries created, restoration works on second compile

Ready for your review!

[olamy]

If I understand correctly, all of this is for being able to use the cache with mvn compile? Why not test-compile?

[cowwoc]

@elharo Fixed. Try now. @olamy The code change should work for both compile and test-compile. Did I misunderstand your question?

[cowwoc]

I fixed the build failures. At the same time, I introduced a change that requires your review.

To protect against caching stale artifacts caused by:

1. Future timestamps (due to clock skew or malicious parties)
2. Orphaned class files (from deleted source files)
3. Stale JARs/WARs (from previous builds)

I implemented a "staging directory" approach. The performance impact should be minimal as the staging operation only occurs for uncached builds and uses fast filesystem moves rather than copies. Cached builds are unaffected.

How it works

Before the build: Pre-existing artifacts are moved to target/maven-build-cache-extension/ in the multimodule root, preserving their full path structure (e.g., module1/target/classes). This ensures Maven sees a clean target directory and must rebuild everything.

After save(): Files are restored from staging if they weren't rebuilt (indicating they're still valid), or discarded if fresh versions exist.

This approach is more robust than timestamp comparison, as it physically separates potentially stale artifacts rather than relying on heuristics.

[erik-meuwese-topicus]

This pr fixes the build errors we had with maven-build-cache-extension 1.2.1 mvn compile. It would be nice if this patch could be in the next version of maven-build-cache-extension.

There are a few files which need the license header to follow the Maven code convention https://maven.apache.org/developers/conventions/code.html . I placed a comment on those files.

[cowwoc]

@erik-meuwese-topicus Thanks for catching the missing license headers. I've fixed them. Is there anything else blocking approval and merge?

[cowwoc]

@erik-meuwese-topicus Try now.

[erik-meuwese-topicus]

The tests fail after the log levels have been changed from info to debug https://github.com/apache/maven-build-cache-extension/pull/394/commits/32e939919ec0a4a847675458e9d41bdba8860474. Add verifier.addCliOption("--debug"); or verifier.addCliOption("-X"); to output the debug logging so verifyTextInLog() can find the logs.

Patch to fix the tests:

Index: src/test/java/org/apache/maven/buildcache/its/BuildExtensionTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/java/org/apache/maven/buildcache/its/BuildExtensionTest.java b/src/test/java/org/apache/maven/buildcache/its/BuildExtensionTest.java
--- a/src/test/java/org/apache/maven/buildcache/its/BuildExtensionTest.java	(revision 32e939919ec0a4a847675458e9d41bdba8860474)
+++ b/src/test/java/org/apache/maven/buildcache/its/BuildExtensionTest.java	(revision 9c6bf073e6fc5fafcf84e894172e72ee31274501)
@@ -58,6 +58,7 @@
         verifier.getCliOptions().clear();
         verifier.addCliOption("-D" + CACHE_LOCATION_PROPERTY_NAME + "=" + tempDirectory.toAbsolutePath());
         verifier.addCliOption("-D" + SKIP_SAVE + "=true");
+        verifier.addCliOption("--debug");
 
         verifier.setLogFileName("../log-1.txt");
         verifier.executeGoal("verify");
Index: src/test/java/org/apache/maven/buildcache/its/MandatoryCleanTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/java/org/apache/maven/buildcache/its/MandatoryCleanTest.java b/src/test/java/org/apache/maven/buildcache/its/MandatoryCleanTest.java
--- a/src/test/java/org/apache/maven/buildcache/its/MandatoryCleanTest.java	(revision 32e939919ec0a4a847675458e9d41bdba8860474)
+++ b/src/test/java/org/apache/maven/buildcache/its/MandatoryCleanTest.java	(revision 9c6bf073e6fc5fafcf84e894172e72ee31274501)
@@ -52,6 +52,7 @@
         Path tempDirectory = Files.createTempDirectory("simple-mandatory-clean");
         verifier.getCliOptions().clear();
         verifier.addCliOption("-D" + CACHE_LOCATION_PROPERTY_NAME + "=" + tempDirectory.toAbsolutePath());
+        verifier.addCliOption("--debug");
 
         verifier.setLogFileName("../log-1.txt");
         verifier.executeGoal("verify");
@@ -99,6 +100,7 @@
     void disabledViaProperty(Verifier verifier) throws VerificationException {
 
         verifier.setAutoclean(false);
+        verifier.addCliOption("--debug");
 
         verifier.setLogFileName("../log-1.txt");
         verifier.executeGoal("verify");
@@ -120,6 +122,7 @@
         verifier.getCliOptions().clear();
         // With "true", we do not change the initially expected behaviour
         verifier.addCliOption("-D" + CacheConfigImpl.MANDATORY_CLEAN + "=true");
+        verifier.addCliOption("--debug");
         verifier.executeGoal("verify");
         verifier.verifyErrorFreeLog();
         List<String> cacheSkippedBuild2 = LogFileUtils.findLinesContainingTextsInLogs(
@@ -138,6 +141,7 @@
         // With "false", we remove the need for the clean phase
         verifier.getCliOptions().clear();
         verifier.addCliOption("-D" + CacheConfigImpl.MANDATORY_CLEAN + "=false");
+        verifier.addCliOption("--debug");
         verifier.setLogFileName("../log-3.txt");
         verifier.executeGoal("verify");
         verifier.verifyErrorFreeLog();

[cowwoc]

Thanks @erik-meuwese-topicus! Fixed.

Can you please rerun the build?