Change default attachedOutputs to include classes and test-classes #388

cowwoc · 2025-10-05T21:48:33Z

Problem

When attachedOutputs is not specified in maven-build-cache-config.xml, the build cache returns an empty list, meaning no directories are cached by default. This causes issues where compiled classes are not restored from cache, leading to:

Build failures when running mvn test after cache restoration
Inconsistent behavior between cached and non-cached builds
Users needing to manually configure attachedOutputs in every project

As reported in #259 and #340, users encounter errors like:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:test
(default-test) on project: There are test failures.

This occurs because test-classes are restored but the main classes directory is empty.

Solution

Change the default value of attachedOutputs to cache the most commonly needed directories:

<attachedOutputs>
  <dirNames>
    <dirName>classes</dirName>
    <dirName>test-classes</dirName>
  </dirNames>
</attachedOutputs>

Changes

Code Changes

CacheConfigImpl.java: Modified getAttachedOutputs() to return default directories when configuration is null
Added getDefaultAttachedOutputs() helper method to create DirName instances for classes and test-classes

Documentation Updates

usage.md: Updated IDE support section to reflect that classes and test-classes are now restored by default
build-cache-config.mdo: Added documentation explaining the new default behavior

Related Issues & PRs

This change addresses multiple user requests:

Fixes [MBUILDCACHE-45] Compiled classes are not copied/restored to target, causing higher phase builds to fail #259: Compiled classes are not copied/restored to target, causing higher phase builds to fail
Fixes [MBUILDCACHE-101] mvn test fails on missing classes after mvn compile #340: mvn test fails on missing classes after mvn compile
Related to [MBUILDCACHE-103] Allow incremental restore in case of plugin parameter mismatch #177: PR discussion where contributors requested this exact default configuration

Backward Compatibility

This change is backward compatible:

Projects that explicitly configure attachedOutputs will continue to work exactly as before
Projects without configuration now get sensible defaults instead of an empty list
No breaking changes to existing functionality

Testing

✅ Build completes successfully with changes
✅ Default configuration creates proper DirName objects
✅ Existing explicit configurations remain unaffected

olamy · 2025-10-06T01:33:48Z

not sure to understand the problem.
Do you have a unit test with use case failing?

AlexanderAshitkin

Suggestions:

Add tests to check restorations.
I think the restore option should be either opt-in or opt-out, but not implicit. Users should choose when to use restorations. (We can discuss this with other members.)

src/main/java/org/apache/maven/buildcache/CacheUtils.java

AlexanderAshitkin · 2025-10-06T01:17:45Z

src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java

