Allow config_settings to match on constraint_values to a target_platform. This is on the way to making select() work with constraint_values re: #305.
PiperOrigin-RevId: 169454982
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/BUILD b/src/main/java/com/google/devtools/build/lib/rules/config/BUILD
index 4145009..4f04785 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/BUILD
@@ -21,6 +21,8 @@
"//src/main/java/com/google/devtools/build/lib:preconditions",
"//src/main/java/com/google/devtools/build/lib:skylarkinterface",
"//src/main/java/com/google/devtools/build/lib/actions",
+ "//src/main/java/com/google/devtools/build/lib/analysis/platform",
+ "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
"//src/main/java/com/google/devtools/build/lib/analysis/whitelisting",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect",
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 88d1283..f4cc35e 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
@@ -39,6 +39,10 @@
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.TransitiveOptionDetails;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
+import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
+import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
+import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.BuildType;
@@ -51,6 +55,7 @@
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.Collection;
+import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
@@ -91,25 +96,37 @@
ruleContext.getPrerequisites(
ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET);
- if (nativeFlagSettings.size() == 0 && userDefinedFlagSettings.isEmpty()) {
- ruleContext.ruleError(
- String.format(
- "Either %s or %s must be specified and non-empty",
- ConfigSettingRule.SETTINGS_ATTRIBUTE,
- ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE));
+ // Get the constraint values that match this rule
+ Iterable<ConstraintValueInfo> constraintValues =
+ PlatformProviderUtils.constraintValues(
+ ruleContext.getPrerequisites(
+ ConfigSettingRule.CONSTRAINT_VALUES_ATTRIBUTE, Mode.DONT_CHECK));
+
+ // Get the target platform
+ Iterable<PlatformInfo> targetPlatforms =
+ PlatformProviderUtils.platforms(
+ ruleContext.getPrerequisites(
+ ConfigSettingRule.TARGET_PLATFORMS_ATTRIBUTE, Mode.DONT_CHECK));
+
+ // Check that this config_setting contains at least one of {values, define_values,
+ // constraint_values}
+ if (!checkValidConditions(
+ nativeFlagSettings, userDefinedFlagSettings, constraintValues, ruleContext)) {
return null;
}
- ConfigFeatureFlagMatch featureFlags =
- ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites(
- userDefinedFlagSettings, flagValues, ruleContext);
-
boolean nativeFlagsMatch =
matchesConfig(
nativeFlagSettings.entries(),
BuildConfigurationOptionDetails.get(ruleContext.getConfiguration()),
ruleContext);
+ ConfigFeatureFlagMatch featureFlags =
+ ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites(
+ userDefinedFlagSettings, flagValues, ruleContext);
+
+ boolean constraintValuesMatch = matchesConstraints(constraintValues, targetPlatforms);
+
if (ruleContext.hasErrors()) {
return null;
}
@@ -119,7 +136,7 @@
ruleContext.getLabel(),
nativeFlagSettings,
featureFlags.getSpecifiedFlagValues(),
- nativeFlagsMatch && featureFlags.matches());
+ nativeFlagsMatch && featureFlags.matches() && constraintValuesMatch);
return new RuleConfiguredTargetBuilder(ruleContext)
.addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY)
@@ -130,11 +147,79 @@
.build();
}
+ private boolean checkValidConditions(
+ ImmutableMultimap<String, String> nativeFlagSettings,
+ Map<Label, String> userDefinedFlagSettings,
+ Iterable<ConstraintValueInfo> constraintValues,
+ RuleErrorConsumer errors) {
+ // Check to make sure this config_setting contains and sets least one of {values, define_values,
+ // flag_value or constraint_values}.
+ if (!valuesAreSet(nativeFlagSettings, userDefinedFlagSettings, constraintValues, errors)) {
+ return false;
+ }
+
+ // The set of constraint_values in a config_setting should never contain multiple
+ // constraint_values that map to the same constraint_setting. This checks if there are
+ // duplicates and records an error if so.
+ if (containsDuplicateSettings(constraintValues, errors)) {
+ return false;
+ }
+
+ return true;
+ }
+
/**
* User error when value settings can't be properly parsed.
*/
private static final String PARSE_ERROR_MESSAGE = "error while parsing configuration settings: ";
+ private boolean valuesAreSet(
+ ImmutableMultimap<String, String> nativeFlagSettings,
+ Map<Label, String> userDefinedFlagSettings,
+ Iterable<ConstraintValueInfo> constraintValues,
+ RuleErrorConsumer errors) {
+ if (nativeFlagSettings.isEmpty()
+ && userDefinedFlagSettings.isEmpty()
+ && Iterables.isEmpty(constraintValues)) {
+ errors.ruleError(
+ String.format(
+ "Either %s, %s or %s must be specified and non-empty",
+ ConfigSettingRule.SETTINGS_ATTRIBUTE,
+ ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE,
+ ConfigSettingRule.CONSTRAINT_VALUES_ATTRIBUTE));
+ return false;
+ }
+ return true;
+ }
+
+
+ /**
+ * The set of constraint_values in a config_setting should never contain multiple
+ * constraint_values that map to the same constraint_setting. This method checks if there are
+ * duplicates and records an error if so.
+ */
+ private boolean containsDuplicateSettings(
+ Iterable<ConstraintValueInfo> constraintValues, RuleErrorConsumer errors) {
+ HashMap<ConstraintSettingInfo, ConstraintValueInfo> constraints = new HashMap<>();
+ for (ConstraintValueInfo constraint : constraintValues) {
+ ConstraintSettingInfo setting = constraint.constraint();
+ if (constraints.containsKey(setting)) {
+ errors.attributeError(
+ ConfigSettingRule.TARGET_PLATFORMS_ATTRIBUTE,
+ String.format(
+ PARSE_ERROR_MESSAGE
+ + "the target platform contains multiple values '%s' "
+ + "and '%s' that map to the same setting '%s'",
+ constraint.label(),
+ constraints.get(setting).label(),
+ setting.label()));
+ return true;
+ }
+ constraints.put(setting, constraint);
+ }
+ return false;
+ }
+
/**
* Given a list of [flagName, flagValue] pairs for native Blaze flags, returns true if
* flagName == flagValue for every item in the list under this configuration, false otherwise.
@@ -244,6 +329,33 @@
return actualList.contains(expectedSingleValue);
}
+ private boolean matchesConstraints(
+ Iterable<ConstraintValueInfo> expected, Iterable<PlatformInfo> targetPlatforms) {
+ // config_setting didn't specify any constraint values
+ if (Iterables.isEmpty(expected)) {
+ return true;
+ }
+
+ // TODO(jcater): re-evaluate this for multiple target platforms.
+ PlatformInfo targetPlatform = Iterables.getOnlyElement(targetPlatforms);
+ // config_setting DID specify constraint_value(s) but no target platforms are set
+ // in the configuration.
+ if (Iterables.isEmpty(targetPlatform.constraints())) {
+ return false;
+ }
+
+ // For every constraint in the attr check if it is (1)set aka non-null and
+ // (2)set correctly in the platform.
+ for (ConstraintValueInfo constraint : expected) {
+ ConstraintSettingInfo setting = constraint.constraint();
+ ConstraintValueInfo targetValue = targetPlatform.getConstraint(setting);
+ if (targetValue == null || !constraint.equals(targetValue)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
private static final class ConfigFeatureFlagMatch {
private final boolean matches;
private final ImmutableMap<Label, String> specifiedFlagValues;
@@ -338,3 +450,4 @@
}
}
}
+
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 9def906..f45a85e 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
@@ -239,9 +239,11 @@
*/
@Test
public void emptySettings() throws Exception {
- checkError("foo", "empty",
+ checkError(
+ "foo",
+ "empty",
"in config_setting rule //foo:empty: "
- + "Either values or flag_values must be specified and non-empty",
+ + "Either values, flag_values or constraint_values must be specified and non-empty",
"config_setting(",
" name = 'empty',",
" values = {})");
@@ -1122,4 +1124,130 @@
" default_value = 'valid',",
")");
}
+
+ @Test
+ public void constraintValue() throws Exception {
+ scratch.file(
+ "test/BUILD",
+ "constraint_setting(name = 'notable_building')",
+ "constraint_value(name = 'empire_state', constraint_setting = 'notable_building')",
+ "constraint_value(name = 'space_needle', constraint_setting = 'notable_building')",
+ "platform(",
+ " name = 'new_york_platform',",
+ " constraint_values = [':empire_state'],",
+ ")",
+ "platform(",
+ " name = 'seattle_platform',",
+ " constraint_values = [':space_needle'],",
+ ")",
+ "config_setting(",
+ " name = 'match',",
+ " constraint_values = [':empire_state'],",
+ ");");
+
+ useConfiguration("--experimental_platforms=//test:new_york_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
+ useConfiguration("--experimental_platforms=//test:seattle_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse();
+ useConfiguration("");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse();
+ }
+
+ @Test
+ public void multipleConstraintValues() throws Exception {
+ scratch.file(
+ "test/BUILD",
+ "constraint_setting(name = 'notable_building')",
+ "constraint_value(name = 'empire_state', constraint_setting = 'notable_building')",
+ "constraint_setting(name = 'museum')",
+ "constraint_value(name = 'cloisters', constraint_setting = 'museum')",
+ "constraint_setting(name = 'theme_park')",
+ "constraint_value(name = 'coney_island', constraint_setting = 'theme_park')",
+ "platform(",
+ " name = 'manhattan_platform',",
+ " constraint_values = [",
+ " ':empire_state',",
+ " ':cloisters',",
+ " ],",
+ ")",
+ "platform(",
+ " name = 'museum_platform',",
+ " constraint_values = [':cloisters'],",
+ ")",
+ "platform(",
+ " name = 'new_york_platform',",
+ " constraint_values = [",
+ " ':empire_state',",
+ " ':cloisters',",
+ " ':coney_island',",
+ " ],",
+ ")",
+ "config_setting(",
+ " name = 'match',",
+ " constraint_values = [':empire_state', ':cloisters'],",
+ ");");
+ useConfiguration("--experimental_platforms=//test:manhattan_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
+ useConfiguration("--experimental_platforms=//test:museum_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse();
+ useConfiguration("--experimental_platforms=//test:new_york_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
+ }
+
+ @Test
+ public void definesAndConstraints() throws Exception {
+ scratch.file(
+ "test/BUILD",
+ "constraint_setting(name = 'notable_building')",
+ "constraint_value(name = 'empire_state', constraint_setting = 'notable_building')",
+ "constraint_value(name = 'space_needle', constraint_setting = 'notable_building')",
+ "platform(",
+ " name = 'new_york_platform',",
+ " constraint_values = [':empire_state'],",
+ ")",
+ "platform(",
+ " name = 'seattle_platform',",
+ " constraint_values = [':space_needle'],",
+ ")",
+ "config_setting(",
+ " name = 'match',",
+ " constraint_values = [':empire_state'],",
+ " values = {",
+ " 'define': 'a=c',",
+ " },",
+ " define_values = {",
+ " 'b': 'd',",
+ " },",
+ ");");
+
+ useConfiguration(
+ "--experimental_platforms=//test:new_york_platform", "--define", "a=c", "--define", "b=d");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue();
+ useConfiguration("--experimental_platforms=//test:new_york_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse();
+ useConfiguration("--define", "a=c");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse();
+ useConfiguration("--define", "a=c", "--experimental_platforms=//test:new_york_platform");
+ assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse();
+ }
+
+ /**
+ * Tests that a config_setting doesn't allow a constraint_values list with more than one
+ * constraint value per constraint setting.
+ */
+ @Test
+ public void multipleValuesPerSetting() throws Exception {
+ checkError("foo", "bad",
+ "in :target_platforms attribute of config_setting rule //foo:bad: "
+ + "error while parsing configuration settings: "
+ + "the target platform contains multiple values '//foo:space_needle' "
+ + "and '//foo:empire_state' that map to the same setting '//foo:notable_building'",
+ "constraint_setting(name = 'notable_building')",
+ "constraint_value(name = 'empire_state', constraint_setting = 'notable_building')",
+ "constraint_value(name = 'space_needle', constraint_setting = 'notable_building')",
+ "config_setting(",
+ " name = 'bad',",
+ " constraint_values = [':empire_state', ':space_needle'],",
+ ");");
+ }
}
