Skip to content

Commit

Permalink
Clear --platforms on Starlark transitions that set --cpu.
Browse files Browse the repository at this point in the history 
This prevents platform mappings from accidentally overwriting
transitions. See code comments for details.

Both native and Starlark transitions must do this. Native transitions added this in 8396c2f

PiperOrigin-RevId: 347425124
  • Loading branch information
@gregestren @copybara-github
gregestren authored and copybara-github committed Dec 14, 2020
1 parent 07400c0 commit d679546
Show file tree
Hide file tree
Showing 3 changed files with 300 additions and 45 deletions.
40 changes: 37 additions & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ static Map<String, BuildOptions> applyAndValidate(
StructImpl attrObject,
EventHandler eventHandler)
throws EvalException, InterruptedException {
checkForBlacklistedOptions(starlarkTransition);
checkForDenylistedOptions(starlarkTransition);

// TODO(waltl): consider building this once and use it across different split
// transitions.
Expand All @@ -91,14 +91,48 @@ static Map<String, BuildOptions> applyAndValidate(
validateFunctionOutputsMatchesDeclaredOutputs(transitions.values(), starlarkTransition);

for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
BuildOptions transitionedOptions =
applyTransition(buildOptions, entry.getValue(), optionInfoMap, starlarkTransition);
applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition);
splitBuildOptions.put(entry.getKey(), transitionedOptions);
}
return splitBuildOptions.build();
}

