diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
index 0cf754541bcfd0..7fffe2e7d215cf 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
@@ -267,31 +267,39 @@ public static Map<String, BuildOptions> validate(
Map<PackageValue.Key, PackageValue> buildSettingPackages,
Map<String, BuildOptions> toOptions)
throws TransitionException {
- // Collect settings changed during this transition and their types. This includes settings that
- // were only used as inputs as to the transition and thus had their default values added to the
- // fromOptions, which in case of a no-op transition directly end up in toOptions.
- Map<Label, Rule> changedSettingToRule = Maps.newHashMap();
+ // Collect settings that are inputs or outputs of the transition together with their types.
+ // Output setting values will be validated and removed if set to their default.
+ Map<Label, Rule> inputAndOutputSettingsToRule = Maps.newHashMap();
+ // Collect settings that were only used as inputs to the transition and thus possibly had their
+ // default values added to the fromOptions. They will be removed if set to ther default, but
+ // should not be validated.
+ Set<Label> inputOnlySettings = Sets.newHashSet();
root.visit(
(StarlarkTransitionVisitor)
transition -> {
- ImmutableSet<Label> changedSettings =
+ ImmutableSet<Label> inputAndOutputSettings =
getRelevantStarlarkSettingsFromTransition(
transition, Settings.INPUTS_AND_OUTPUTS);
- for (Label setting : changedSettings) {
- changedSettingToRule.put(
+ ImmutableSet<Label> outputSettings =
+ getRelevantStarlarkSettingsFromTransition(transition, Settings.OUTPUTS);
+ for (Label setting : inputAndOutputSettings) {
+ inputAndOutputSettingsToRule.put(
setting, getActual(buildSettingPackages, setting).getAssociatedRule());
+ if (!outputSettings.contains(setting)) {
+ inputOnlySettings.add(setting);
+ }
}
});
- // Return early if no starlark settings were affected.
- if (changedSettingToRule.isEmpty()) {
+ // Return early if the transition has neither inputs nor outputs (rare).
+ if (inputAndOutputSettingsToRule.isEmpty()) {
return toOptions;
}
ImmutableMap.Builder<Label, Label> aliasToActualBuilder = new ImmutableMap.Builder<>();
- for (Map.Entry<Label, Rule> changedSettingWithRule : changedSettingToRule.entrySet()) {
- Label setting = changedSettingWithRule.getKey();
- Rule rule = changedSettingWithRule.getValue();
+ for (Map.Entry<Label, Rule> settingWithRule : inputAndOutputSettingsToRule.entrySet()) {
+ Label setting = settingWithRule.getKey();
+ Rule rule = settingWithRule.getValue();
if (!rule.getLabel().equals(setting)) {
aliasToActualBuilder.put(setting, rule.getLabel());
}
@@ -307,69 +315,28 @@ public static Map<String, BuildOptions> validate(
BuildOptions.Builder cleanedOptions = null;
// Clean up aliased values.
BuildOptions options = unalias(entry.getValue(), aliasToActual);
- for (Map.Entry<Label, Rule> changedSettingWithRule : changedSettingToRule.entrySet()) {
+ for (Map.Entry<Label, Rule> settingWithRule : inputAndOutputSettingsToRule.entrySet()) {
// If the build setting was referenced in the transition via an alias, this is that alias
- Label maybeAliasSetting = changedSettingWithRule.getKey();
- Rule rule = changedSettingWithRule.getValue();
- // If the build setting was *not* referenced in the transition by an alias, this is the same
- // value as {@code maybeAliasSetting} above.
+ Label maybeAliasSetting = settingWithRule.getKey();
+ Rule rule = settingWithRule.getValue();
Label actualSetting = rule.getLabel();
- Object newValue = options.getStarlarkOptions().get(actualSetting);
- // TODO(b/154132845): fix NPE occasionally observed here.
- Preconditions.checkState(
- newValue != null,
- "Error while attempting to validate new values from starlark"
- + " transition(s) with the outputs %s. Post-transition configuration should include"
- + " '%s' but only includes starlark options: %s. If you run into this error"
- + " please ping b/154132845 or email blaze-configurability@google.com.",
- changedSettingToRule.keySet(),
- actualSetting,
- options.getStarlarkOptions().keySet());
- boolean allowsMultiple = rule.getRuleClassObject().getBuildSetting().allowsMultiple();
- if (allowsMultiple) {
- // if this setting allows multiple settings
- if (!(newValue instanceof List)) {
- throw new TransitionException(
- String.format(
- "'%s' allows multiple values and must be set"
- + " in transition using a starlark list instead of single value '%s'",
- actualSetting, newValue));
- }
- List<?> rawNewValueAsList = (List<?>) newValue;
- List<Object> convertedValue = new ArrayList<>();
- Type<?> type = rule.getRuleClassObject().getBuildSetting().getType();
- for (Object value : rawNewValueAsList) {
- try {
- convertedValue.add(type.convert(value, maybeAliasSetting));
- } catch (ConversionException e) {
- throw new TransitionException(e);
- }
- }
- if (convertedValue.equals(
- ImmutableList.of(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)))) {
- if (cleanedOptions == null) {
- cleanedOptions = options.toBuilder();
- }
- cleanedOptions.removeStarlarkOption(rule.getLabel());
- }
- } else {
- // if this setting does not allow multiple settings
- Object convertedValue;
- try {
- convertedValue =
- rule.getRuleClassObject()
- .getBuildSetting()
- .getType()
- .convert(newValue, maybeAliasSetting);
- } catch (ConversionException e) {
- throw new TransitionException(e);
- }
- if (convertedValue.equals(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME))) {
- if (cleanedOptions == null) {
- cleanedOptions = options.toBuilder();
- }
- cleanedOptions.removeStarlarkOption(rule.getLabel());
+ // Input-only settings may have had their literal default value added to the BuildOptions
+ // so that the transition can read them. We have to remove these explicitly set value here
+ // to preserve the invariant that Starlark settings at default values are not explicitly set
+ // in the BuildOptions.
+ final boolean isInputOnlySettingAtDefault =
+ inputOnlySettings.contains(maybeAliasSetting)
+ && rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)
+ .equals(options.getStarlarkOptions().get(actualSetting));
+ // For output settings, the raw value returned by the transition first has to be validated
+ // and converted to the proper type before it can be compared to the default value.
+ if (isInputOnlySettingAtDefault
+ || validateAndCheckIfAtDefault(
+ rule, options, maybeAliasSetting, inputAndOutputSettingsToRule.keySet())) {
+ if (cleanedOptions == null) {
+ cleanedOptions = options.toBuilder();
}
+ cleanedOptions.removeStarlarkOption(rule.getLabel());
}
}
// Keep the same instance if we didn't do anything to maintain reference equality later on.
@@ -381,6 +348,81 @@ public static Map<String, BuildOptions> validate(
return cleanedOptionMap.build();
}
+ /**
+ * Validate the value of a particular build setting after a transition has been applied.
+ *
+ * @param buildSettingRule the build setting to validate.
+ * @param options the {@link BuildOptions} reflecting the post-transition configuration.
+ * @param maybeAliasSetting the label used to refer to the build setting in the transition,
+ * possibly an alias. This is only used for error messages.
+ * @param inputAndOutputSettings the transition input and output settings. This is only used for
+ * error messages.
+ * @return {@code true} if and only if the setting is set to its default value after the
+ * transition.
+ * @throws TransitionException if the value returned by the transition for this setting has an
+ * invalid type.
+ */
+ private static boolean validateAndCheckIfAtDefault(
+ Rule buildSettingRule,
+ BuildOptions options,
+ Label maybeAliasSetting,
+ Set<Label> inputAndOutputSettings)
+ throws TransitionException {
+ // If the build setting was *not* referenced in the transition by an alias, this is the same
+ // value as {@code maybeAliasSetting}.
+ Label actualSetting = buildSettingRule.getLabel();
+ Object newValue = options.getStarlarkOptions().get(actualSetting);
+ // TODO(b/154132845): fix NPE occasionally observed here.
+ Preconditions.checkState(
+ newValue != null,
+ "Error while attempting to validate new values from starlark"
+ + " transition(s) with the inputs and outputs %s. Post-transition configuration should"
+ + " include '%s' but only includes starlark options: %s. If you run into this error"
+ + " please ping b/154132845 or email blaze-configurability@google.com.",
+ inputAndOutputSettings,
+ actualSetting,
+ options.getStarlarkOptions().keySet());
+ boolean allowsMultiple =
+ buildSettingRule.getRuleClassObject().getBuildSetting().allowsMultiple();
+ if (allowsMultiple) {
+ // if this setting allows multiple settings
+ if (!(newValue instanceof List)) {
+ throw new TransitionException(
+ String.format(
+ "'%s' allows multiple values and must be set"
+ + " in transition using a starlark list instead of single value '%s'",
+ actualSetting, newValue));
+ }
+ List<?> rawNewValueAsList = (List<?>) newValue;
+ List<Object> convertedValue = new ArrayList<>();
+ Type<?> type = buildSettingRule.getRuleClassObject().getBuildSetting().getType();
+ for (Object value : rawNewValueAsList) {
+ try {
+ convertedValue.add(type.convert(value, maybeAliasSetting));
+ } catch (ConversionException e) {
+ throw new TransitionException(e);
+ }
+ }
+ return convertedValue.equals(
+ ImmutableList.of(buildSettingRule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)));
+ } else {
+ // if this setting does not allow multiple settings
+ Object convertedValue;
+ try {
+ convertedValue =
+ buildSettingRule
+ .getRuleClassObject()
+ .getBuildSetting()
+ .getType()
+ .convert(newValue, maybeAliasSetting);
+ } catch (ConversionException e) {
+ throw new TransitionException(e);
+ }
+ return convertedValue.equals(
+ buildSettingRule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME));
+ }
+ }
+
/*
* Resolve aliased build setting issues
*
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index bb7211909ccc71..682d7c8984d8ac 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -2489,4 +2489,140 @@ public void testExplicitNoopTransitionTrimsInputBuildSettings() throws Exception
new EventBus());
assertNoEvents();
}
+
+ @Test
+ public void testTransitionPreservesAllowMultipleDefault() throws Exception {
+ writeAllowlistFile();
+ scratch.file(
+ "test/starlark/rules.bzl",
+ "P = provider(fields = ['value'])",
+ "def _s_impl(ctx):",
+ " return [P(value = ctx.build_setting_value)]",
+ "def _t_impl(settings, attr):",
+ " if 'foo' in settings['//test/starlark:a']:",
+ " return {'//test/starlark:b': ['bar']}",
+ " else:",
+ " return {'//test/starlark:b': ['baz']}",
+ "def _r_impl(ctx):",
+ " pass",
+ "s = rule(",
+ " implementation = _s_impl,",
+ " build_setting = config.string(allow_multiple = True, flag = True),",
+ ")",
+ "t = transition(",
+ " implementation = _t_impl,",
+ " inputs = ['//test/starlark:a'],",
+ " outputs = ['//test/starlark:b'],",
+ ")",
+ "r = rule(",
+ " implementation = _r_impl,",
+ " attrs = {",
+ " 'setting': attr.label(cfg = t),",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " },",
+ ")");
+ scratch.file(
+ "test/starlark/BUILD",
+ "load(':rules.bzl', 's', 'r')",
+ "s(",
+ " name = 'a',",
+ " build_setting_default = '',",
+ ")",
+ "s(",
+ " name = 'b',",
+ " build_setting_default = '',",
+ ")",
+ "r(",
+ " name = 'c',",
+ " setting = ':b',",
+ ")");
+ update(
+ ImmutableList.of("//test/starlark:c"),
+ /*keepGoing=*/ false,
+ LOADING_PHASE_THREADS,
+ /*doAnalysis=*/ true,
+ new EventBus());
+ assertNoEvents();
+ }
+
+ @Test
+ public void testTransitionPreservesNonDefaultInputOnlySetting() throws Exception {
+ writeAllowlistFile();
+ scratch.file(
+ "test/starlark/rules.bzl",
+ "def _string_impl(ctx):",
+ " return []",
+ "string_flag = rule(",
+ " implementation = _string_impl,",
+ " build_setting = config.string(flag = True),",
+ ")",
+ "def _transition_impl(settings, attr):",
+ " return {",
+ " '//test/starlark:output_only': settings['//test/starlark:input_only'],",
+ " }",
+ "_transition = transition(",
+ " implementation = _transition_impl,",
+ " inputs = [",
+ " '//test/starlark:input_only',",
+ " ],",
+ " outputs = [",
+ " '//test/starlark:output_only',",
+ " ],",
+ ")",
+ "def _apply_transition_impl(ctx):",
+ " ctx.actions.symlink(",
+ " output = ctx.outputs.out,",
+ " target_file = ctx.file.target,",
+ " )",
+ " return [DefaultInfo(executable = ctx.outputs.out)]",
+ "apply_transition = rule(",
+ " implementation = _apply_transition_impl,",
+ " attrs = {",
+ " 'target': attr.label(",
+ " cfg = _transition,",
+ " allow_single_file = True,",
+ " ),",
+ " 'out': attr.output(),",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " },",
+ " executable = False,",
+ ")");
+ scratch.file(
+ "test/starlark/BUILD",
+ "load('//test/starlark:rules.bzl', 'apply_transition', 'string_flag')",
+ "",
+ "string_flag(",
+ " name = 'input_only',",
+ " build_setting_default = 'input_only_default',",
+ ")",
+ "string_flag(",
+ " name = 'output_only',",
+ " build_setting_default = 'output_only_default',",
+ ")",
+ "cc_binary(",
+ " name = 'main',",
+ " srcs = ['main.cc'],",
+ ")",
+ "apply_transition(",
+ " name = 'transitioned_main',",
+ " target = ':main',",
+ " out = 'main_out',",
+ ")");
+
+ useConfiguration(ImmutableMap.of("//test/starlark:input_only", "not_the_default"));
+ Label inputOnlySetting = Label.parseAbsoluteUnchecked("//test/starlark:input_only");
+ ConfiguredTarget transitionedDep =
+ getDirectPrerequisite(
+ getConfiguredTarget("//test/starlark:transitioned_main"), "//test/starlark:main");
+ assertThat(
+ getConfiguration(transitionedDep)
+ .getOptions()
+ .getStarlarkOptions()
+ .get(inputOnlySetting))
+ .isEqualTo("not_the_default");
+ }
}