Properly parse build setting strings in flag_values attr for config_settings
PiperOrigin-RevId: 249047560
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
index d0e8ff5..31341ef 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
@@ -14,12 +14,24 @@
package com.google.devtools.build.lib.analysis.config;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
+import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
+import static com.google.devtools.build.lib.syntax.Type.INTEGER;
+import static com.google.devtools.build.lib.syntax.Type.STRING;
+import static com.google.devtools.build.lib.syntax.Type.STRING_LIST;
+
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.common.options.Converter;
+import com.google.devtools.common.options.Converters.BooleanConverter;
+import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
+import com.google.devtools.common.options.Converters.IntegerConverter;
+import com.google.devtools.common.options.Converters.StringConverter;
import com.google.devtools.common.options.EnumConverter;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.Collections;
@@ -32,6 +44,21 @@
* domain-specific (i.e. aren't consumed within a single {@link FragmentOptions}).
*/
public class CoreOptionConverters {
+
+ /**
+ * The set of converters used for {@link com.google.devtools.build.lib.packages.BuildSetting}
+ * value parsing.
+ */
+ public static final ImmutableMap<Type<?>, Converter<?>> BUILD_SETTING_CONVERTERS =
+ new ImmutableMap.Builder<Type<?>, Converter<?>>()
+ .put(INTEGER, new IntegerConverter())
+ .put(BOOLEAN, new BooleanConverter())
+ .put(STRING, new StringConverter())
+ .put(STRING_LIST, new CommaSeparatedOptionListConverter())
+ .put(LABEL, new LabelConverter())
+ .put(LABEL_LIST, new LabelListConverter())
+ .build();
+
/** A converter from strings to Labels. */
public static class LabelConverter implements Converter<Label> {
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
index 828f0a7..7522324 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
@@ -234,6 +234,13 @@
.add(
attr(DEFINE_SETTINGS_ATTRIBUTE, STRING_DICT)
.nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
+ // Originally this attribute was a map of feature flags targets -> feature flag values,
+ // the latter of which are always strings. Now it also includes starlark build setting
+ // targets -> starlark build setting values. In other places in the starlark configuration
+ // API, starlark setting values are passed as their actual object instead of a string
+ // representation. It would be more consistent to be able to pass starlark setting values
+ // as objects to this attribute as well. But attributes are strongly-typed so
+ // label->object dict is not an option for attribute types right now.
.add(
attr(FLAG_SETTINGS_ATTRIBUTE, LABEL_KEYED_STRING_DICT)
.undocumented("the feature flag feature has not yet been launched")
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
index 5d16409..67bdfc5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.rules.config;
+import static com.google.devtools.build.lib.analysis.config.CoreOptionConverters.BUILD_SETTING_CONVERTERS;
+
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
@@ -58,7 +60,6 @@
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import com.google.devtools.build.lib.rules.config.ConfigRuleClasses.ConfigSettingRule;
import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.syntax.Type.ConversionException;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
@@ -403,6 +404,16 @@
return targetsToAliases.build();
}
+ /**
+ * The 'flag_values' attribute takes a label->string dictionary of feature flags and
+ * starlark-defined settings to their values in string form.
+ *
+ * @param attributeValue map of user-defined flag labels to their values as set in the
+ * 'flag_values' attribute
+ * @param prerequisites targets that match the keyset of the attributeValue map
+ * @param optionDetails information about the configuration to match against
+ * @param errors error consumer
+ */
static UserDefinedFlagMatch fromAttributeValueAndPrerequisites(
Map<Label, String> attributeValue,
Iterable<? extends TransitiveInfoCollection> prerequisites,
@@ -446,8 +457,9 @@
: provider.getDefaultValue();
Object convertedSpecifiedValue;
try {
- convertedSpecifiedValue = provider.getType().convert(specifiedValue, specifiedLabel);
- } catch (ConversionException e) {
+ convertedSpecifiedValue =
+ BUILD_SETTING_CONVERTERS.get(provider.getType()).convert(specifiedValue);
+ } catch (OptionsParsingException e) {
errors.attributeError(
ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
String.format(
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 2fa39b5..aad40d2 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
@@ -14,20 +14,14 @@
package com.google.devtools.build.lib.runtime;
-import static com.google.devtools.build.lib.packages.BuildType.LABEL;
-import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
+import static com.google.devtools.build.lib.analysis.config.CoreOptionConverters.BUILD_SETTING_CONVERTERS;
import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
-import static com.google.devtools.build.lib.syntax.Type.INTEGER;
-import static com.google.devtools.build.lib.syntax.Type.STRING;
-import static com.google.devtools.build.lib.syntax.Type.STRING_LIST;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
-import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelConverter;
-import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.LabelListConverter;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.events.Reporter;
@@ -43,10 +37,6 @@
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.common.options.Converter;
-import com.google.devtools.common.options.Converters.BooleanConverter;
-import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
-import com.google.devtools.common.options.Converters.IntegerConverter;
-import com.google.devtools.common.options.Converters.StringConverter;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.Collections;
@@ -66,16 +56,6 @@
private final Reporter reporter;
private final OptionsParser nativeOptionsParser;
- private final ImmutableMap<Type<?>, Converter<?>> converters =
- new ImmutableMap.Builder<Type<?>, Converter<?>>()
- .put(INTEGER, new IntegerConverter())
- .put(BOOLEAN, new BooleanConverter())
- .put(STRING, new StringConverter())
- .put(STRING_LIST, new CommaSeparatedOptionListConverter())
- .put(LABEL, new LabelConverter())
- .put(LABEL_LIST, new LabelListConverter())
- .build();
-
private StarlarkOptionsParser(
SkyframeExecutor skyframeExecutor,
PathFragment relativeWorkingDirectory,
@@ -146,7 +126,7 @@
String.format("Unrecognized option: %s=%s", loadedFlag, unparsedValue));
}
Type<?> type = buildSetting.getType();
- Converter<?> converter = converters.get(type);
+ Converter<?> converter = BUILD_SETTING_CONVERTERS.get(type);
Object value;
try {
value = converter.convert(unparsedValue);
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 b40c670..c250a6a 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
@@ -1320,6 +1320,32 @@
assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
}
+ /**
+ * Regression test to ensure that non-String typed build setting values are being properly
+ * converted from Strings to their real type.
+ */
+ @Test
+ public void buildsettings_convertedType() throws Exception {
+ setSkylarkSemanticsOptions("--experimental_build_setting_api=true");
+
+ scratch.file(
+ "test/build_settings.bzl",
+ "def _impl(ctx):",
+ " return []",
+ "bool_flag = rule(implementation = _impl, build_setting = config.bool(flag = True))");
+ scratch.file(
+ "test/BUILD",
+ "load('//test:build_settings.bzl', 'bool_flag')",
+ "config_setting(",
+ " name = 'match',",
+ " flag_values = {",
+ " ':cheese': 'True',",
+ " },",
+ ")",
+ "bool_flag(name = 'cheese', build_setting_default = True)");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
+ }
+
@Test
public void buildsettings_doesntMatch() throws Exception {
setSkylarkSemanticsOptions("--experimental_build_setting_api=true");
