Skip to content

Commit 95de355

Browse files
fmeumfmeum
authored
Do not validate input-only settings in transitions (bazelbuild#15048)
Starlark transition logic temporarily explicitly sets all input build settings of a transition to their defaults. Since bazelbuild#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 bazelbuild#14894 Closes bazelbuild#14972. PiperOrigin-RevId: 434589143
1 parent 1345938 commit 95de355

File tree

2 files changed

+250
-72
lines changed

2 files changed

+250
-72
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java‎

Lines changed: 114 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -267,31 +267,39 @@ public static Map<String, BuildOptions> validate(
267267
Map<PackageValue.Key, PackageValue> buildSettingPackages,
268268
Map<String, BuildOptions> toOptions)
269269
throws TransitionException {
270-
// Collect settings changed during this transition and their types. This includes settings that
271-
// were only used as inputs as to the transition and thus had their default values added to the
272-
// fromOptions, which in case of a no-op transition directly end up in toOptions.
273-
Map<Label, Rule> changedSettingToRule = Maps.newHashMap();
270+
// Collect settings that are inputs or outputs of the transition together with their types.
271+
// Output setting values will be validated and removed if set to their default.
272+
Map<Label, Rule> inputAndOutputSettingsToRule = Maps.newHashMap();
273+
// Collect settings that were only used as inputs to the transition and thus possibly had their
274+
// default values added to the fromOptions. They will be removed if set to ther default, but
275+
// should not be validated.
276+
Set<Label> inputOnlySettings = Sets.newHashSet();
274277
root.visit(
275278
(StarlarkTransitionVisitor)
276279
transition -> {
277-
ImmutableSet<Label> changedSettings =
280+
ImmutableSet<Label> inputAndOutputSettings =
278281
getRelevantStarlarkSettingsFromTransition(
279282
transition, Settings.INPUTS_AND_OUTPUTS);
280-
for (Label setting : changedSettings) {
281-
changedSettingToRule.put(
283+
ImmutableSet<Label> outputSettings =
284+
getRelevantStarlarkSettingsFromTransition(transition, Settings.OUTPUTS);
285+
for (Label setting : inputAndOutputSettings) {
286+
inputAndOutputSettingsToRule.put(
282287
setting, getActual(buildSettingPackages, setting).getAssociatedRule());
288+
if (!outputSettings.contains(setting)) {
289+
inputOnlySettings.add(setting);
290+
}
283291
}
284292
});
285293

286-
// Return early if no starlark settings were affected.
287-
if (changedSettingToRule.isEmpty()) {
294+
// Return early if the transition has neither inputs nor outputs (rare).
295+
if (inputAndOutputSettingsToRule.isEmpty()) {
288296
return toOptions;
289297
}
290298

291299
ImmutableMap.Builder<Label, Label> aliasToActualBuilder = new ImmutableMap.Builder<>();
292-
for (Map.Entry<Label, Rule> changedSettingWithRule : changedSettingToRule.entrySet()) {
293-
Label setting = changedSettingWithRule.getKey();
294-
Rule rule = changedSettingWithRule.getValue();
300+
for (Map.Entry<Label, Rule> settingWithRule : inputAndOutputSettingsToRule.entrySet()) {
301+
Label setting = settingWithRule.getKey();
302+
Rule rule = settingWithRule.getValue();
295303
if (!rule.getLabel().equals(setting)) {
296304
aliasToActualBuilder.put(setting, rule.getLabel());
297305
}
@@ -307,69 +315,28 @@ public static Map<String, BuildOptions> validate(
307315
BuildOptions.Builder cleanedOptions = null;
308316
// Clean up aliased values.
309317
BuildOptions options = unalias(entry.getValue(), aliasToActual);
310-
for (Map.Entry<Label, Rule> changedSettingWithRule : changedSettingToRule.entrySet()) {
318+
for (Map.Entry<Label, Rule> settingWithRule : inputAndOutputSettingsToRule.entrySet()) {
311319
// If the build setting was referenced in the transition via an alias, this is that alias
312-
Label maybeAliasSetting = changedSettingWithRule.getKey();
313-
Rule rule = changedSettingWithRule.getValue();
314-
// If the build setting was *not* referenced in the transition by an alias, this is the same
315-
// value as {@code maybeAliasSetting} above.
320+
Label maybeAliasSetting = settingWithRule.getKey();
321+
Rule rule = settingWithRule.getValue();
316322
Label actualSetting = rule.getLabel();
317-
Object newValue = options.getStarlarkOptions().get(actualSetting);
318-
// TODO(b/154132845): fix NPE occasionally observed here.
319-
Preconditions.checkState(
320-
newValue != null,
321-
"Error while attempting to validate new values from starlark"
322-
+ " transition(s) with the outputs %s. Post-transition configuration should include"
323-
+ " '%s' but only includes starlark options: %s. If you run into this error"
324-
+ " please ping b/154132845 or email [email protected].",
325-
changedSettingToRule.keySet(),
326-
actualSetting,
327-
options.getStarlarkOptions().keySet());
328-
boolean allowsMultiple = rule.getRuleClassObject().getBuildSetting().allowsMultiple();
329-
if (allowsMultiple) {
330-
// if this setting allows multiple settings
331-
if (!(newValue instanceof List)) {
332-
throw new TransitionException(
333-
String.format(
334-
"'%s' allows multiple values and must be set"
335-
+ " in transition using a starlark list instead of single value '%s'",
336-
actualSetting, newValue));
337-
}
338-
List<?> rawNewValueAsList = (List<?>) newValue;
339-
List<Object> convertedValue = new ArrayList<>();
340-
Type<?> type = rule.getRuleClassObject().getBuildSetting().getType();
341-
for (Object value : rawNewValueAsList) {
342-
try {
343-
convertedValue.add(type.convert(value, maybeAliasSetting));
344-
} catch (ConversionException e) {
345-
throw new TransitionException(e);
346-
}
347-
}
348-
if (convertedValue.equals(
349-
ImmutableList.of(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)))) {
350-
if (cleanedOptions == null) {
351-
cleanedOptions = options.toBuilder();
352-
}
353-
cleanedOptions.removeStarlarkOption(rule.getLabel());
354-
}
355-
} else {
356-
// if this setting does not allow multiple settings
357-
Object convertedValue;
358-
try {
359-
convertedValue =
360-
rule.getRuleClassObject()
361-
.getBuildSetting()
362-
.getType()
363-
.convert(newValue, maybeAliasSetting);
364-
} catch (ConversionException e) {
365-
throw new TransitionException(e);
366-
}
367-
if (convertedValue.equals(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME))) {
368-
if (cleanedOptions == null) {
369-
cleanedOptions = options.toBuilder();
370-
}
371-
cleanedOptions.removeStarlarkOption(rule.getLabel());
323+
// Input-only settings may have had their literal default value added to the BuildOptions
324+
// so that the transition can read them. We have to remove these explicitly set value here
325+
// to preserve the invariant that Starlark settings at default values are not explicitly set
326+
// in the BuildOptions.
327+
final boolean isInputOnlySettingAtDefault =
328+
inputOnlySettings.contains(maybeAliasSetting)
329+
&& rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)
330+
.equals(options.getStarlarkOptions().get(actualSetting));
331+
// For output settings, the raw value returned by the transition first has to be validated
332+
// and converted to the proper type before it can be compared to the default value.
333+
if (isInputOnlySettingAtDefault
334+
|| validateAndCheckIfAtDefault(
335+
rule, options, maybeAliasSetting, inputAndOutputSettingsToRule.keySet())) {
336+
if (cleanedOptions == null) {
337+
cleanedOptions = options.toBuilder();
372338
}
339+
cleanedOptions.removeStarlarkOption(rule.getLabel());
373340
}
374341
}
375342
// 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(
381348
return cleanedOptionMap.build();
382349
}
383350

351+
/**
352+
* Validate the value of a particular build setting after a transition has been applied.
353+
*
354+
* @param buildSettingRule the build setting to validate.
355+
* @param options the {@link BuildOptions} reflecting the post-transition configuration.
356+
* @param maybeAliasSetting the label used to refer to the build setting in the transition,
357+
* possibly an alias. This is only used for error messages.
358+
* @param inputAndOutputSettings the transition input and output settings. This is only used for
359+
* error messages.
360+
* @return {@code true} if and only if the setting is set to its default value after the
361+
* transition.
362+
* @throws TransitionException if the value returned by the transition for this setting has an
363+
* invalid type.
364+
*/
365+
private static boolean validateAndCheckIfAtDefault(
366+
Rule buildSettingRule,
367+
BuildOptions options,
368+
Label maybeAliasSetting,
369+
Set<Label> inputAndOutputSettings)
370+
throws TransitionException {
371+
// If the build setting was *not* referenced in the transition by an alias, this is the same
372+
// value as {@code maybeAliasSetting}.
373+
Label actualSetting = buildSettingRule.getLabel();
374+
Object newValue = options.getStarlarkOptions().get(actualSetting);
375+
// TODO(b/154132845): fix NPE occasionally observed here.
376+
Preconditions.checkState(
377+
newValue != null,
378+
"Error while attempting to validate new values from starlark"
379+
+ " transition(s) with the inputs and outputs %s. Post-transition configuration should"
380+
+ " include '%s' but only includes starlark options: %s. If you run into this error"
381+
+ " please ping b/154132845 or email [email protected].",
382+
inputAndOutputSettings,
383+
actualSetting,
384+
options.getStarlarkOptions().keySet());
385+
boolean allowsMultiple =
386+
buildSettingRule.getRuleClassObject().getBuildSetting().allowsMultiple();
387+
if (allowsMultiple) {
388+
// if this setting allows multiple settings
389+
if (!(newValue instanceof List)) {
390+
throw new TransitionException(
391+
String.format(
392+
"'%s' allows multiple values and must be set"
393+
+ " in transition using a starlark list instead of single value '%s'",
394+
actualSetting, newValue));
395+
}
396+
List<?> rawNewValueAsList = (List<?>) newValue;
397+
List<Object> convertedValue = new ArrayList<>();
398+
Type<?> type = buildSettingRule.getRuleClassObject().getBuildSetting().getType();
399+
for (Object value : rawNewValueAsList) {
400+
try {
401+
convertedValue.add(type.convert(value, maybeAliasSetting));
402+
} catch (ConversionException e) {
403+
throw new TransitionException(e);
404+
}
405+
}
406+
return convertedValue.equals(
407+
ImmutableList.of(buildSettingRule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)));
408+
} else {
409+
// if this setting does not allow multiple settings
410+
Object convertedValue;
411+
try {
412+
convertedValue =
413+
buildSettingRule
414+
.getRuleClassObject()
415+
.getBuildSetting()
416+
.getType()
417+
.convert(newValue, maybeAliasSetting);
418+
} catch (ConversionException e) {
419+
throw new TransitionException(e);
420+
}
421+
return convertedValue.equals(
422+
buildSettingRule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME));
423+
}
424+
}
425+
384426
/*
385427
* Resolve aliased build setting issues
386428
*

‎src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2489,4 +2489,140 @@ public void testExplicitNoopTransitionTrimsInputBuildSettings() throws Exception
24892489
new EventBus());
24902490
assertNoEvents();
24912491
}
2492+
2493+
@Test
2494+
public void testTransitionPreservesAllowMultipleDefault() throws Exception {
2495+
writeAllowlistFile();
2496+
scratch.file(
2497+
"test/starlark/rules.bzl",
2498+
"P = provider(fields = ['value'])",
2499+
"def _s_impl(ctx):",
2500+
" return [P(value = ctx.build_setting_value)]",
2501+
"def _t_impl(settings, attr):",
2502+
" if 'foo' in settings['//test/starlark:a']:",
2503+
" return {'//test/starlark:b': ['bar']}",
2504+
" else:",
2505+
" return {'//test/starlark:b': ['baz']}",
2506+
"def _r_impl(ctx):",
2507+
" pass",
2508+
"s = rule(",
2509+
" implementation = _s_impl,",
2510+
" build_setting = config.string(allow_multiple = True, flag = True),",
2511+
")",
2512+
"t = transition(",
2513+
" implementation = _t_impl,",
2514+
" inputs = ['//test/starlark:a'],",
2515+
" outputs = ['//test/starlark:b'],",
2516+
")",
2517+
"r = rule(",
2518+
" implementation = _r_impl,",
2519+
" attrs = {",
2520+
" 'setting': attr.label(cfg = t),",
2521+
" '_allowlist_function_transition': attr.label(",
2522+
" default = '//tools/allowlists/function_transition_allowlist',",
2523+
" ),",
2524+
" },",
2525+
")");
2526+
scratch.file(
2527+
"test/starlark/BUILD",
2528+
"load(':rules.bzl', 's', 'r')",
2529+
"s(",
2530+
" name = 'a',",
2531+
" build_setting_default = '',",
2532+
")",
2533+
"s(",
2534+
" name = 'b',",
2535+
" build_setting_default = '',",
2536+
")",
2537+
"r(",
2538+
" name = 'c',",
2539+
" setting = ':b',",
2540+
")");
2541+
update(
2542+
ImmutableList.of("//test/starlark:c"),
2543+
/*keepGoing=*/ false,
2544+
LOADING_PHASE_THREADS,
2545+
/*doAnalysis=*/ true,
2546+
new EventBus());
2547+
assertNoEvents();
2548+
}
2549+
2550+
@Test
2551+
public void testTransitionPreservesNonDefaultInputOnlySetting() throws Exception {
2552+
writeAllowlistFile();
2553+
scratch.file(
2554+
"test/starlark/rules.bzl",
2555+
"def _string_impl(ctx):",
2556+
" return []",
2557+
"string_flag = rule(",
2558+
" implementation = _string_impl,",
2559+
" build_setting = config.string(flag = True),",
2560+
")",
2561+
"def _transition_impl(settings, attr):",
2562+
" return {",
2563+
" '//test/starlark:output_only': settings['//test/starlark:input_only'],",
2564+
" }",
2565+
"_transition = transition(",
2566+
" implementation = _transition_impl,",
2567+
" inputs = [",
2568+
" '//test/starlark:input_only',",
2569+
" ],",
2570+
" outputs = [",
2571+
" '//test/starlark:output_only',",
2572+
" ],",
2573+
")",
2574+
"def _apply_transition_impl(ctx):",
2575+
" ctx.actions.symlink(",
2576+
" output = ctx.outputs.out,",
2577+
" target_file = ctx.file.target,",
2578+
" )",
2579+
" return [DefaultInfo(executable = ctx.outputs.out)]",
2580+
"apply_transition = rule(",
2581+
" implementation = _apply_transition_impl,",
2582+
" attrs = {",
2583+
" 'target': attr.label(",
2584+
" cfg = _transition,",
2585+
" allow_single_file = True,",
2586+
" ),",
2587+
" 'out': attr.output(),",
2588+
" '_allowlist_function_transition': attr.label(",
2589+
" default = '//tools/allowlists/function_transition_allowlist',",
2590+
" ),",
2591+
" },",
2592+
" executable = False,",
2593+
")");
2594+
scratch.file(
2595+
"test/starlark/BUILD",
2596+
"load('//test/starlark:rules.bzl', 'apply_transition', 'string_flag')",
2597+
"",
2598+
"string_flag(",
2599+
" name = 'input_only',",
2600+
" build_setting_default = 'input_only_default',",
2601+
")",
2602+
"string_flag(",
2603+
" name = 'output_only',",
2604+
" build_setting_default = 'output_only_default',",
2605+
")",
2606+
"cc_binary(",
2607+
" name = 'main',",
2608+
" srcs = ['main.cc'],",
2609+
")",
2610+
"apply_transition(",
2611+
" name = 'transitioned_main',",
2612+
" target = ':main',",
2613+
" out = 'main_out',",
2614+
")");
2615+
2616+
useConfiguration(ImmutableMap.of("//test/starlark:input_only", "not_the_default"));
2617+
Label inputOnlySetting = Label.parseAbsoluteUnchecked("//test/starlark:input_only");
2618+
ConfiguredTarget transitionedDep =
2619+
getDirectPrerequisite(
2620+
getConfiguredTarget("//test/starlark:transitioned_main"), "//test/starlark:main");
2621+
assertThat(
2622+
getConfiguration(transitionedDep)
2623+
.getOptions()
2624+
.getStarlarkOptions()
2625+
.get(inputOnlySetting))
2626+
.isEqualTo("not_the_default");
2627+
}
24922628
}

0 commit comments

Comments
 (0)