diff options
author | mstaib <mstaib@google.com> | 2017-05-31 18:17:06 +0200 |
---|---|---|
committer | László Csomor <laszlocsomor@google.com> | 2017-06-01 14:07:41 +0200 |
commit | fd2c682a6a6bb0759f92476e533bffd2883b9c27 (patch) | |
tree | 10fe94488353823aea7d0872995e65823d8f632e /src/main/java/com/google/devtools/build/lib | |
parent | 65ceda9c9f686bc27f7319817985a4df07b60f9d (diff) |
Fix aliases for users of label-keyed string dicts.
Aliases mess with the assumption that
attributeValue.containsKey(target.getLabel())
for every target in the prerequisites of a LABEL_KEYED_STRING_DICT
attribute.
The solution is to use AliasProvider.getDependencyLabel(target) instead.
This fixes it for all current users, including SkylarkRuleContext.
This also adjusts config_setting flag_values and Android feature_flags
to do intelligent things with aliases in their respective attributes.
RELNOTES: None.
PiperOrigin-RevId: 157594095
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
3 files changed, 123 insertions, 54 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java index 565da4d942..7c28ed8f47 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java @@ -404,7 +404,7 @@ public final class SkylarkRuleContext implements SkylarkValue { List<? extends TransitiveInfoCollection> allPrereq = ruleContext.getPrerequisites(a.getName(), Mode.DONT_CHECK); for (TransitiveInfoCollection prereq : allPrereq) { - builder.put(prereq, original.get(prereq.getLabel())); + builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq))); } attrBuilder.put(skyname, SkylarkType.convertToSkylark(builder.build(), null)); } else if (type == BuildType.LABEL_DICT_UNARY) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java index 315aea19c6..29375a9ead 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; 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.rules.AliasProvider; import com.google.devtools.build.lib.rules.config.ConfigFeatureFlagProvider; import java.util.Map; @@ -53,7 +54,7 @@ public abstract class AndroidFeatureFlagSetProvider implements TransitiveInfoPro /** * Builds a map which can be used with create, confirming that the desired flag values were * actually received, and producing an error if they were not (because dynamic configurations are - * not enabled). + * not enabled, or because aliases were used). */ public static ImmutableMap<Label, String> getAndValidateFlagMapFromRuleContext( RuleContext ruleContext) throws RuleErrorException { @@ -62,8 +63,18 @@ public abstract class AndroidFeatureFlagSetProvider implements TransitiveInfoPro .get(FEATURE_FLAG_ATTR, BuildType.LABEL_KEYED_STRING_DICT); Iterable<? extends TransitiveInfoCollection> actualTargets = ruleContext.getPrerequisites(FEATURE_FLAG_ATTR, Mode.TARGET); + boolean aliasFound = false; for (TransitiveInfoCollection target : actualTargets) { - Label label = target.getLabel(); + Label label = AliasProvider.getDependencyLabel(target); + if (!label.equals(target.getLabel())) { + ruleContext.attributeError( + FEATURE_FLAG_ATTR, + String.format( + "Feature flags must be named directly, not through aliases; use '%s', not '%s'", + target.getLabel(), label)); + aliasFound = true; + } + String expectedValue = expectedValues.get(label); String actualValue = ConfigFeatureFlagProvider.fromTarget(target).getValue(); @@ -75,6 +86,9 @@ public abstract class AndroidFeatureFlagSetProvider implements TransitiveInfoPro throw new RuleErrorException(); } } + if (aliasFound) { + throw new RuleErrorException(); + } return ImmutableMap.copyOf(expectedValues); } 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 1da66e0cb1..ae98678b71 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,8 +14,11 @@ package com.google.devtools.build.lib.rules.config; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; @@ -34,6 +37,7 @@ 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.syntax.Type; import com.google.devtools.build.lib.util.Preconditions; @@ -41,6 +45,7 @@ import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -69,9 +74,6 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { ruleContext.getPrerequisites( ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET); - ImmutableMap<Label, ConfigFeatureFlagProvider> configProviders = - buildConfigFeatureFlagMap(flagValues); - if (settings.isEmpty() && flagSettings.isEmpty()) { ruleContext.ruleError( String.format( @@ -81,7 +83,9 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { return null; } - boolean flagSettingsMatch = matchesUserConfig(flagSettings, configProviders, ruleContext); + ConfigFeatureFlagMatch featureFlags = + ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites( + flagSettings, flagValues, (RuleErrorConsumer) ruleContext); boolean settingsMatch = matchesConfig( @@ -95,7 +99,10 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { ConfigMatchingProvider configMatcher = new ConfigMatchingProvider( - ruleContext.getLabel(), settings, flagSettings, settingsMatch && flagSettingsMatch); + ruleContext.getLabel(), + settings, + featureFlags.getSpecifiedFlagValues(), + settingsMatch && featureFlags.matches()); return new RuleConfiguredTargetBuilder(ruleContext) .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY) @@ -106,52 +113,6 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { .build(); } - /** Maps the labels of the given prerequisites to their {@link ConfigFeatureFlagProvider}s. */ - private static ImmutableMap<Label, ConfigFeatureFlagProvider> buildConfigFeatureFlagMap( - Iterable<? extends TransitiveInfoCollection> prerequisites) { - ImmutableMap.Builder<Label, ConfigFeatureFlagProvider> output = new ImmutableMap.Builder<>(); - - for (TransitiveInfoCollection target : prerequisites) { - ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.fromTarget(target); - // We know the provider exists because only labels with ConfigFeatureFlagProvider can be added - // to this attribute. - assert provider != null; - output.put(target.getLabel(), provider); - } - - return output.build(); - } - - /** Returns whether the actual user-defined flags are set to the specified values. */ - private static boolean matchesUserConfig( - Map<Label, String> specifiedFlags, - Map<Label, ConfigFeatureFlagProvider> actualFlags, - RuleErrorConsumer errors) { - boolean foundMismatch = false; - for (Map.Entry<Label, String> specifiedFlag : specifiedFlags.entrySet()) { - Label label = specifiedFlag.getKey(); - String specifiedValue = specifiedFlag.getValue(); - ConfigFeatureFlagProvider provider = actualFlags.get(label); - // Both specifiedFlags and actualFlags are built from the same set of keys; therefore, the - // provider will always be present. - assert provider != null; - if (!provider.isValidValue(specifiedValue)) { - errors.attributeError( - ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, - String.format( - "error while parsing user-defined configuration values: " - + "'%s' is not a valid value for '%s'", - specifiedValue, label)); - foundMismatch = true; - continue; - } - if (!provider.getValue().equals(specifiedValue)) { - foundMismatch = true; - } - } - return !foundMismatch; - } - /** * Given a list of [flagName, flagValue] pairs, returns true if flagName == flagValue for every * item in the list under this configuration, false otherwise. @@ -258,4 +219,98 @@ public class ConfigSetting implements RuleConfiguredTargetFactory { // Multi-value list: return actualList.contains(expectedSingleValue); } + + private static final class ConfigFeatureFlagMatch { + private final boolean matches; + private final ImmutableMap<Label, String> specifiedFlagValues; + + private static final Joiner QUOTED_COMMA_JOINER = Joiner.on("', '"); + + private ConfigFeatureFlagMatch( + boolean matches, ImmutableMap<Label, String> specifiedFlagValues) { + this.matches = matches; + this.specifiedFlagValues = specifiedFlagValues; + } + + /** Returns whether the specified flag values matched the actual flag values. */ + public boolean matches() { + return matches; + } + + /** Gets the specified flag values, with aliases converted to their original targets' labels. */ + public ImmutableMap<Label, String> getSpecifiedFlagValues() { + return specifiedFlagValues; + } + + /** Groups aliases in the list of prerequisites by the target they point to. */ + private static ListMultimap<Label, Label> collectAliases( + Iterable<? extends TransitiveInfoCollection> prerequisites) { + ImmutableListMultimap.Builder<Label, Label> targetsToAliases = + new ImmutableListMultimap.Builder<>(); + for (TransitiveInfoCollection target : prerequisites) { + targetsToAliases.put(target.getLabel(), AliasProvider.getDependencyLabel(target)); + } + return targetsToAliases.build(); + } + + public static ConfigFeatureFlagMatch fromAttributeValueAndPrerequisites( + Map<Label, String> attributeValue, + Iterable<? extends TransitiveInfoCollection> prerequisites, + RuleErrorConsumer errors) { + Map<Label, String> specifiedFlagValues = new LinkedHashMap<>(); + boolean matches = true; + boolean foundDuplicate = false; + + for (TransitiveInfoCollection target : prerequisites) { + ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.fromTarget(target); + // We know the provider exists because only labels with ConfigFeatureFlagProvider can be + // added to this attribute. + assert provider != null; + + Label actualLabel = target.getLabel(); + Label specifiedLabel = AliasProvider.getDependencyLabel(target); + String specifiedValue = attributeValue.get(specifiedLabel); + if (specifiedFlagValues.containsKey(actualLabel)) { + foundDuplicate = true; + } + specifiedFlagValues.put(actualLabel, specifiedValue); + + if (!provider.isValidValue(specifiedValue)) { + errors.attributeError( + ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + String.format( + "error while parsing user-defined configuration values: " + + "'%s' is not a valid value for '%s'", + specifiedValue, specifiedLabel)); + matches = false; + continue; + } + if (!provider.getValue().equals(specifiedValue)) { + matches = false; + } + } + + // attributeValue is the source of the prerequisites in prerequisites, so the final map built + // from iterating over prerequisites should always be the same size, barring duplicates. + assert foundDuplicate || attributeValue.size() == specifiedFlagValues.size(); + + if (foundDuplicate) { + ListMultimap<Label, Label> aliases = collectAliases(prerequisites); + for (Label actualLabel : aliases.keySet()) { + List<Label> aliasList = aliases.get(actualLabel); + if (aliasList.size() > 1) { + errors.attributeError( + ConfigRuleClasses.ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + String.format( + "flag '%s' referenced multiple times as ['%s']", + actualLabel, QUOTED_COMMA_JOINER.join(aliasList))); + } + } + + matches = false; + } + + return new ConfigFeatureFlagMatch(matches, ImmutableMap.copyOf(specifiedFlagValues)); + } + } } |