Skip to content

Commit

Permalink
Allow string_list flags to be set via repeated flag uses
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 bazelbuild#13817

Closes bazelbuild#14911.

PiperOrigin-RevId: 462146752
Change-Id: I91ddc4328a1b2244f4303fcda7b4228b182a5d56
  • Loading branch information
fmeum authored and copybara-github committed Jul 20, 2022
1 parent dcd8585 commit cdf01a3
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 @@ -1774,6 +1774,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 @@ -3024,6 +3024,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 cdf01a3

Please sign in to comment.