private static void checkForBlacklistedOptions(StarlarkDefinedConfigTransition transition)
/**
* If the transition changes --cpu but not --platforms, clear out --platforms.
*
* <p>Purpose:
*
* <ol>
* <li>A platform mapping sets --cpu=foo when --platforms=foo.
* <li>A transition sets --cpu=bar.
* <li>Because --platforms=foo, the platform mapping kicks in to set --cpu back to foo.
* <li>Result: the mapping accidentally overrides the transition
* </ol>
*
* <p>Transitions can alo explicitly set --platforms to be clear what platform they set.
*
* <p>Platform mappings:
* https://docs.bazel.build/versions/master/platforms-intro.html#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;
}

private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition)
throws EvalException {
if (transition.getOutputs().contains("//command_line_option:define")) {
throw Starlark.errorf(
Expand Down
149 changes: 127 additions & 22 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import java.util.Collection;
import java.util.List;
Expand Down Expand Up @@ -91,7 +92,6 @@ private void writeAllowlistFile() throws Exception {
}

private void writeBasicTestFiles() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();
getAnalysisMock().ccSupport().setupCcToolchainConfigForCpu(mockToolsConfig, "armeabi-v7a");
scratch.file(
Expand Down Expand Up @@ -128,7 +128,6 @@ private void writeBasicTestFiles() throws Exception {

@Test
public void testStarlarkSplitTransitionSplitAttr() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();
scratch.file(
"test/starlark/rules.bzl",
Expand Down Expand Up @@ -186,7 +185,6 @@ public void testStarlarkSplitTransitionSplitAttr() throws Exception {

@Test
public void testStarlarkListSplitTransitionSplitAttr() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();
scratch.file(
"test/starlark/rules.bzl",
Expand Down Expand Up @@ -238,7 +236,6 @@ public void testStarlarkListSplitTransitionSplitAttr() throws Exception {

@Test
public void testStarlarkPatchTransitionSplitAttr() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();
scratch.file(
"test/starlark/rules.bzl",
Expand Down Expand Up @@ -289,7 +286,6 @@ public void testStarlarkPatchTransitionSplitAttr() throws Exception {
public void testStarlarkConfigSplitAttr() throws Exception {
// This is a customized test case for b/152078818, where a starlark transition that takes a
// starlark config as input caused a failure when no custom values were provided for the config.
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();
scratch.file(
"test/starlark/rules.bzl",
Expand Down Expand Up @@ -353,7 +349,6 @@ public void testFunctionSplitTransitionCheckAttrDep() throws Exception {

@Test
public void testTargetAndRuleNotInAllowlist() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();
getAnalysisMock().ccSupport().setupCcToolchainConfigForCpu(mockToolsConfig, "armeabi-v7a");
scratch.file(
Expand Down Expand Up @@ -420,7 +415,6 @@ private void testSplitTransitionCheckAttrDep(ConfiguredTarget target) throws Exc
}

private void writeReadSettingsTestFiles() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -472,7 +466,6 @@ public void testReadSettingsSplitDepAttrDep() throws Exception {
}

private void writeOptionConversionTestFiles() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -521,7 +514,6 @@ public void testOptionConversionCpu() throws Exception {
}

private void writeReadAndPassthroughOptionsTestFiles() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -606,7 +598,6 @@ public void testCompilationModeReadableInStarlarkTransitions() throws Exception

@Test
public void testUndeclaredOptionKey() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -639,7 +630,6 @@ public void testUndeclaredOptionKey() throws Exception {

@Test
public void testDeclaredOutputNotReturned() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -676,7 +666,6 @@ public void testDeclaredOutputNotReturned() throws Exception {

@Test
public void testSettingsContainOnlyInputs() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -713,7 +702,6 @@ public void testSettingsContainOnlyInputs() throws Exception {

@Test
public void testInvalidInputKey() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -748,7 +736,6 @@ public void testInvalidInputKey() throws Exception {

@Test
public void testInvalidNativeOptionInput() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -784,7 +771,6 @@ public void testInvalidNativeOptionInput() throws Exception {

@Test
public void testInvalidNativeOptionOutput() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -821,7 +807,6 @@ public void testInvalidNativeOptionOutput() throws Exception {
public void testBannedNativeOptionOutput() throws Exception {
// Just picked an arbirtary incompatible_ flag; however, could be any flag
// besides incompatible_enable_cc_toolchain_resolution (and might not even need to be real).
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -856,7 +841,6 @@ public void testBannedNativeOptionOutput() throws Exception {

@Test
public void testAllowIncompatibleEnableCcToolchainResolution() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -901,7 +885,6 @@ public void testAllowIncompatibleEnableCcToolchainResolution() throws Exception

@Test
public void testInvalidOutputKey() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -936,7 +919,6 @@ public void testInvalidOutputKey() throws Exception {

@Test
public void testInvalidOptionValue() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -969,7 +951,6 @@ public void testInvalidOptionValue() throws Exception {

@Test
public void testDuplicateOutputs() throws Exception {
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");
writeAllowlistFile();

scratch.file(
Expand Down Expand Up @@ -1913,7 +1894,6 @@ public void starlarkSplitTransitionRequiredFragments() throws Exception {
*/
private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throws Exception {
writeAllowlistFile();
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");

String outputValue = directRead ? "settings['//command_line_option:test_arg']" : "['frisbee']";
String inputs = directRead ? "['//command_line_option:test_arg']" : "[]";
Expand Down Expand Up @@ -1974,7 +1954,6 @@ public void testNoOpTransitionLeavesSameConfig_native_setToSame() throws Excepti
private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boolean setToDefault)
throws Exception {
writeAllowlistFile();
setBuildLanguageOptions("--experimental_starlark_config_transitions=true");

String outputValue = directRead ? "settings['//test:flag']" : "'frisbee'";
String inputs = directRead ? "['//test:flag']" : "[]";
Expand Down Expand Up @@ -2047,4 +2026,130 @@ public void testOptionConversionDynamicMode() throws Exception {
public void testOptionConversionCrosstoolTop() throws Exception {
// TODO(waltl): check that crosstool_top is parsed properly.
}

/**
* Changing --cpu implicitly changes the target platform. Test that the old value of --platforms
* gets cleared out (platform mappings can then kick in to set --platforms correctly).
*/
@Test
public void testImplicitPlatformsChange() throws Exception {
writeAllowlistFile();
getAnalysisMock().ccSupport().setupCcToolchainConfigForCpu(mockToolsConfig, "armeabi-v7a");
scratch.file("platforms/BUILD", "platform(name = 'my_platform', constraint_values = [])");
scratch.file(
"test/starlark/my_rule.bzl",
"def transition_func(settings, attr):",
" return {'//command_line_option:cpu': 'armeabi-v7a'}",
"my_transition = transition(implementation = transition_func,",
" inputs = [], 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'])");

useConfiguration("--platforms=//platforms:my_platform");
ConfiguredTarget dep =
getDirectPrerequisite(getConfiguredTarget("//test/starlark:test"), "//test/starlark:main1");
// When --platforms is empty and no platform mapping triggers, PlatformMappingValue sets
// --platforms to PlatformOptions.computeTargetPlatform(), which defaults to the host.
assertThat(getConfiguration(dep).getOptions().get(PlatformOptions.class).platforms)
.containsExactly(
Label.parseAbsoluteUnchecked(TestConstants.PLATFORM_PACKAGE_ROOT + ":default_host"));
}

@Test
public void testExplicitPlatformsChange() throws Exception {
writeAllowlistFile();
getAnalysisMock().ccSupport().setupCcToolchainConfigForCpu(mockToolsConfig, "armeabi-v7a");
scratch.file(
"platforms/BUILD",
"platform(name = 'my_platform', constraint_values = [])",
"platform(name = 'my_other_platform', constraint_values = [])");
scratch.file(
"test/starlark/my_rule.bzl",
"def transition_func(settings, attr):",
" return {",
" '//command_line_option:cpu': 'armeabi-v7a',",
" '//command_line_option:platforms': ['//platforms:my_other_platform']",
" }",
"my_transition = transition(implementation = transition_func,",
" inputs = [],",
" outputs = [",
" '//command_line_option:cpu',",
" '//command_line_option:platforms'",
" ]",
")",
"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'])");

useConfiguration("--platforms=//platforms:my_platform");
ConfiguredTarget dep =
getDirectPrerequisite(getConfiguredTarget("//test/starlark:test"), "//test/starlark:main1");
assertThat(getConfiguration(dep).getOptions().get(PlatformOptions.class).platforms)
.containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_other_platform"));
}

/* If the transition doesn't change --cpu, it doesn't constitute a platform change. */
@Test
public void testNoPlatformChange() throws Exception {
writeAllowlistFile();
scratch.file("platforms/BUILD", "platform(name = 'my_platform', constraint_values = [])");
scratch.file(
"test/starlark/my_rule.bzl",
"def transition_func(settings, attr):",
" return {",
" '//command_line_option:test_arg': ['blah'],",
" }",
"my_transition = transition(implementation = transition_func,",
" inputs = [],",
" outputs = [",
" '//command_line_option:test_arg',",
" ]",
")",
"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'])");

useConfiguration("--platforms=//platforms:my_platform");
ConfiguredTarget dep =
getDirectPrerequisite(getConfiguredTarget("//test/starlark:test"), "//test/starlark:main1");
assertThat(getConfiguration(dep).getOptions().get(PlatformOptions.class).platforms)
.containsExactly(Label.parseAbsoluteUnchecked("//platforms:my_platform"));
}
}
Loading

0 comments on commit d679546

Please sign in to comment.