Skip to content

Commit

Permalink
Do not clear --platforms on no-op change to --cpu
Browse files Browse the repository at this point in the history
Resolves a TODO in the code to not unnecessarily fork the configuration
when a transition has `--cpu` as a declared output but doesn't change
it.
  • Loading branch information
fmeum committed Jan 6, 2023
1 parent e2759df commit 57fc3d2
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
}

for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
Map<String, Object> newValues = handleImplicitPlatformChange(buildOptions,
entry.getValue());
BuildOptions transitionedOptions =
applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition);
splitBuildOptions.put(entry.getKey(), transitionedOptions);
Expand Down Expand Up @@ -127,21 +128,22 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
* <p>Transitions can also explicitly set --platforms to be clear what platform they set.
*
* <p>Platform mappings: https://bazel.build/concepts/platforms-intro#platform-mappings.
*
* <p>This doesn't check that the changed value is actually different than the source (i.e.
* setting {@code --cpu=foo} when {@code --cpu} is already {@code foo}). That could unnecessarily
* fork configurations that are really the same. That's a possible optimization TODO.
*/
private static Map<String, Object> handleImplicitPlatformChange(
Map<String, Object> originalOutput) {
boolean changesCpu = originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "cpu");
boolean changesPlatforms = originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms");
return changesCpu && !changesPlatforms
? ImmutableMap.<String, Object>builder()
.putAll(originalOutput)
.put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
.build()
: originalOutput;
BuildOptions options, Map<String, Object> originalOutput) {
Object newCpu = originalOutput.get(COMMAND_LINE_OPTION_PREFIX + "cpu");
if (newCpu == null || newCpu.equals(options.get(CoreOptions.class).cpu)) {
// No effective change to --cpu, so no need to prevent the platform mapping from resetting it.
return originalOutput;
}
if (originalOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms")) {
// Explicitly setting --platforms overrides the implicit clearing.
return originalOutput;
}
return ImmutableMap.<String, Object>builder()
.putAll(originalOutput)
.put(COMMAND_LINE_OPTION_PREFIX + "platforms", ImmutableList.<Label>of())
.build();
}

private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2501,6 +2501,54 @@ public void testNoPlatformChange() throws Exception {
.containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_platform"));
}

/*
* If the transition claims to change --cpu but doesn't, it doesn't constitute a platform change
* and also doesn't affect any other options (such as affectedByStarlarkTransition).
*/
@Test
public void testCpuNoOpChangeIsFullyNoOp() throws Exception {
writeAllowlistFile();
scratch.file(
"platforms/BUILD",
"platform(name = 'my_platform',",
" parents = ['" + TestConstants.LOCAL_CONFIG_PLATFORM_PACKAGE_ROOT + ":host'],",
" constraint_values = [],",
")");
scratch.file(
"test/starlark/my_rule.bzl",
"def transition_func(settings, attr):",
" return settings",
"my_transition = transition(implementation = transition_func,",
" inputs = [",
" '//command_line_option:cpu',",
" ],",
" outputs = [",
" '//command_line_option:cpu',",
" ]",
")",
"def impl(ctx): ",
" return []",
"my_rule = rule(",
" implementation = impl,",
" attrs = {",
" 'dep': attr.label(cfg = my_transition),",
" '_allowlist_function_transition': attr.label(",
" default = '//tools/allowlists/function_transition_allowlist',",
" ),",
" })");
scratch.file(
"test/starlark/BUILD",
"load('//test/starlark:my_rule.bzl', 'my_rule')",
"my_rule(name = 'test', dep = ':main1')",
"cc_binary(name = 'main1', srcs = ['main1.c'])");

ConfiguredTarget topLevel = getConfiguredTarget("//test/starlark:test");
ConfiguredTarget dep =
getDirectPrerequisite(getConfiguredTarget("//test/starlark:test"), "//test/starlark:main1");
assertThat(getConfiguration(dep).getOptions()).isEqualTo(
getConfiguration(topLevel).getOptions());
}

@Test
public void testEffectiveNoopTransitionTrimsInputBuildSettings() throws Exception {
writeAllowlistFile();
Expand Down

0 comments on commit 57fc3d2

Please sign in to comment.