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 f44925cd9259ae..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; @@ -44,8 +45,11 @@ public BuildSetting stringSetting(Boolean flag, Boolean allowMultiple) { } @Override - public BuildSetting stringListSetting(Boolean flag, Boolean repeated) { - return BuildSetting.create(flag, STRING_LIST, false, repeated); + 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 d09c0aecd13a54..5f82849363eb4e 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,17 +27,18 @@ public class BuildSetting implements BuildSettingApi { private final boolean isFlag; private final Type type; private final boolean allowMultiple; - private final boolean repeated; + private final boolean repeatable; - private BuildSetting(boolean isFlag, Type type, boolean allowMultiple, boolean repeated) { + private BuildSetting(boolean isFlag, Type type, boolean allowMultiple, boolean repeatable) { this.isFlag = isFlag; this.type = type; this.allowMultiple = allowMultiple; - this.repeated = repeated; + this.repeatable = repeatable; } - 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, boolean allowMultiple, + boolean repeatable) { + return new BuildSetting(isFlag, type, allowMultiple, repeatable); } public static BuildSetting create(boolean isFlag, Type type) { @@ -60,7 +61,7 @@ public boolean allowsMultiple() { return allowMultiple; } - public boolean usesRepeatedStyle() { return repeated; } + public boolean isRepeatableFlag() { return repeatable; } @Override public void repr(Printer printer) { 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 449790d3160ad6..9a0685c6ac30ae 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,7 +168,7 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept String.format("Unrecognized option: %s=%s", loadedFlag, unparsedValue)); } Type type = buildSetting.getType(); - if (buildSetting.usesRepeatedStyle()) { + if (buildSetting.isRepeatableFlag()) { type = Preconditions.checkNotNull(type.getListElementType()); } Converter converter = BUILD_SETTING_CONVERTERS.get(type); @@ -182,7 +182,7 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept loadedFlag, unparsedValue, unparsedValue, type), e); } - if (buildSetting.allowsMultiple() || buildSetting.usesRepeatedStyle()) { + if (buildSetting.allowsMultiple() || buildSetting.isRepeatableFlag()) { List newValue; if (buildSettingWithTargetAndValue.containsKey(loadedFlag)) { newValue = @@ -375,7 +375,7 @@ public OptionsParser getNativeOptionsParserFortesting() { public boolean checkIfParsedOptionAllowsMultiple(String option) { BuildSetting setting = parsedBuildSettings.get(option); - return setting.allowsMultiple() || setting.usesRepeatedStyle(); + 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 54d39a438fea86..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; @@ -91,7 +92,7 @@ public interface StarlarkConfigApi extends StarlarkValue { defaultValue = "False", doc = "Deprecated, use a string_list setting with" - + " repeated = True instead. If set, this flag is allowed to be" + + " 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" @@ -115,7 +116,7 @@ public interface StarlarkConfigApi extends StarlarkValue { named = true, positional = false), @Param( - name = "repeated", + name = "repeatable", defaultValue = "False", doc = "If set, instead of expecting a comma-separated value, this flag is allowed to be" @@ -126,7 +127,7 @@ public interface StarlarkConfigApi extends StarlarkValue { named = true, positional = false) }) - BuildSettingApi stringListSetting(Boolean flag, Boolean repeated); + BuildSettingApi stringListSetting(Boolean flag, Boolean repeatable) throws EvalException; /** The API for build setting descriptors. */ @StarlarkBuiltin( 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 17151a7e81cebd..c53d993954e7db 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 @@ -1775,14 +1775,14 @@ public void buildsettings_allowMultipleWorks() throws Exception { } @Test - public void buildsettings_repeatedWorks() throws Exception { + 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, repeated = True),", + " build_setting = config.string_list(flag = True, repeatable = True),", ")"); scratch.file( "test/BUILD", @@ -1794,10 +1794,31 @@ public void buildsettings_repeatedWorks() throws Exception { " },", ")", "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 865b4b234e3b39..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 @@ -488,7 +488,7 @@ public void testRepeatedStringListFlag() throws Exception { " return []", "repeated_flag = rule(", " implementation = _build_setting_impl,", - " build_setting = config.string_list(flag=True, repeated=True)", + " build_setting = config.string_list(flag=True, repeatable=True)", ")"); scratch.file( "test/BUILD", 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 69f545451a6812..0aa9e5408efb9d 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 @@ -3036,7 +3036,7 @@ public void testBuildSettingValue_isRepeatedSetting() throws Exception { "", "string_list_flag = rule(", " implementation = _impl,", - " build_setting = config.string_list(flag = True, repeated = True),", + " build_setting = config.string_list(flag = True, repeatable = True),", ")"); scratch.file( "test/BUILD",