diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/BuildConfigurationKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/BuildConfigurationKey.java index a805b56ec75009..050f79a5bca8eb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/config/BuildConfigurationKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/config/BuildConfigurationKey.java @@ -42,7 +42,8 @@ public final class BuildConfigurationKey implements SkyKey { public static BuildConfigurationKey withPlatformMapping( PlatformMappingValue platformMappingValue, BuildOptions options) throws OptionsParsingException { - return platformMappingValue.map(withoutPlatformMapping(options)); + BuildOptions mappedOptions = platformMappingValue.map(options); + return BuildConfigurationKey.withoutPlatformMapping(mappedOptions); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java index 5ca978c3e7cd64..6e2b173a941a89 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java @@ -48,8 +48,8 @@ import javax.annotation.Nullable; /** - * Stores contents of a platforms/flags mapping file for transforming one {@link - * BuildConfigurationKey} into another. + * Stores contents of a platforms/flags mapping file for transforming one {@link BuildOptions} into + * another. * *

See * the design for more details on how the mapping can be defined and the desired logic on how it @@ -146,7 +146,7 @@ public SkyKeyInterner getSkyKeyInterner() { private final ImmutableMap, Label> flagsToPlatforms; private final ImmutableSet> optionsClasses; private final LoadingCache parserCache; - private final LoadingCache mappingCache; + private final LoadingCache mappingCache; private final RepositoryMapping mainRepositoryMapping; /** @@ -177,7 +177,7 @@ public SkyKeyInterner getSkyKeyInterner() { } /** - * Maps one {@link BuildConfigurationKey} to another by way of mappings provided in a file. + * Maps one {@link BuildOptions} to another by way of mappings provided in a file. * *

The * full design contains the details for the mapping logic but in short: @@ -196,7 +196,7 @@ public SkyKeyInterner getSkyKeyInterner() { * @throws IllegalArgumentException if the original does not contain a {@link PlatformOptions} * fragment */ - public BuildConfigurationKey map(BuildConfigurationKey original) throws OptionsParsingException { + public BuildOptions map(BuildOptions original) throws OptionsParsingException { try { return mappingCache.get(original); } catch (CompletionException e) { @@ -205,13 +205,11 @@ public BuildConfigurationKey map(BuildConfigurationKey original) throws OptionsP } } - private BuildConfigurationKey computeMapping(BuildConfigurationKey original) - throws OptionsParsingException { - BuildOptions originalOptions = original.getOptions(); + private BuildOptions computeMapping(BuildOptions originalOptions) throws OptionsParsingException { if (originalOptions.hasNoConfig()) { // The empty configuration (produced by NoConfigTransition) is terminal: it'll never change. - return original; + return originalOptions; } checkArgument( @@ -228,7 +226,7 @@ private BuildConfigurationKey computeMapping(BuildConfigurationKey original) if (!platformsToFlags.containsKey(targetPlatform)) { // This can happen if the user has set the platform and any other flags that would normally // be mapped from it on the command line instead of relying on the mapping. - return original; + return originalOptions; } modifiedOptions = @@ -254,7 +252,7 @@ private BuildConfigurationKey computeMapping(BuildConfigurationKey original) } } - return BuildConfigurationKey.withoutPlatformMapping(modifiedOptions); + return modifiedOptions; } private OptionsParsingResult parseWithCache(NativeAndStarlarkFlags args) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java index 4c26a9f7ed0db8..a20c44579157d3 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java @@ -88,11 +88,9 @@ public void testMappingFileDoesNotExistDefaultLocation() throws Exception { PlatformMappingValue platformMappingValue = executeFunction(PlatformMappingValue.Key.create(null)); - BuildConfigurationKey key = BuildConfigurationKey.withoutPlatformMapping(defaultBuildOptions); + BuildOptions mapped = platformMappingValue.map(defaultBuildOptions); - BuildConfigurationKey mapped = platformMappingValue.map(key); - - assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) + assertThat(mapped.get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @@ -121,9 +119,9 @@ public void testMappingFileIsRead() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -143,9 +141,9 @@ public void testMappingFileIsRead_fromAlternatePackagePath() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -169,9 +167,9 @@ public void handlesNoWorkspaceFile() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -190,9 +188,9 @@ public void multiplePackagePaths() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -216,9 +214,9 @@ public void multiplePackagePathsFirstWins() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); } // Internal flags (OptionMetadataTag.INTERNAL) cannot be set from the command-line, but @@ -237,9 +235,9 @@ public void ableToChangeInternalOption() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(DummyTestFragment.DummyTestOptions.class).internalFoo) + assertThat(mapped.get(DummyTestFragment.DummyTestOptions.class).internalFoo) .isEqualTo("something_new"); } @@ -269,9 +267,9 @@ public void starlarkFlagMapping() throws Exception { BuildOptions modifiedOptions = defaultBuildOptions.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = platformMappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = platformMappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().getStarlarkOptions()) + assertThat(mapped.getStarlarkOptions()) .containsExactly(Label.parseCanonical("//test:my_string_flag"), "mapped_value"); } @@ -388,8 +386,4 @@ private PlatformMappingValue executeFunction(PlatformMappingValue.Key key) throw } return result.get(key); } - - private static BuildConfigurationKey keyForOptions(BuildOptions modifiedOptions) { - return BuildConfigurationKey.withoutPlatformMapping(modifiedOptions); - } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java index bdc702c97a90b0..d4e33e51772435 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java @@ -62,12 +62,9 @@ public void testMapNoMappings() throws OptionsParsingException { new PlatformMappingValue( platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS, REPO_MAPPING); - BuildConfigurationKey key = - BuildConfigurationKey.withoutPlatformMapping(DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); + BuildOptions mapped = mappingValue.map(DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS); - BuildConfigurationKey mapped = mappingValue.map(key); - - assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) + assertThat(mapped.get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @@ -87,9 +84,9 @@ public void testMapPlatformToFlags() throws Exception { BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(PlatformOptions.class).platforms = ImmutableList.of(PLATFORM1); - BuildConfigurationKey mapped = mappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(CoreOptions.class).cpu).isEqualTo("one"); + assertThat(mapped.get(CoreOptions.class).cpu).isEqualTo("one"); } @Test @@ -106,9 +103,9 @@ public void testMapFlagsToPlatform() throws Exception { modifiedOptions.get(CoreOptions.class).cpu = "one"; modifiedOptions.get(CoreOptions.class).compilationMode = CompilationMode.DBG; - BuildConfigurationKey mapped = mappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(PlatformOptions.class).platforms).containsExactly(PLATFORM1); + assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM1); } @Test @@ -126,9 +123,9 @@ public void testMapFlagsToPlatformPriority() throws Exception { BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "foo"; - BuildConfigurationKey mapped = mappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(PlatformOptions.class).platforms).containsExactly(PLATFORM2); + assertThat(mapped.get(PlatformOptions.class).platforms).containsExactly(PLATFORM2); } @Test @@ -144,9 +141,9 @@ public void testMapFlagsToPlatformNoneMatching() throws Exception { BuildOptions modifiedOptions = DEFAULT_BUILD_CONFIG_PLATFORM_OPTIONS.clone(); modifiedOptions.get(CoreOptions.class).cpu = "bar"; - BuildConfigurationKey mapped = mappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(mapped.getOptions().get(PlatformOptions.class).platforms) + assertThat(mapped.get(PlatformOptions.class).platforms) .containsExactly(DEFAULT_TARGET_PLATFORM); } @@ -162,7 +159,7 @@ public void testMapNoPlatformOptions() throws Exception { BuildOptions options = BuildOptions.of(ImmutableList.of()); - assertThrows(IllegalArgumentException.class, () -> mappingValue.map(keyForOptions(options))); + assertThrows(IllegalArgumentException.class, () -> mappingValue.map(options)); } @Test @@ -186,9 +183,9 @@ public void testMapNoMappingIfPlatformIsSetButNotMatching() throws Exception { BUILD_CONFIG_PLATFORM_OPTIONS, RepositoryMapping.ALWAYS_FALLBACK); - BuildConfigurationKey mapped = mappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(keyForOptions(modifiedOptions)).isEqualTo(mapped); + assertThat(modifiedOptions).isEqualTo(mapped); } @Test @@ -205,9 +202,9 @@ public void testMapNoMappingIfPlatformIsSetAndNoPlatformMapping() throws Excepti new PlatformMappingValue( platformsToFlags, flagsToPlatforms, BUILD_CONFIG_PLATFORM_OPTIONS, REPO_MAPPING); - BuildConfigurationKey mapped = mappingValue.map(keyForOptions(modifiedOptions)); + BuildOptions mapped = mappingValue.map(modifiedOptions); - assertThat(keyForOptions(modifiedOptions)).isEqualTo(mapped); + assertThat(modifiedOptions).isEqualTo(mapped); } @Test @@ -226,8 +223,4 @@ public void testCustomKey() { assertThat(key.getWorkspaceRelativeMappingPath()).isEqualTo(PathFragment.create("my/path")); assertThat(key.wasExplicitlySetByUser()).isTrue(); } - - private static BuildConfigurationKey keyForOptions(BuildOptions modifiedOptions) { - return BuildConfigurationKey.withoutPlatformMapping(modifiedOptions); - } }