Skip to content

Commit 70ee53b

Browse files
claudeclaude
committed
Add project-model-based defaults with opt-in/opt-out for attachedOutputs
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
1 parent e0819ce commit 70ee53b

File tree

4 files changed

+146
-8
lines changed

4 files changed

+146
-8
lines changed

‎src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -930,7 +930,7 @@ public void attachGeneratedSources(MavenProject project) throws IOException {
930930
}
931931

932932
private void attachOutputs(MavenProject project) throws IOException {
933-
final List<DirName> attachedDirs = cacheConfig.getAttachedOutputs();
933+
final List<DirName> attachedDirs = cacheConfig.getAttachedOutputs(project);
934934
for (DirName dir : attachedDirs) {
935935
final Path targetDir = Paths.get(project.getBuild().getDirectory());
936936
final Path outputDir = targetDir.resolve(dir.getValue());

‎src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.apache.maven.model.Plugin;
3636
import org.apache.maven.model.PluginExecution;
3737
import org.apache.maven.plugin.MojoExecution;
38+
import org.apache.maven.project.MavenProject;
3839

3940
/**
4041
* A java interface to the information configured in the maven-build-cache-config.xml file
@@ -106,7 +107,7 @@ public interface CacheConfig {
106107

107108
String getLocalRepositoryLocation();
108109

109-
List<DirName> getAttachedOutputs();
110+
List<DirName> getAttachedOutputs(MavenProject project);
110111

111112
boolean adjustMetaInfVersion();
112113

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

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
import org.apache.maven.model.Plugin;
6868
import org.apache.maven.model.PluginExecution;
6969
import org.apache.maven.plugin.MojoExecution;
70+
import org.apache.maven.project.MavenProject;
7071
import org.apache.maven.rtinfo.RuntimeInformation;
7172
import org.slf4j.Logger;
7273
import org.slf4j.LoggerFactory;
@@ -95,6 +96,7 @@ public class CacheConfigImpl implements org.apache.maven.buildcache.xml.CacheCon
9596
public static final String LAZY_RESTORE_PROPERTY_NAME = "maven.build.cache.lazyRestore";
9697
public static final String RESTORE_ON_DISK_ARTIFACTS_PROPERTY_NAME = "maven.build.cache.restoreOnDiskArtifacts";
9798
public static final String RESTORE_GENERATED_SOURCES_PROPERTY_NAME = "maven.build.cache.restoreGeneratedSources";
99+
public static final String ATTACHED_OUTPUTS_ENABLED_PROPERTY_NAME = "maven.build.cache.attachedOutputs.enabled";
98100
public static final String ALWAYS_RUN_PLUGINS = "maven.build.cache.alwaysRunPlugins";
99101
public static final String MANDATORY_CLEAN = "maven.build.cache.mandatoryClean";
100102

@@ -572,29 +574,52 @@ public String getLocalRepositoryLocation() {
572574
}
573575

574576
@Override
575-
public List<DirName> getAttachedOutputs() {
577+
public List<DirName> getAttachedOutputs(MavenProject project) {
576578
checkInitializedState();
577579
final AttachedOutputs attachedOutputs = getConfiguration().getAttachedOutputs();
578580
if (attachedOutputs == null) {
579-
return getDefaultAttachedOutputs();
581+
return getDefaultAttachedOutputs(project);
580582
}
581583
return attachedOutputs.getDirNames();
582584
}
583585

584-
private List<DirName> getDefaultAttachedOutputs() {
586+
private List<DirName> getDefaultAttachedOutputs(MavenProject project) {
587+
// Check if default attachedOutputs are enabled (default: true)
588+
boolean enabled = getProperty(ATTACHED_OUTPUTS_ENABLED_PROPERTY_NAME, true);
589+
if (!enabled) {
590+
return Collections.emptyList();
591+
}
592+
585593
List<DirName> defaults = new ArrayList<>();
586594

595+
// Get output directories from project build configuration
596+
String buildDirectory = project.getBuild().getDirectory();
597+
String outputDirectory = project.getBuild().getOutputDirectory();
598+
String testOutputDirectory = project.getBuild().getTestOutputDirectory();
599+
600+
// Compute relative paths from build directory
601+
String classesRelative = getRelativePath(buildDirectory, outputDirectory);
602+
String testClassesRelative = getRelativePath(buildDirectory, testOutputDirectory);
603+
587604
DirName classes = new DirName();
588-
classes.setValue("classes");
605+
classes.setValue(classesRelative);
589606
defaults.add(classes);
590607

591608
DirName testClasses = new DirName();
592-
testClasses.setValue("test-classes");
609+
testClasses.setValue(testClassesRelative);
593610
defaults.add(testClasses);
594611

595612
return defaults;
596613
}
597614

615+
private String getRelativePath(String basePath, String fullPath) {
616+
// Convert to Path objects and compute relative path
617+
java.nio.file.Path base = java.nio.file.Paths.get(basePath);
618+
java.nio.file.Path full = java.nio.file.Paths.get(fullPath);
619+
java.nio.file.Path relative = base.relativize(full);
620+
return relative.toString();
621+
}
622+
598623
@Override
599624
public boolean adjustMetaInfVersion() {
600625
if (isEnabled()) {

‎src/test/java/org/apache/maven/buildcache/xml/CacheConfigImplTest.java‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.Arrays;
3232
import java.util.Collections;
3333
import java.util.HashMap;
34+
import java.util.List;
3435
import java.util.Map;
3536
import java.util.Optional;
3637
import java.util.Properties;
@@ -39,13 +40,17 @@
3940
import org.apache.commons.lang3.tuple.Pair;
4041
import org.apache.maven.buildcache.DefaultPluginScanConfig;
4142
import org.apache.maven.buildcache.hash.HashFactory;
43+
import org.apache.maven.buildcache.xml.config.AttachedOutputs;
4244
import org.apache.maven.buildcache.xml.config.Configuration;
45+
import org.apache.maven.buildcache.xml.config.DirName;
4346
import org.apache.maven.buildcache.xml.config.Remote;
4447
import org.apache.maven.execution.MavenExecutionRequest;
4548
import org.apache.maven.execution.MavenSession;
49+
import org.apache.maven.model.Build;
4650
import org.apache.maven.model.Plugin;
4751
import org.apache.maven.model.PluginExecution;
4852
import org.apache.maven.plugin.MojoExecution;
53+
import org.apache.maven.project.MavenProject;
4954
import org.apache.maven.rtinfo.RuntimeInformation;
5055
import org.junit.jupiter.api.BeforeEach;
5156
import org.junit.jupiter.api.Test;
@@ -169,7 +174,7 @@ private void assertDefaults(Map<String, Runnable> overrides) {
169174
asserts.put("calculateProjectVersionChecksum", () -> assertFalse(testObject.calculateProjectVersionChecksum()));
170175
asserts.put("canIgnore", () -> assertFalse(testObject.canIgnore(mock(MojoExecution.class))));
171176
asserts.put("getAlwaysRunPlugins", () -> assertNull(testObject.getAlwaysRunPlugins()));
172-
asserts.put("getAttachedOutputs", () -> assertEquals(Collections.emptyList(), testObject.getAttachedOutputs()));
177+
// getAttachedOutputs removed - requires MavenProject parameter, tested separately
173178
asserts.put("getBaselineCacheUrl", () -> assertNull(testObject.getBaselineCacheUrl()));
174179
asserts.put("getDefaultGlob", () -> assertEquals("*", testObject.getDefaultGlob()));
175180
asserts.put(
@@ -482,4 +487,111 @@ void testRemoveSaveFinalIgnoredWhenRemoteSaveDisabled() {
482487
Pair.of("getUrl", () -> assertEquals("dummy.url.xyz", testObject.getUrl())),
483488
Pair.of("isRemoteCacheEnabled", () -> assertTrue(testObject.isRemoteCacheEnabled())));
484489
}
490+
491+
@Test
492+
void testDefaultAttachedOutputsWhenNotConfigured() {
493+
// When attachedOutputs is not configured in XML, should return default list
494+
Configuration configuration = new Configuration();
495+
// Deliberately not setting attachedOutputs
496+
testCacheConfig.setConfiguration(configuration);
497+
498+
assertEquals(CacheState.INITIALIZED, testObject.initialize());
499+
500+
// Create mock project with default build configuration
501+
MavenProject mockProject = mock(MavenProject.class);
502+
Build mockBuild = mock(Build.class);
503+
when(mockProject.getBuild()).thenReturn(mockBuild);
504+
when(mockBuild.getDirectory()).thenReturn("/project/target");
505+
when(mockBuild.getOutputDirectory()).thenReturn("/project/target/classes");
506+
when(mockBuild.getTestOutputDirectory()).thenReturn("/project/target/test-classes");
507+
508+
List<DirName> attachedOutputs = testObject.getAttachedOutputs(mockProject);
509+
assertEquals(2, attachedOutputs.size(), "Should have 2 default attached outputs");
510+
511+
List<String> dirNames = attachedOutputs.stream()
512+
.map(DirName::getValue)
513+
.collect(Collectors.toList());
514+
515+
assertTrue(dirNames.contains("classes"), "Should include 'classes' directory by default");
516+
assertTrue(dirNames.contains("test-classes"), "Should include 'test-classes' directory by default");
517+
}
518+
519+
@Test
520+
void testExplicitAttachedOutputsOverridesDefaults() {
521+
// When attachedOutputs is explicitly configured, should use those values instead of defaults
522+
Configuration configuration = new Configuration();
523+
AttachedOutputs attachedOutputs = new AttachedOutputs();
524+
525+
DirName customDir = new DirName();
526+
customDir.setValue("custom-output");
527+
attachedOutputs.addDirName(customDir);
528+
529+
configuration.setAttachedOutputs(attachedOutputs);
530+
testCacheConfig.setConfiguration(configuration);
531+
532+
assertEquals(CacheState.INITIALIZED, testObject.initialize());
533+
534+
// Create mock project (not used when explicit config is set, but required by interface)
535+
MavenProject mockProject = mock(MavenProject.class);
536+
Build mockBuild = mock(Build.class);
537+
when(mockProject.getBuild()).thenReturn(mockBuild);
538+
when(mockBuild.getDirectory()).thenReturn("/project/target");
539+
when(mockBuild.getOutputDirectory()).thenReturn("/project/target/classes");
540+
when(mockBuild.getTestOutputDirectory()).thenReturn("/project/target/test-classes");
541+
542+
List<DirName> result = testObject.getAttachedOutputs(mockProject);
543+
assertEquals(1, result.size(), "Should have 1 explicitly configured output");
544+
assertEquals("custom-output", result.get(0).getValue(),
545+
"Should use explicitly configured directory, not defaults");
546+
}
547+
548+
@Test
549+
void testDefaultAttachedOutputsDisabledViaProperty() {
550+
// When attachedOutputs.enabled property is false, should return empty list
551+
Configuration configuration = new Configuration();
552+
testCacheConfig.setConfiguration(configuration);
553+
554+
when(mockProperties.getProperty(CacheConfigImpl.ATTACHED_OUTPUTS_ENABLED_PROPERTY_NAME))
555+
.thenReturn("false");
556+
557+
assertEquals(CacheState.INITIALIZED, testObject.initialize());
558+
559+
// Create mock project
560+
MavenProject mockProject = mock(MavenProject.class);
561+
Build mockBuild = mock(Build.class);
562+
when(mockProject.getBuild()).thenReturn(mockBuild);
563+
when(mockBuild.getDirectory()).thenReturn("/project/target");
564+
when(mockBuild.getOutputDirectory()).thenReturn("/project/target/classes");
565+
when(mockBuild.getTestOutputDirectory()).thenReturn("/project/target/test-classes");
566+
567+
List<DirName> attachedOutputs = testObject.getAttachedOutputs(mockProject);
568+
assertEquals(0, attachedOutputs.size(), "Should return empty list when disabled via property");
569+
}
570+
571+
@Test
572+
void testDefaultAttachedOutputsWithCustomDirectories() {
573+
// When project has custom output directories, should use those
574+
Configuration configuration = new Configuration();
575+
testCacheConfig.setConfiguration(configuration);
576+
577+
assertEquals(CacheState.INITIALIZED, testObject.initialize());
578+
579+
// Create mock project with custom output directories
580+
MavenProject mockProject = mock(MavenProject.class);
581+
Build mockBuild = mock(Build.class);
582+
when(mockProject.getBuild()).thenReturn(mockBuild);
583+
when(mockBuild.getDirectory()).thenReturn("/project/build");
584+
when(mockBuild.getOutputDirectory()).thenReturn("/project/build/custom-classes");
585+
when(mockBuild.getTestOutputDirectory()).thenReturn("/project/build/custom-test-classes");
586+
587+
List<DirName> attachedOutputs = testObject.getAttachedOutputs(mockProject);
588+
assertEquals(2, attachedOutputs.size(), "Should have 2 default attached outputs");
589+
590+
List<String> dirNames = attachedOutputs.stream()
591+
.map(DirName::getValue)
592+
.collect(Collectors.toList());
593+
594+
assertTrue(dirNames.contains("custom-classes"), "Should include custom output directory");
595+
assertTrue(dirNames.contains("custom-test-classes"), "Should include custom test output directory");
596+
}
485597
}

0 commit comments

Comments
 (0)