Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not clear --platforms on no-op change to --cpu #17158

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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> rawTransitionOutput) {
Object newCpu = rawTransitionOutput.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 rawTransitionOutput;
}
if (rawTransitionOutput.containsKey(COMMAND_LINE_OPTION_PREFIX + "platforms")) {
// Explicitly setting --platforms overrides the implicit clearing.
return rawTransitionOutput;
}
return ImmutableMap.<String, Object>builder()
.putAll(rawTransitionOutput)
.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,56 @@ 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 {
fmeum marked this conversation as resolved.
Show resolved Hide resolved
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):",
// Leave --cpu unchanged, but still trigger the full transition logic that would be
// bypassed by returning {}.
" return settings",
katre marked this conversation as resolved.
Show resolved Hide resolved
"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