Skip to content

Commit

Permalink
Update platform mapping to work on BuildOptions.
Browse files Browse the repository at this point in the history 
Remove BuildConfigurationKey from platform mapping calculations. This enables locking down access to BCK contents from other locations.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 570430837
Change-Id: I5645a6782015455ebf3513cbac60fa78b0696e57
  • Loading branch information
@katre @copybara-github
katre authored and copybara-github committed Oct 3, 2023
1 parent 91bcc09 commit 4647633
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 56 deletions.
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/config/BuildConfigurationKey.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
20 changes: 9 additions & 11 deletions src/main/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>See <a href=https://docs.google.com/document/d/1Vg_tPgiZbSrvXcJ403vZVAGlsWhH9BUDrAxMOYnO0Ls>
* the design</a> for more details on how the mapping can be defined and the desired logic on how it
Expand Down Expand Up @@ -146,7 +146,7 @@ public SkyKeyInterner<Key> getSkyKeyInterner() {
private final ImmutableMap<ImmutableSet<String>, Label> flagsToPlatforms;
private final ImmutableSet<Class<? extends FragmentOptions>> optionsClasses;
private final LoadingCache<NativeAndStarlarkFlags, OptionsParsingResult> parserCache;
private final LoadingCache<BuildConfigurationKey, BuildConfigurationKey> mappingCache;
private final LoadingCache<BuildOptions, BuildOptions> mappingCache;
private final RepositoryMapping mainRepositoryMapping;

/**
Expand Down Expand Up @@ -177,7 +177,7 @@ public SkyKeyInterner<Key> 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.
*
* <p>The <a href=https://docs.google.com/document/d/1Vg_tPgiZbSrvXcJ403vZVAGlsWhH9BUDrAxMOYnO0Ls>
* full design</a> contains the details for the mapping logic but in short:
Expand All @@ -196,7 +196,7 @@ public SkyKeyInterner<Key> 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) {
Expand All @@ -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(
Expand All @@ -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 =
Expand All @@ -254,7 +252,7 @@ private BuildConfigurationKey computeMapping(BuildConfigurationKey original)
}
}

return BuildConfigurationKey.withoutPlatformMapping(modifiedOptions);
return modifiedOptions;
}

private OptionsParsingResult parseWithCache(NativeAndStarlarkFlags args)
Expand Down
38 changes: 16 additions & 22 deletions src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingFunctionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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");
}

Expand Down Expand Up @@ -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");
}

Expand Down Expand Up @@ -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);
}
}
37 changes: 15 additions & 22 deletions src/test/java/com/google/devtools/build/lib/skyframe/config/PlatformMappingValueTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}

Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}
}

0 comments on commit 4647633

Please sign in to comment.