Skip to content

Commit

Permalink
Do not validate input-only settings in transitions (#15048)
Browse files Browse the repository at this point in the history
Starlark transition logic temporarily explicitly sets all input build
settings of a transition to their defaults. Since #13971, these values
are cleared after the transition. However, since then they have also
been subject to type validation, which is not only unnecessary, but
also breaks in the special case of a string build setting with
allow_multiple.

With this commit, input-only build settings are unconditionally
stripped from the post-transition BuildOptions and do not undergo
validation. This is made possible by a refactoring of
`StarlarkTransition#validate` that extracts the validation logic into a
separate function and also improves some variable names.

Fixes #14894

Closes #14972.

PiperOrigin-RevId: 434589143
  • Loading branch information
fmeum authored Mar 15, 2022
1 parent 1345938 commit 95de355
Show file tree
Hide file tree
Showing 2 changed files with 250 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -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.
Expand All @@ -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
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}

0 comments on commit 95de355

Please sign in to comment.