diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java index 96584b184e7bd1..d583cb09514828 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkConfig.java @@ -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; @@ -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 diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java index c64e81f657e1d0..d9b01dcca86316 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java @@ -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() { @@ -58,6 +61,10 @@ public boolean allowsMultiple() { return allowMultiple; } + public boolean isRepeatableFlag() { + return repeatable; + } + @Override public void repr(Printer printer) { printer.append(""); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java index 1d9eb89d490305..0c60ee576e0541 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java @@ -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 { @@ -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 newValue; if (buildSettingWithTargetAndValue.containsKey(loadedFlag)) { newValue = @@ -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) { diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java index 1b5b9e79d2808b..bba02e72ea9b5e 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkConfigApi.java @@ -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; @@ -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 string_list setting with" + + " repeatable = True 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) }) @@ -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( diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java index f40afe104280dd..0582beb4d71b73 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeConfigApi.java @@ -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(); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java index 02813807ab0e05..d6f5782143e727 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java @@ -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( diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java index 0e93717352e940..6579eec0c1b3a5 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java @@ -478,4 +478,27 @@ public void testAllowMultipleStringFlag() throws Exception { assertThat((List) 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) result.getStarlarkOptions().get("//test:cats")) + .containsExactly("calico", "bengal"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java index 94964936fa1ae1..b87da48480fdc2 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java @@ -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) 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) 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) buildSettingInfo.getValue("value")) + .containsExactly("a,b,c", "a", "b,c"); + } + @Test public void testBuildSettingValue_nonBuildSettingRule() throws Exception { scratch.file(