Skip to content

Commit

Permalink
Make no-op starlark transition not affect the output directory.
Browse files Browse the repository at this point in the history 
Not having this logic in was leading to c++ action conflicts. Before this CL we naively hashed the map of options->values returned by a transition into the output directory fragment. Now we only update the output directory fragment if values actually change. In order to do this, we also need to add the default values of all the output options to the starlark options map (if they don't already have a non-default value in the map) to take care of the set-to-default case also being a no-op. Add a bunch of tests and delete tests that showed the undesirable behavior before.

Also fixes #11196

TESTED: patch unknown commit && devblaze build //googlex/koi/...
PiperOrigin-RevId: 327116054
  • Loading branch information
@juliexxia @aiuto
juliexxia authored and aiuto committed Aug 20, 2020
1 parent 889bc0b commit d6b9469
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 122 deletions.
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -419,8 +419,7 @@ public static Map<String, BuildOptions> applyTransition(
if (doesStarlarkTransition) {
fromOptions =
addDefaultStarlarkOptions(
fromOptions,
StarlarkTransition.getDefaultInputValues(buildSettingPackages, transition));
fromOptions, StarlarkTransition.getDefaultValues(buildSettingPackages, transition));
}

// TODO(bazel-team): Add safety-check that this never mutates fromOptions.
Expand Down
38 changes: 30 additions & 8 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 @@ -238,18 +238,28 @@ private static BuildOptions applyTransition(
StarlarkDefinedConfigTransition starlarkTransition)
throws EvalException {
BuildOptions buildOptions = buildOptionsToTransition.clone();
// The names and values of options that are different after this transition.
HashMap<String, Object> convertedNewValues = new HashMap<>();
for (Map.Entry<String, Object> entry : newValues.entrySet()) {
String optionName = entry.getKey();
Object optionValue = entry.getValue();

if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
buildOptions =
BuildOptions.builder()
.merge(buildOptions)
.addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue)
.build();
convertedNewValues.put(optionName, optionValue);
Object oldValue =
buildOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName));
if ((oldValue == null && optionValue != null)
|| (oldValue != null && optionValue == null)
|| (oldValue != null && !oldValue.equals(optionValue))) {
// TODO(bazel-team): Figure out if we need to create a whole new build options every
// time. Can we just keep track of the running changes and actually build a new build
// options after this loop?
buildOptions =
BuildOptions.builder()
.merge(buildOptions)
.addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue)
.build();
convertedNewValues.put(optionName, optionValue);
}
} else {
optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());

Expand Down Expand Up @@ -298,8 +308,15 @@ private static BuildOptions applyTransition(
} else {
throw Starlark.errorf("Invalid value type for option '%s'", optionName);
}
field.set(options, convertedValue);
convertedNewValues.put(entry.getKey(), convertedValue);

