diff options
4 files changed, 162 insertions, 38 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java index 586c46257a..c1a28bca67 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.analysis.config; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Multimap; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; @@ -33,7 +35,7 @@ public final class ConfigMatchingProvider implements TransitiveInfoProvider { private final Label label; private final boolean matches; - private final Map<String, String> settingsMap; + private final Multimap<String, String> settingsMap; private final Map<Label, String> flagSettingsMap; /** @@ -45,7 +47,7 @@ public final class ConfigMatchingProvider implements TransitiveInfoProvider { */ public ConfigMatchingProvider( Label label, - Map<String, String> settingsMap, + Multimap<String, String> settingsMap, Map<Label, String> flagSettingsMap, boolean matches) { this.label = label; @@ -74,8 +76,8 @@ public final class ConfigMatchingProvider implements TransitiveInfoProvider { * conditions, i.e. if this matcher is a specialization of the other one. */ public boolean refines(ConfigMatchingProvider other) { - Set<Map.Entry<String, String>> settings = settingsMap.entrySet(); - Set<Map.Entry<String, String>> otherSettings = other.settingsMap.entrySet(); + Set<Map.Entry<String, String>> settings = ImmutableSet.copyOf(settingsMap.entries()); + Set<Map.Entry<String, String>> otherSettings = ImmutableSet.copyOf(other.settingsMap.entries()); Set<Map.Entry<Label, String>> flagSettings = flagSettingsMap.entrySet(); Set<Map.Entry<Label, String>> otherFlagSettings = other.flagSettingsMap.entrySet(); 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 29ecacea1a..a6b30a4baa 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 @@ -102,10 +102,10 @@ public class ConfigRuleClasses { */ public static final String RULE_NAME = "config_setting"; - /** - * The name of the attribute that declares flag bindings. - */ + /** The name of the attribute that declares flag bindings. */ public static final String SETTINGS_ATTRIBUTE = "values"; + /** The name of the attribute that declares "--define foo=bar" flag bindings.*/ + public static final String DEFINE_SETTINGS_ATTRIBUTE = "define_values"; /** The name of the attribute that declares user-defined flag bindings. */ public static final String FLAG_SETTINGS_ATTRIBUTE = "flag_values"; @@ -143,12 +143,58 @@ public class ConfigRuleClasses { <i>any</i> of those settings match. <p> - <p>This attribute cannot be empty. + <p>This and <a href="${link config_setting.define_values}"><code>values</code></a> cannot + both be empty. </p> <!-- #END_BLAZE_RULE.ATTRIBUTE --> */ .add( attr(SETTINGS_ATTRIBUTE, STRING_DICT) .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) + /* <!-- #BLAZE_RULE(config_setting).ATTRIBUTE(define_values) --> + The same as <a href="${link config_setting.values}"><code>values</code></a> but + specifically for the <code>--define</code> flag. + + <p><code>--define</code> is special for two reasons: + + <ol> + <li>It's the primary interface Blaze has today for declaring user-definable settings. + </li> + <li>Its syntax (<code>--define KEY=VAL</code>) means <code>KEY=VAL</code> is + a <i>value</i> from a Blaze flag perspective.</li> + </ol> + + <p>That means: + + <pre class="code"> + config_setting( + name = "a_and_b", + values = { + "define": "a=1", + "define": "b=2", + }) + </pre> + + <p>doesn't work because the same key (<code>define</code>) appears twice in the + dictionary. This attribute solves that problem: + + <pre class="code"> + config_setting( + name = "a_and_b", + define_values = { + "a": "1", + "b": "2", + }) + </pre> + + <p>corrrectly matches <code>blaze build //foo --define a=1 --define b=2</code>. + + <p><code>--define</code> can still appear in + <a href="${link config_setting.values}"><code>values</code></a> with normal flag syntax, + and can be mixed freely with this attribute as long as dictionary keys remain distinct. + <!-- #END_BLAZE_RULE.ATTRIBUTE --> */ + .add( + attr(DEFINE_SETTINGS_ATTRIBUTE, STRING_DICT) + .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) .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 8215f4ad78..4b66c1f2a4 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 @@ -15,11 +15,16 @@ package com.google.devtools.build.lib.rules.config; import com.google.common.base.Joiner; +import com.google.common.collect.HashMultiset; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import com.google.common.collect.Multiset; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; @@ -34,17 +39,20 @@ 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.featurecontrol.FeaturePolicyConfiguration; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; -import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.rules.AliasProvider; import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory; +import com.google.devtools.build.lib.rules.config.ConfigRuleClasses.ConfigSettingRule; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.Preconditions; import com.google.devtools.common.options.OptionsBase; 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; @@ -62,41 +70,53 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { @Override public ConfiguredTarget create(RuleContext ruleContext) throws InterruptedException, RuleErrorException { - // Get the required flag=value settings for this rule. - Map<String, String> settings = NonconfigurableAttributeMapper.of(ruleContext.getRule()) - .get(ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE, Type.STRING_DICT); - Map<Label, String> flagSettings = + AttributeMap attributes = NonconfigurableAttributeMapper.of(ruleContext.getRule()); + // Get the built-in Blaze flag settings that match this rule. + ImmutableMultimap<String, String> nativeFlagSettings = + ImmutableMultimap.<String, String>builder() + .putAll(attributes.get(ConfigSettingRule.SETTINGS_ATTRIBUTE, Type.STRING_DICT) + .entrySet()) + .putAll(attributes.get(ConfigSettingRule.DEFINE_SETTINGS_ATTRIBUTE, Type.STRING_DICT) + .entrySet() + .stream() + .map(in -> Maps.immutableEntry("define", in.getKey() + "=" + in.getValue())) + .collect(ImmutableList.toImmutableList())) + .build(); + + // Get the user-defined flag settings that match this rule. + Map<Label, String> userDefinedFlagSettings = NonconfigurableAttributeMapper.of(ruleContext.getRule()) .get( - ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, BuildType.LABEL_KEYED_STRING_DICT); - if (!flagSettings.isEmpty()) { + + if (!userDefinedFlagSettings.isEmpty()) { FeaturePolicyConfiguration.checkAvailable( ruleContext, ConfigFeatureFlag.POLICY_NAME, - "the " + ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE + " attribute"); + "the " + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE + " attribute"); } List<? extends TransitiveInfoCollection> flagValues = ruleContext.getPrerequisites( - ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET); + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET); - if (settings.isEmpty() && flagSettings.isEmpty()) { + if (nativeFlagSettings.size() == 0 && userDefinedFlagSettings.isEmpty()) { ruleContext.ruleError( String.format( "Either %s or %s must be specified and non-empty", - ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE, - ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE)); + ConfigSettingRule.SETTINGS_ATTRIBUTE, + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE)); return null; } ConfigFeatureFlagMatch featureFlags = ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites( - flagSettings, flagValues, (RuleErrorConsumer) ruleContext); + userDefinedFlagSettings, flagValues, ruleContext); - boolean settingsMatch = + boolean nativeFlagsMatch = matchesConfig( - settings, + nativeFlagSettings.entries(), BuildConfigurationOptionDetails.get(ruleContext.getConfiguration()), ruleContext); @@ -107,9 +127,9 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { ConfigMatchingProvider configMatcher = new ConfigMatchingProvider( ruleContext.getLabel(), - settings, + nativeFlagSettings, featureFlags.getSpecifiedFlagValues(), - settingsMatch && featureFlags.matches()); + nativeFlagsMatch && featureFlags.matches()); return new RuleConfiguredTargetBuilder(ruleContext) .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY) @@ -121,30 +141,40 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { } /** - * Given a list of [flagName, flagValue] pairs, returns true if flagName == flagValue for every - * item in the list under this configuration, false otherwise. + * User error when value settings can't be properly parsed. + */ + private static final String PARSE_ERROR_MESSAGE = "error while parsing configuration settings: "; + + /** + * 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. */ private boolean matchesConfig( - Map<String, String> expectedSettings, + Collection<Map.Entry<String, String>> expectedSettings, TransitiveOptionDetails options, RuleErrorConsumer errors) { // Rather than returning fast when we find a mismatch, continue looking at the other flags - // to check that they're indeed valid flag specifications. + // to check they're indeed valid flag specifications. boolean foundMismatch = false; + // Flags that appear multiple times are known as "multi-value options". Each time the options + // parser parses one of their values it adds it to an existing list. In those cases we need to + // make sure to examine only the value we just parsed: not the entire list. + Multiset<String> optionsCount = HashMultiset.create(); + // Since OptionsParser instantiation involves reflection, let's try to minimize that happening. Map<Class<? extends OptionsBase>, OptionsParser> parserCache = new HashMap<>(); - for (Map.Entry<String, String> setting : expectedSettings.entrySet()) { + for (Map.Entry<String, String> setting : expectedSettings) { String optionName = setting.getKey(); String expectedRawValue = setting.getValue(); + int previousOptionCount = optionsCount.add(optionName, 1); Class<? extends OptionsBase> optionClass = options.getOptionClass(optionName); if (optionClass == null) { errors.attributeError( - ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE, - String.format( - "error while parsing configuration settings: unknown option: '%s'", optionName)); + ConfigSettingRule.SETTINGS_ATTRIBUTE, + String.format(PARSE_ERROR_MESSAGE + "unknown option: '%s'", optionName)); foundMismatch = true; continue; } @@ -156,14 +186,18 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { parser.parse("--" + optionName + "=" + expectedRawValue); } catch (OptionsParsingException ex) { errors.attributeError( - ConfigRuleClasses.ConfigSettingRule.SETTINGS_ATTRIBUTE, - "error while parsing configuration settings: " + ex.getMessage()); + ConfigSettingRule.SETTINGS_ATTRIBUTE, + PARSE_ERROR_MESSAGE + ex.getMessage()); foundMismatch = true; continue; } Object expectedParsedValue = parser.getOptions(optionClass).asMap().get(optionName); - + if (previousOptionCount > 0) { + // We've seen this option before, so it's a multi-value option with multiple entries. + int listLength = ((List<?>) expectedParsedValue).size(); + expectedParsedValue = ((List<?>) expectedParsedValue).subList(listLength - 1, listLength); + } if (!optionMatches(options, optionName, expectedParsedValue)) { foundMismatch = true; } @@ -281,7 +315,7 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { if (!provider.isValidValue(specifiedValue)) { errors.attributeError( - ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, String.format( "error while parsing user-defined configuration values: " + "'%s' is not a valid value for '%s'", @@ -304,7 +338,7 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { List<Label> aliasList = aliases.get(actualLabel); if (aliasList.size() > 1) { errors.attributeError( - ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, String.format( "flag '%s' referenced multiple times as ['%s']", actualLabel, QUOTED_COMMA_JOINER.join(aliasList))); 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 b6f869ecd1..741f4dec9d 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 @@ -288,6 +288,48 @@ public class ConfigSettingTest extends BuildViewTestCase { assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); } + @Test + public void multipleDefines() throws Exception { + scratch.file("test/BUILD", + "config_setting(", + " name = 'match',", + " define_values = {", + " 'foo1': 'bar',", + " 'foo2': 'baz',", + " })"); + + useConfiguration(""); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + useConfiguration("--define", "foo1=bar"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + useConfiguration("--define", "foo2=baz"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + useConfiguration("--define", "foo1=bar", "--define", "foo2=baz"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); + } + + @Test + public void definesCrossAttributes() throws Exception { + scratch.file("test/BUILD", + "config_setting(", + " name = 'match',", + " values = {", + " 'define': 'a=c'", + " },", + " define_values = {", + " 'b': 'd',", + " })"); + + useConfiguration(""); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + useConfiguration("--define", "a=c"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + useConfiguration("--define", "b=d"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + useConfiguration("--define", "a=c", "--define", "b=d"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); + } + /** * Tests matching on multi-value attributes with primitive values. */ |