From cdf01a39ab9def4d46f41595ac1ac9206a96d6f8 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 20 Jul 2022 08:22:48 -0700 Subject: [PATCH] Allow string_list flags to be set via repeated flag uses 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 --- .../lib/analysis/starlark/StarlarkConfig.java | 10 +++- .../build/lib/packages/BuildSetting.java | 15 +++-- .../lib/runtime/StarlarkOptionsParser.java | 8 ++- .../starlarkbuildapi/StarlarkConfigApi.java | 26 ++++++-- .../skydoc/fakebuildapi/FakeConfigApi.java | 2 +- .../lib/rules/config/ConfigSettingTest.java | 45 ++++++++++++++ .../starlark/StarlarkOptionsParsingTest.java | 23 +++++++ .../lib/starlark/StarlarkRuleContextTest.java | 60 +++++++++++++++++++ 8 files changed, 173 insertions(+), 16 deletions(-) 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 89a35f50397207..edde8cfe3344da 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 359358b054eb7b..76287d2f7ee348 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 31d7d4e6c19987..aa425fb7200c54 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 @@ -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( 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 943d435a7f31cc..6a6b5548c660fb 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 @@ -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) 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(