Skip to content

Commit

Permalink
Allow string_list flags to be set via repeated flag uses (#15943)
Browse files Browse the repository at this point in the history
For all parts of Bazel other than option parsing, string build setting
flags with `allow_multiple = True` should behave just like `string_list`
settings, even though they are fundamentally of a different type. This
requires a lot of special casing all over the code base and also causes
confusing user-facing behavior when transitioning on such settings.

This commit deprecates the `allow_multiple` named parameter of
`config.string` and as a replacement introduces a new named parameter
`repeatable` on `config.string_list`. Settings with the latter set to True
behave exactly like `string` settings with `allow_multiple = True` with
respect to command-line option parsing and exactly like ordinary
`string_list` flags in all other situations.

Fixes #13817

Closes #14911.

PiperOrigin-RevId: 462146752
Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56

Co-authored-by: Fabian Meumertzheim <[email protected]>
  • Loading branch information
ckolli5 and fmeum authored Jul 22, 2022
1 parent aaf19a8 commit 9c2c3de
Show file tree
Hide file tree
Showing 8 changed files with 173 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.packages.BuildSetting;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.Starlark;

Expand All @@ -40,12 +41,15 @@ public BuildSetting boolSetting(Boolean flag) {

@Override
public BuildSetting stringSetting(Boolean flag, Boolean allowMultiple) {
return BuildSetting.create(flag, STRING, allowMultiple);
return BuildSetting.create(flag, STRING, allowMultiple, false);
}

@Override
public BuildSetting stringListSetting(Boolean flag) {
return BuildSetting.create(flag, STRING_LIST);
public BuildSetting stringListSetting(Boolean flag, Boolean repeatable) throws EvalException {
if (repeatable && !flag) {
throw Starlark.errorf("'repeatable' can only be set for a setting with 'flag = True'");
}
return BuildSetting.create(flag, STRING_LIST, false, repeatable);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,25 @@ public class BuildSetting implements BuildSettingApi {
private final boolean isFlag;
private final Type<?> type;
private final boolean allowMultiple;
private final boolean repeatable;

private BuildSetting(boolean isFlag, Type<?> type, boolean allowMultiple) {
private BuildSetting(boolean isFlag, Type<?> type, boolean allowMultiple, boolean repeatable) {
this.isFlag = isFlag;
this.type = type;
this.allowMultiple = allowMultiple;
this.repeatable = repeatable;
}

public static BuildSetting create(boolean isFlag, Type<?> type, boolean allowMultiple) {
return new BuildSetting(isFlag, type, allowMultiple);
public static BuildSetting create(
boolean isFlag, Type<?> type, boolean allowMultiple, boolean repeatable) {
return new BuildSetting(isFlag, type, allowMultiple, repeatable);
}

public static BuildSetting create(boolean isFlag, Type<?> type) {
Preconditions.checkState(
type.getLabelClass() != LabelClass.DEPENDENCY,
"Build settings should not create a dependency with their default attribute");
return new BuildSetting(isFlag, type, /* allowMultiple= */ false);
return new BuildSetting(isFlag, type, /* allowMultiple= */ false, false);
}

public Type<?> getType() {
Expand All @@ -58,6 +61,10 @@ public boolean allowsMultiple() {
return allowMultiple;
}

public boolean isRepeatableFlag() {
return repeatable;
}

@Override
public void repr(Printer printer) {
printer.append("<build_setting." + type + ">");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept
String.format("Unrecognized option: %s=%s", loadedFlag, unparsedValue));
}
Type<?> type = buildSetting.getType();
if (buildSetting.isRepeatableFlag()) {
type = Preconditions.checkNotNull(type.getListElementType());
}
Converter<?> converter = BUILD_SETTING_CONVERTERS.get(type);
Object value;
try {
Expand All @@ -179,7 +182,7 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept
loadedFlag, unparsedValue, unparsedValue, type),
e);
}
if (buildSetting.allowsMultiple()) {
if (buildSetting.allowsMultiple() || buildSetting.isRepeatableFlag()) {
List<Object> newValue;
if (buildSettingWithTargetAndValue.containsKey(loadedFlag)) {
newValue =
Expand Down Expand Up @@ -371,7 +374,8 @@ public OptionsParser getNativeOptionsParserFortesting() {
}

public boolean checkIfParsedOptionAllowsMultiple(String option) {
return parsedBuildSettings.get(option).allowsMultiple();
BuildSetting setting = parsedBuildSettings.get(option);
return setting.allowsMultiple() || setting.isRepeatableFlag();
}

public Type<?> getParsedOptionType(String option) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import net.starlark.java.annot.ParamType;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.NoneType;
import net.starlark.java.eval.StarlarkValue;

Expand Down Expand Up @@ -90,11 +91,13 @@ public interface StarlarkConfigApi extends StarlarkValue {
name = "allow_multiple",
defaultValue = "False",
doc =
"If set, this flag is allowed to be set multiple times on the command line. The"
+ " Value of the flag as accessed in transitions and build setting"
+ " implementation function will be a list of strings. Insertion order and"
+ " repeated values are both maintained. This list can be post-processed in the"
+ " build setting implementation function if different behavior is desired.",
"Deprecated, use a <code>string_list</code> setting with"
+ " <code>repeatable = True</code> instead. If set, this flag is allowed to be"
+ " set multiple times on the command line. The Value of the flag as accessed"
+ " in transitions and build setting implementation function will be a list of"
+ " strings. Insertion order and repeated values are both maintained. This list"
+ " can be post-processed in the build setting implementation function if"
+ " different behavior is desired.",
named = true,
positional = false)
})
Expand All @@ -111,9 +114,20 @@ public interface StarlarkConfigApi extends StarlarkValue {
defaultValue = "False",
doc = FLAG_ARG_DOC,
named = true,
positional = false),
@Param(
name = "repeatable",
defaultValue = "False",
doc =
"If set, instead of expecting a comma-separated value, this flag is allowed to be"
+ " set multiple times on the command line with each individual value treated"
+ " as a single string to add to the list value. Insertion order and repeated"
+ " values are both maintained. This list can be post-processed in the build"
+ " setting implementation function if different behavior is desired.",
named = true,
positional = false)
})
BuildSettingApi stringListSetting(Boolean flag);
BuildSettingApi stringListSetting(Boolean flag, Boolean repeatable) throws EvalException;

/** The API for build setting descriptors. */
@StarlarkBuiltin(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public BuildSettingApi stringSetting(Boolean flag, Boolean allowMultiple) {
}

@Override
public BuildSettingApi stringListSetting(Boolean flag) {
public BuildSettingApi stringListSetting(Boolean flag, Boolean repeated) {
return new FakeBuildSettingDescriptor();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1534,6 +1534,51 @@ public void buildsettings_allowMultipleWorks() throws Exception {
assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
}

@Test
public void buildsettings_repeatableWorks() throws Exception {
scratch.file(
"test/build_settings.bzl",
"def _impl(ctx):",
" return []",
"string_list_flag = rule(",
" implementation = _impl,",
" build_setting = config.string_list(flag = True, repeatable = True),",
")");
scratch.file(
"test/BUILD",
"load('//test:build_settings.bzl', 'string_list_flag')",
"config_setting(",
" name = 'match',",
" flag_values = {",
" ':cheese': 'pepperjack',",
" },",
")",
"string_list_flag(name = 'cheese', build_setting_default = ['gouda'])");

useConfiguration(ImmutableMap.of("//test:cheese", ImmutableList.of("pepperjack", "brie")));
assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
}

@Test
public void buildsettings_repeatableWithoutFlagErrors() throws Exception {
scratch.file(
"test/build_settings.bzl",
"def _impl(ctx):",
" return []",
"string_list_setting = rule(",
" implementation = _impl,",
" build_setting = config.string_list(repeatable = True),",
")");
scratch.file(
"test/BUILD",
"load('//test:build_settings.bzl', 'string_list_setting')",
"string_list_setting(name = 'cheese', build_setting_default = ['gouda'])");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:cheese");
assertContainsEvent("'repeatable' can only be set for a setting with 'flag = True'");
}

@Test
public void notBuildSettingOrFeatureFlag() throws Exception {
scratch.file(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,4 +478,27 @@ public void testAllowMultipleStringFlag() throws Exception {
assertThat((List<String>) result.getStarlarkOptions().get("//test:cats"))
.containsExactly("calico", "bengal");
}

@Test
@SuppressWarnings("unchecked")
public void testRepeatedStringListFlag() throws Exception {
scratch.file(
"test/build_setting.bzl",
"def _build_setting_impl(ctx):",
" return []",
"repeated_flag = rule(",
" implementation = _build_setting_impl,",
" build_setting = config.string_list(flag=True, repeatable=True)",
")");
scratch.file(
"test/BUILD",
"load('//test:build_setting.bzl', 'repeated_flag')",
"repeated_flag(name = 'cats', build_setting_default = ['tabby'])");

OptionsParsingResult result = parseStarlarkOptions("--//test:cats=calico --//test:cats=bengal");

assertThat(result.getStarlarkOptions().keySet()).containsExactly("//test:cats");
assertThat((List<String>) result.getStarlarkOptions().get("//test:cats"))
.containsExactly("calico", "bengal");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2960,6 +2960,66 @@ public void testBuildSettingValue_allowMultipleSetting() throws Exception {
.containsExactly("some-other-value", "some-other-other-value");
}

@SuppressWarnings("unchecked")
@Test
public void testBuildSettingValue_isRepeatedSetting() throws Exception {
scratch.file(
"test/build_setting.bzl",
"BuildSettingInfo = provider(fields = ['name', 'value'])",
"def _impl(ctx):",
" return [BuildSettingInfo(name = ctx.attr.name, value = ctx.build_setting_value)]",
"",
"string_list_flag = rule(",
" implementation = _impl,",
" build_setting = config.string_list(flag = True, repeatable = True),",
")");
scratch.file(
"test/BUILD",
"load('//test:build_setting.bzl', 'string_list_flag')",
"string_list_flag(name = 'string_list_flag', build_setting_default = ['some-value'])");

// from default
ConfiguredTarget buildSetting = getConfiguredTarget("//test:string_list_flag");
Provider.Key key =
new StarlarkProvider.Key(
Label.create(buildSetting.getLabel().getPackageIdentifier(), "build_setting.bzl"),
"BuildSettingInfo");
StructImpl buildSettingInfo = (StructImpl) buildSetting.get(key);

assertThat(buildSettingInfo.getValue("value")).isInstanceOf(List.class);
assertThat((List<String>) buildSettingInfo.getValue("value")).containsExactly("some-value");

// Set multiple times
useConfiguration(
ImmutableMap.of(
"//test:string_list_flag",
ImmutableList.of("some-other-value", "some-other-other-value")));
buildSetting = getConfiguredTarget("//test:string_list_flag");
key =
new StarlarkProvider.Key(
Label.create(buildSetting.getLabel().getPackageIdentifier(), "build_setting.bzl"),
"BuildSettingInfo");
buildSettingInfo = (StructImpl) buildSetting.get(key);

assertThat(buildSettingInfo.getValue("value")).isInstanceOf(List.class);
assertThat((List<String>) buildSettingInfo.getValue("value"))
.containsExactly("some-other-value", "some-other-other-value");

// No splitting on comma.
useConfiguration(
ImmutableMap.of("//test:string_list_flag", ImmutableList.of("a,b,c", "a", "b,c")));
buildSetting = getConfiguredTarget("//test:string_list_flag");
key =
new StarlarkProvider.Key(
Label.create(buildSetting.getLabel().getPackageIdentifier(), "build_setting.bzl"),
"BuildSettingInfo");
buildSettingInfo = (StructImpl) buildSetting.get(key);

assertThat(buildSettingInfo.getValue("value")).isInstanceOf(List.class);
assertThat((List<String>) buildSettingInfo.getValue("value"))
.containsExactly("a,b,c", "a", "b,c");
}

@Test
public void testBuildSettingValue_nonBuildSettingRule() throws Exception {
scratch.file(
Expand Down

0 comments on commit 9c2c3de

Please sign in to comment.