Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow literal default for allow_multiple build setting in transitions #14908

Closed
wants to merge 1 commit into from
Closed

Allow literal default for allow_multiple build setting in transitions #14908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,20 @@ public static Map<String, BuildOptions> validate(
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));
if (newValue.equals(rule.getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME))) {
Copy link
Contributor

@gregestren gregestren Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really took me some time to realize changedSettingToRule from line 310 is now a misleading name. As I understand, the reality is:

  1. changedSettingToRule was originally written to encapsulate settings the transition changed
  2. build_setting inputs are implicitly changed by transition code as an implementation detail, from non-existent to their explicit default values, to guarantee the transition can read them
  3. Trim input build settings after transition #13971 causes the original logic, which historically only checked user-changed settings, to also include these implicit changes. For the purpose of guaranteeing the default that the transition added are then removed.

I'm worried about these overloading of concepts, even if it keeps the code concise.

What if we broke out the logic some: revert https://github.com/bazelbuild/bazel/pull/13971/files, then do a separate distinct call to getRelevantStarlarkSettingsFromTransition(Settings.INPUTS) which only replicates the part of validate that removes an option if it's at its default value?

I realize that takes more refactoring, likely breaking out a helper method for validate(). But I'd really like to keep the code as clear as possible in transition logic, since it's super-complicated as it is.

At the very least I'd like the naming and commenting (including method Javadoc) to remain accurate. I worry it could be more work to clearly make the commenting accurate than do the refactoring I'm suggesting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point - input build settings should be cleared unconditionally rather than undergo validation. I created #14972 for this refactoring and a better fix for #14894.

// We make an exception for the default value here for two reasons:
// 1. It ensures that specifying a setting with allow_multiple as an input but not as
// an output of a transition does not run into the exception below.
// 2. It provides a clear way to reset the setting to its (literal) default from a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. here feels a bit API-shaky to me. Basically, a transition has to set a setting to a certain type unless it's set to a certain special value in which case it's exempt. I get the intended use case described here. But I can also see transitions that, say, read their values from attributes, which happen to set values to defaults - or not - by coincidence. i.e. whoever's setting the values has no insight into this and may be confused why some arbitrary string works while another fails.

To the point, do you think #14911 obsoletes all this complexity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it looks very hacky and is another reason to consider #14972 the better kind of fix. That said, I don't think this accidental resetting would be very realistic: The "magic" literal default value is the only valid one of its type (string rather than list of string) due to type validation and any attribute would only ever yield one type.

To the point, do you think #14911 obsoletes all this complexity?

The new representation of repeated string flags would no longer require any special handling for transitions, so we could eventually delete the entire allowsMultiple branch. But given that finalizing the design of #14911 will take some more time, either this PR or #14972 are probably needed to deal with the regression for 5.1.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on your last point - I'm happy to fast track a temporary fix for the regression. I just wanted to verify the long-term expected state.

// transition.
newValue = ImmutableList.of(newValue);
} else {
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<>();
Expand Down
57 changes: 57 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2549,4 +2549,61 @@ 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();
}
}