Object oldValue = field.get(options);
if ((oldValue == null && convertedValue != null)
|| (oldValue != null && convertedValue == null)
|| (oldValue != null && !oldValue.equals(convertedValue))) {
field.set(options, convertedValue);
convertedNewValues.put(entry.getKey(), convertedValue);
}

} catch (IllegalArgumentException e) {
throw Starlark.errorf(
"IllegalArgumentError for option '%s': %s", optionName, e.getMessage());
Expand Down Expand Up @@ -342,6 +359,11 @@ private static BuildOptions applyTransition(
// makes it so that two configurations that are the same in value may hash differently.
private static void updateOutputDirectoryNameFragment(
Set<String> changedOptions, Map<String, OptionInfo> optionInfoMap, BuildOptions toOptions) {
// Return without doing anything if this transition hasn't changed any option values.
if (changedOptions.isEmpty()) {
return;
}

CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class);
Set<String> updatedAffectedByStarlarkTransition =
new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition);
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,8 @@ private static BuildOptions unalias(
}

/**
* For a given transition, find all Starlark build settings that are read while applying it, then
* return a map of their label to their default values.
* For a given transition, find all Starlark build settings that are input/output while applying
* it, then return a map of their label to their default values.
*
* <p>If the build setting is referenced by an {@link com.google.devtools.build.lib.rules.Alias},
* the returned map entry is still keyed by the alias.
Expand All @@ -404,15 +404,16 @@ private static BuildOptions unalias(
* build settings *written* by relevant transitions) so do not iterate over for input
* packages.
*/
public static ImmutableMap<Label, Object> getDefaultInputValues(
public static ImmutableMap<Label, Object> getDefaultValues(
Map<PackageValue.Key, PackageValue> buildSettingPackages, ConfigurationTransition root)
throws TransitionException {
ImmutableMap.Builder<Label, Object> defaultValues = new ImmutableMap.Builder<>();
root.visit(
(StarlarkTransitionVisitor)
transition -> {
ImmutableSet<Label> settings =
getRelevantStarlarkSettingsFromTransition(transition, Settings.INPUTS);
getRelevantStarlarkSettingsFromTransition(
transition, Settings.INPUTS_AND_OUTPUTS);
for (Label setting : settings) {
defaultValues.put(
setting,
Expand Down
237 changes: 131 additions & 106 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 @@ -1161,60 +1161,6 @@ public void testOutputDirHash_noop_emptyReturns() throws Exception {
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
}

// Test that a no-op starlark transition to the top level configuration results in a
// different configuration.
// TODO(bazel-team): This can be optimized. Make these the same configuration.
@Test
public void testOutputDirHash_noop_changeToDifferentStateAsTopLevel() throws Exception {
writeAllowlistFile();
scratch.file(
"test/transitions.bzl",
"def _bar_impl(settings, attr):",
" return {'//test:bar': 'barsball'}",
"bar_transition = transition(implementation = _bar_impl, inputs = [],",
" outputs = ['//test:bar'])");
scratch.file(
"test/rules.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"load('//test:transitions.bzl', 'bar_transition')",
"def _impl(ctx):",
" return MyInfo(dep = ctx.attr.dep)",
"my_rule = rule(",
" implementation = _impl,",
" attrs = {",
" 'dep': attr.label(cfg = bar_transition), ",
" '_allowlist_function_transition': attr.label(",
" default = '//tools/allowlists/function_transition_allowlist',",
" ),",
" })",
"def _basic_impl(ctx):",
" return []",
"string_flag = rule(",
" implementation = _basic_impl,",
" build_setting = config.string(flag = True),",
")",
"simple = rule(_basic_impl)");
scratch.file(
"test/BUILD",
"load('//test:rules.bzl', 'my_rule', 'string_flag', 'simple')",
"my_rule(name = 'test', dep = ':dep')",
"simple(name = 'dep')",
"string_flag(name = 'bar', build_setting_default = '')");

// Use configuration that is same as what the transition will change.
useConfiguration(ImmutableMap.of("//test:bar", "barsball"));

ConfiguredTarget test = getConfiguredTarget("//test");

@SuppressWarnings("unchecked")
ConfiguredTarget dep =
Iterables.getOnlyElement(
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));

assertThat(getCoreOptions(test).transitionDirectoryNameFragment).isNull();
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull();
}

// Test that setting all starlark options back to default != null hash of top level.
// We could set some starlark options on the command line but we don't count this as a starlark
// transition to the command line configuration will always have a null values for
Expand Down Expand Up @@ -1276,58 +1222,6 @@ public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefaultCom
assertThat(getCoreOptions(dep).transitionDirectoryNameFragment).isNotNull();
}

// Test that setting a starlark option to default (if it was already at default) doesn't
// produce the same hash. This is because we do hashing before scrubbing default values
// out of {@code BuildOptions}.
// TODO(bazel-team): This can be optimized. Make these the same configuration.
@Test
public void testOutputDirHash_multipleStarlarkOptionTransitions_backToDefault() throws Exception {
writeAllowlistFile();
scratch.file(
"test/transitions.bzl",
"def _foo_two_impl(settings, attr):",
" return {'//test:foo': 'foosballerina'}",
"foo_two_transition = transition(implementation = _foo_two_impl, inputs = [],",
" outputs = ['//test:foo'])");
scratch.file(
"test/rules.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"load('//test:transitions.bzl', 'foo_two_transition')",
"def _impl(ctx):",
" return MyInfo(dep = ctx.attr.dep)",
"my_rule = rule(",
" implementation = _impl,",
" attrs = {",
" 'dep': attr.label(cfg = foo_two_transition), ",
" '_allowlist_function_transition': attr.label(",
" default = '//tools/allowlists/function_transition_allowlist',",
" ),",
" })",
"def _basic_impl(ctx):",
" return []",
"string_flag = rule(",
" implementation = _basic_impl,",
" build_setting = config.string(flag = True),",
")",
"simple = rule(_basic_impl)");
scratch.file(
"test/BUILD",
"load('//test:rules.bzl', 'my_rule', 'string_flag', 'simple')",
"my_rule(name = 'test', dep = ':dep')",
"simple(name = 'dep')",
"string_flag(name = 'foo', build_setting_default = 'foosballerina')");

ConfiguredTarget test = getConfiguredTarget("//test");

@SuppressWarnings("unchecked")
ConfiguredTarget dep =
Iterables.getOnlyElement(
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));

assertThat(getCoreOptions(dep).transitionDirectoryNameFragment)
.isNotEqualTo(getCoreOptions(test).transitionDirectoryNameFragment);
}

/** See comment above {@link FunctionTransitionUtil#updateOutputDirectoryNameFragment} */
// TODO(bazel-team): This can be optimized. Make these the same configuration.
@Test
Expand Down Expand Up @@ -1841,6 +1735,137 @@ public void starlarkSplitTransitionRequiredFragments() throws Exception {
.containsExactly("CppOptions");
}

/**
* @param directRead if set to true, reads the output value directly from the input dict, else
* just passes in the same value as a string
*/
private void testNoOpTransitionLeavesSameConfig_native(boolean directRead) throws Exception {
writeAllowlistFile();
setStarlarkSemanticsOptions("--experimental_starlark_config_transitions=true");

String outputValue = directRead ? "settings['//command_line_option:test_arg']" : "['frisbee']";
String inputs = directRead ? "['//command_line_option:test_arg']" : "[]";

scratch.file(
"test/defs.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"def _transition_impl(settings, attr):",
" return {'//command_line_option:test_arg': " + outputValue + "}",
"my_transition = transition(",
" implementation = _transition_impl,",
" inputs = " + inputs + ",",
" outputs = ['//command_line_option:test_arg'],",
")",
"def _impl(ctx):",
" return MyInfo(dep = ctx.attr.dep)",
"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/BUILD",
"load('//test:defs.bzl', 'my_rule')",
"my_rule(name = 'test', dep = ':dep')",
"my_rule(name = 'dep')");

useConfiguration("--test_arg=frisbee");
ConfiguredTarget test = getConfiguredTarget("//test");

@SuppressWarnings("unchecked")
ConfiguredTarget dep =
Iterables.getOnlyElement(
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
}

@Test
public void testNoOpTransitionLeavesSameConfig_native_directRead() throws Exception {
testNoOpTransitionLeavesSameConfig_native(true);
}

@Test
public void testNoOpTransitionLeavesSameConfig_native_setToSame() throws Exception {
testNoOpTransitionLeavesSameConfig_native(false);
}

/**
* @param directRead if set to true, reads the output value directly from the input dict, else
* just passes in the same value as a string
* @param setToDefault if set to true, value getting passed through the transition is the default
* value of the build settings. Internally we don't keep default values in the build settings
* map inside {@link BuildOptions} so it's nice to test this separately.
*/
private void testNoOpTransitionLeavesSameConfig_starlark(boolean directRead, boolean setToDefault)
throws Exception {
writeAllowlistFile();
setStarlarkSemanticsOptions("--experimental_starlark_config_transitions=true");

String outputValue = directRead ? "settings['//test:flag']" : "'frisbee'";
String inputs = directRead ? "['//test:flag']" : "[]";
String buildSettingsDefault = setToDefault ? "frisbee" : "waterpolo";

scratch.file(
"test/defs.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"def _flag_impl(ctx):",
" return []",
"my_flag = rule(",
" implementation = _flag_impl,",
" build_setting = config.string(flag = True)",
")",
"def _transition_impl(settings, attr):",
" return {'//test:flag': " + outputValue + "}",
"my_transition = transition(",
" implementation = _transition_impl,",
" inputs = " + inputs + ",",
" outputs = ['//test:flag'],",
")",
"def _impl(ctx):",
" return MyInfo(dep = ctx.attr.dep)",
"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/BUILD",
"load('//test:defs.bzl', 'my_rule', 'my_flag')",
"my_rule(name = 'test', dep = ':dep')",
"my_rule(name = 'dep')",
"my_flag(name = 'flag', build_setting_default = '" + buildSettingsDefault + "')");

useConfiguration(ImmutableMap.of("//test:flag", "frisbee"));
ConfiguredTarget test = getConfiguredTarget("//test");

@SuppressWarnings("unchecked")
ConfiguredTarget dep =
Iterables.getOnlyElement(
(List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
assertThat(getCoreOptions(test).transitionDirectoryNameFragment)
.isEqualTo(getCoreOptions(dep).transitionDirectoryNameFragment);
}

@Test
public void testNoOpTransitionLeavesSameConfig_starlark_directRead() throws Exception {
testNoOpTransitionLeavesSameConfig_starlark(true, false);
}

@Test
public void testNoOpTransitionLeavesSameConfig_starlark_setToSame() throws Exception {
testNoOpTransitionLeavesSameConfig_starlark(false, false);
}

@Test
public void testNoOpTransitionLeavesSameConfig_starlark_setToDefault() throws Exception {
testNoOpTransitionLeavesSameConfig_starlark(false, true);
}

@Test
public void testOptionConversionDynamicMode() throws Exception {
// TODO(waltl): check that dynamic_mode is parsed properly.
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1600,12 +1600,12 @@ public void testPrecompiledFilesFromDifferentConfigs() throws Exception {
"def _impl(settings, attr):",
" _ignore = (settings, attr)",
" return [",
" {'//command_line_option:cpu': 'k8'},",
" {'//command_line_option:test_arg': ['foo']},",
" ]",
"cpu_transition = transition(",
" implementation = _impl,",
" inputs = [],",
" outputs = ['//command_line_option:cpu'],",
" outputs = ['//command_line_option:test_arg'],",
")",
"def _transitioned_file_impl(ctx):",
" return DefaultInfo(files = depset([ctx.file.src]))",
Expand Down

0 comments on commit d6b9469

Please sign in to comment.