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..f44925cd9259ae 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 @@ -40,12 +40,12 @@ 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 repeated) { + return BuildSetting.create(flag, STRING_LIST, false, repeated); } @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..d09c0aecd13a54 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,24 @@ public class BuildSetting implements BuildSettingApi { private final boolean isFlag; private final Type type; private final boolean allowMultiple; + private final boolean repeated; - private BuildSetting(boolean isFlag, Type type, boolean allowMultiple) { + private BuildSetting(boolean isFlag, Type type, boolean allowMultiple, boolean repeated) { this.isFlag = isFlag; this.type = type; this.allowMultiple = allowMultiple; + this.repeated = repeated; } - 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 repeated) { + return new BuildSetting(isFlag, type, allowMultiple, repeated); } 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 +60,8 @@ public boolean allowsMultiple() { return allowMultiple; } + public boolean usesRepeatedStyle() { return repeated; } + @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 ca412a1864cd63..449790d3160ad6 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.usesRepeatedStyle()) { + 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.usesRepeatedStyle()) { 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.usesRepeatedStyle(); } 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 359358b054eb7b..54d39a438fea86 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 @@ -90,11 +90,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" + + " repeated = 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 +113,20 @@ public interface StarlarkConfigApi extends StarlarkValue { defaultValue = "False", doc = FLAG_ARG_DOC, named = true, + positional = false), + @Param( + name = "repeated", + 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 repeated); /** 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 19315ffd9dae72..0529ceb373b4fd 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 @@ -1532,6 +1532,30 @@ public void buildsettings_allowMultipleWorks() throws Exception { assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); } + @Test + public void buildsettings_repeatedWorks() 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, repeated = 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 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..865b4b234e3b39 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, repeated=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 2d42c602cf0a23..88bb62b8416da6 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 @@ -3032,6 +3032,51 @@ 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, repeated = 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"); + } + @Test public void testBuildSettingValue_nonBuildSettingRule() throws Exception { scratch.file(