+        return attachedOutputs.getDirNames();
+    }
+
+    private List<DirName> getDefaultAttachedOutputs() {


This should be an opt-in feature. For example, CI builds do not need to restore classes. Restoring classes will slow down the builds in projects with large compile outputs or slow files systems.

More reliable approach is to restore output directories based on the project model instead of using hardcoded settings.

Build failures when running mvn test after cache restoration

Let me please share some background. The primary goal for the cache was to provide lightweight and fast build for CI. Saving and restoring compilation directories interferes with this goal, because it effectively duplicates IO and upload efforts - it is necessary to pack and upload almost the same jar twice. For example, Gradle, which restores classes, skips caching jar files by default to avoid doing expensive work twice. In Maven, there is no easy way to find all input/output directories for the project because plugins might have own input/output configurations. Because of that, instead of caching "classes" the choice was mad in favor of caching artifacts, for both correctness and performance reasons.

All in all - though restoring classes might be helpful in local development, it is better to consider this as a separate, opt-in feature. It might require additional efforts to implement correctly. Default save / restore should be based on the reactor introspection.

I agree; the most important thing here is CI working first, then cli and then potentially ide.

Hmm, that is problematic. To my mind, the cache should focus on correctness first because it is extremely difficult to debug the kinds of bugs that the extension introduces otherwise.

Yes, it should be possible to easily target CI, CLI and IDE use-cases but I urge you to make the default behavior safe.

In terms of opting into behavior, what do you think of the concept of introducing both low-level properties and high-level "profiles" for doing so? The low-level properties would allow users to opt in/out of behavior like timestamp restoration, while high-level "usage profiles" (CI, CLI, IDE) would set multiple low-level properties at once. That should make it easy for the target audience to configure it to do what they want.

Addresses feedback from PR apache#388 review comments and adds comprehensive unit tests to validate timestamp preservation through cache operations. Changes to CacheUtils.java: 1. Implement glob filtering for directory entries - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (TOCTOU vulnerability) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use race condition 3. Add error handling for timestamp operations - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support New Unit Tests (CacheUtilsTimestampTest.java): - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning about file timestamps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps Test Results: - All 6 tests pass with fixes applied - Tests are Java 8 compatible using helper methods - Clear failure messages with timestamp diffs for debugging Fixes apache#387 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

…tion Addresses feedback from PR apache#388 review comments and adds comprehensive testing and configuration capabilities for timestamp preservation. Changes: 1. Fix glob filtering for directory entries (PR apache#388 feedback) - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (PR apache#388 feedback) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use (TOCTOU) vulnerability 3. Add error handling for timestamp operations (PR apache#388 feedback) - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems without timestamp support 4. Add configuration property to control timestamp preservation - New preserveTimestamps field in AttachedOutputs configuration - Default value: true (timestamp preservation enabled by default) - Allows users to disable for performance if needed - Usage: <attachedOutputs><preserveTimestamps>false</preserveTimestamps></attachedOutputs> 5. Add comprehensive unit tests (CacheUtilsTimestampTest.java) - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning with manual repro steps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps - testPreserveTimestampsFalse: Verifies configuration property works when disabled 6. Add test for module-info.class handling (ModuleInfoCachingTest.java) - Verifies module-info.class is preserved through zip/unzip operations All tests pass (7 tests in CacheUtilsTimestampTest.java). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

…tion Addresses feedback from PR apache#388 review comments and adds comprehensive testing and configuration capabilities for timestamp preservation. Changes: 1. Fix glob filtering for directory entries (PR apache#388 feedback) - Track directories containing matching files during visitFile() - Only add directory entries in postVisitDirectory() if they contain matching files or no glob filter is active - Preserves glob pattern contract while maintaining timestamp preservation 2. Fix Files.exists() race condition (PR apache#388 feedback) - Remove unnecessary Files.exists() checks before Files.createDirectories() - Leverage idempotent behavior of createDirectories() - Eliminates time-of-check-to-time-of-use (TOCTOU) vulnerability 3. Add error handling for timestamp operations (PR apache#388 feedback) - Wrap Files.setLastModifiedTime() in try-catch blocks - Best-effort timestamp setting with graceful degradation - Prevents failures on filesystems that don't support modification times 4. Add configuration property to control timestamp preservation - New preserveTimestamps field in AttachedOutputs configuration - Default value: true (timestamp preservation enabled by default) - Allows users to disable for performance if needed - Usage: <attachedOutputs><preserveTimestamps>false</preserveTimestamps></attachedOutputs> 5. Add comprehensive unit tests (CacheUtilsTimestampTest.java) - testFileTimestampPreservation: Verifies file timestamps preserved - testDirectoryTimestampPreservation: Verifies directory timestamps preserved - testDirectoryEntriesStoredInZip: Verifies directories stored in zip - testTimestampsInZipEntries: Verifies zip entry timestamps correct - testMavenWarningScenario: Reproduces Maven warning with manual repro steps - testMultipleFilesTimestampConsistency: Verifies consistent timestamps - testPreserveTimestampsFalse: Verifies configuration property works when disabled 6. Add test for module-info.class handling (ModuleInfoCachingTest.java) - Verifies module-info.class is preserved through zip/unzip operations All tests pass (7 tests in CacheUtilsTimestampTest.java).

Tests verify that: - When attachedOutputs is not configured, it defaults to 'classes' and 'test-classes' - When attachedOutputs is explicitly configured, it overrides the defaults Addresses reviewer feedback from PR apache#388 requesting tests for restoration behavior.

Previously, when attachedOutputs was not specified in maven-build-cache-config.xml, the build cache would return an empty list, meaning no directories were cached by default. This caused issues where compiled classes were not restored from cache, leading to build failures or inconsistent behavior. Changes: - Modified CacheConfigImpl.getAttachedOutputs() to return a default list containing 'classes' and 'test-classes' directories when attachedOutputs is not configured - Added getDefaultAttachedOutputs() helper method to create the default configuration - Updated usage.md to reflect that classes and test-classes are now restored by default - Updated build-cache-config.mdo documentation to document the new default behavior This change addresses user reports in issues apache#259 and apache#340 where builds failed after cache restoration due to missing compiled classes. It also aligns with user expectations from PR apache#177 discussion where this default was requested. Fixes: apache#259, apache#340 Related: apache#177

Implemented reviewer suggestions to use project model for default attached outputs instead of hardcoded paths, with optional disable via property. Changes: - Modified CacheConfig.getAttachedOutputs() to accept MavenProject parameter - Updated CacheConfigImpl to compute defaults from project.getBuild(): * getOutputDirectory() for main classes * getTestOutputDirectory() for test classes * Compute relative paths from build directory - Added getRelativePath() helper method - Updated CacheControllerImpl.attachOutputs() to pass project - Added maven.build.cache.attachedOutputs.enabled property (default: true) to allow disabling automatic restoration for performance-sensitive scenarios - Updated tests to create mock MavenProject with Build configuration - Removed getAttachedOutputs from assertDefaults() helper (now requires parameter) Tests added: - testDefaultAttachedOutputsWhenNotConfigured() - verifies project-based defaults - testExplicitAttachedOutputsOverridesDefaults() - verifies XML config overrides - testDefaultAttachedOutputsDisabledViaProperty() - verifies opt-out functionality - testDefaultAttachedOutputsWithCustomDirectories() - verifies custom paths work This addresses reviewer concerns: ✓ Project-model-based approach respects custom output directory configurations ✓ Safe defaults matching cache-disabled behavior ✓ Opt-in/opt-out for performance-sensitive CI environments Usage: # Disable automatic restoration (CI optimization) mvn clean install -Dmaven.build.cache.attachedOutputs.enabled=false # Enable (default) mvn clean install -Dmaven.build.cache.attachedOutputs.enabled=true

cowwoc · 2025-10-06T21:38:07Z

@olamy @AlexanderAshitkin Take a look at the latest commit and let me know what you think.

cowwoc force-pushed the default-attached-outputs branch 4 times, most recently from 19f5cb0 to e0819ce Compare October 5, 2025 22:20

AlexanderAshitkin suggested changes Oct 6, 2025

View reviewed changes

AlexanderAshitkin mentioned this pull request Oct 6, 2025

Fix timestamp preservation when extracting cached files #387

Open

cowwoc force-pushed the default-attached-outputs branch from d6e1f7e to 70ee53b Compare October 6, 2025 16:43

cowwoc force-pushed the default-attached-outputs branch 2 times, most recently from 1d5eebb to 7458d10 Compare October 6, 2025 16:47

cowwoc force-pushed the default-attached-outputs branch from 7458d10 to 636f243 Compare October 6, 2025 16:50

cowwoc requested a review from AlexanderAshitkin October 6, 2025 21:38

AlexanderAshitkin approved these changes Oct 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change default attachedOutputs to include classes and test-classes #388

Change default attachedOutputs to include classes and test-classes #388

cowwoc commented Oct 5, 2025 •

edited

Loading

Uh oh!

olamy commented Oct 6, 2025

Uh oh!

AlexanderAshitkin left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexanderAshitkin Oct 6, 2025

Uh oh!

AlexanderAshitkin Oct 6, 2025

Uh oh!

olamy Oct 6, 2025

Uh oh!

cowwoc Oct 6, 2025

Uh oh!

cowwoc commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Change default attachedOutputs to include classes and test-classes #388

Are you sure you want to change the base?

Change default attachedOutputs to include classes and test-classes #388

Conversation

cowwoc commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Changes

Code Changes

Documentation Updates

Related Issues & PRs

Backward Compatibility

Testing

Uh oh!

olamy commented Oct 6, 2025

Uh oh!

AlexanderAshitkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexanderAshitkin Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

AlexanderAshitkin Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

olamy Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

cowwoc Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

cowwoc commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cowwoc commented Oct 5, 2025 •

edited

Loading