diff options
author | 2016-07-27 14:22:41 +0000 | |
---|---|---|
committer | 2016-07-28 18:36:28 -0400 | |
commit | 974bbd92080ec47ebeef8fb99c6178cbf3fe7e1e (patch) | |
tree | b81f4ed060dc932f1a88c7081fc29c3f8af66465 /src/main/java/com/google | |
parent | 040391a600603c4d4430603e89cd0ff8920228af (diff) |
Clean up DependencyResolver's interface for the dynamic config migration and for general readability.
Major changes:
- Remove the intermediate Attribute -> LabelAndConfiguration multimap (computed in resolveAttributes). Instead, feed discovered values directly into the final Attribute -> Dependency map via a new RuleResolver interface.
- Remove all references to LabelAndConfiguration. The configuration is always the owning rule's configuration except for two special cases: late-bound attributes with splits and late-bound attributes with LateBoundDefault.useHostConfiguration. The original interface made this very unclear and required a lot of awkward and sometimes incorrect logic. The new interface only involves configurations for the cases that actually need them.
- Remove an ugly hack caused by BuildConfiguration.evaluateTransition mixing poorly with LateBoundDefault.useHostConfiguration (https://github.com/bazelbuild/bazel/blo[]e172693c27f3efc95ed163e43a9f0a7a6fb4017/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java#L488).
- Remove a hack that applies split transitions twice because of BuildConfiguration.evaluateTransition mixing poorly with late-bound split attributes (https://github.com/bazelbuild/bazel/blo[]e172693c27f3efc95ed163e43a9f0a7a6fb4017/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java#L319). This happens to be innocent now but won't be when nested splits are possible.
- Solidifies the API contract for Attribute.LateBoundDefault.useHostConfiguration.
- Applies clearer naming and more consistent ordering to method parameters.
- Better documentation.
This is all also prep work for dynamic split transitions.
tl;dr: late-bound attributes are legitimately special. Treat them that way to make the rest of DependencyResolver cleaner and hack-free.
--
MOS_MIGRATED_REVID=128582618
Diffstat (limited to 'src/main/java/com/google')
3 files changed, 315 insertions, 201 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index f71f853de8..ec52681412 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -13,20 +13,20 @@ // limitations under the License. package com.google.devtools.build.lib.analysis; +import com.google.common.base.Verify; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Sets; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.AspectClass; -import com.google.devtools.build.lib.packages.AspectDefinition; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault; import com.google.devtools.build.lib.packages.AttributeMap; @@ -80,16 +80,24 @@ public abstract class DependencyResolver { * <p>The long-term goal is that most configuration transitions be applied here. However, in order * to do that, we first have to eliminate transitions that depend on the rule class of the * dependency. + * + * @param node the target/configuration being evaluated + * @param hostConfig the configuration this target would use if it was evaluated as a host + * tool. This is needed to support {@link LateBoundDefault#useHostConfiguration()}. + * @param aspect the aspect applied to this target (if any) + * @param configConditions resolver for config_setting labels + * + * @return a mapping of each attribute in this rule or aspect to its dependent nodes */ public final ListMultimap<Attribute, Dependency> dependentNodeMap( TargetAndConfiguration node, BuildConfiguration hostConfig, - Aspect aspect, + @Nullable Aspect aspect, ImmutableMap<Label, ConfigMatchingProvider> configConditions) throws EvalException, InterruptedException { NestedSetBuilder<Label> rootCauses = NestedSetBuilder.<Label>stableOrder(); - ListMultimap<Attribute, Dependency> outgoingEdges = - dependentNodeMap(node, hostConfig, aspect, configConditions, rootCauses); + ListMultimap<Attribute, Dependency> outgoingEdges = dependentNodeMap( + node, hostConfig, aspect, configConditions, rootCauses); if (!rootCauses.isEmpty()) { throw new IllegalStateException(rootCauses.build().iterator().next().toString()); } @@ -114,11 +122,20 @@ public abstract class DependencyResolver { * <p>The long-term goal is that most configuration transitions be applied here. However, in order * to do that, we first have to eliminate transitions that depend on the rule class of the * dependency. + * + * @param node the target/configuration being evaluated + * @param hostConfig the configuration this target would use if it was evaluated as a host + * tool. This is needed to support {@link LateBoundDefault#useHostConfiguration()}. + * @param aspect the aspect applied to this target (if any) + * @param configConditions resolver for config_setting labels + * @param rootCauses collector for dep labels that can't be (loading phase) loaded + * + * @return a mapping of each attribute in this rule or aspect to its dependent nodes */ public final ListMultimap<Attribute, Dependency> dependentNodeMap( TargetAndConfiguration node, BuildConfiguration hostConfig, - Aspect aspect, + @Nullable Aspect aspect, ImmutableMap<Label, ConfigMatchingProvider> configConditions, NestedSetBuilder<Label> rootCauses) throws EvalException, InterruptedException { @@ -135,17 +152,7 @@ public abstract class DependencyResolver { } else if (target instanceof EnvironmentGroup) { visitTargetVisibility(node, rootCauses, outgoingEdges.get(null)); } else if (target instanceof Rule) { - Preconditions.checkNotNull(config); - visitTargetVisibility(node, rootCauses, outgoingEdges.get(null)); - Rule rule = (Rule) target; - ListMultimap<Attribute, LabelAndConfiguration> labelMap = - resolveAttributes( - rule, - aspect != null ? aspect.getDefinition() : null, - config, - hostConfig, - configConditions); - visitRule(rule, aspect, labelMap, rootCauses, outgoingEdges); + visitRule(node, hostConfig, aspect, configConditions, rootCauses, outgoingEdges); } else if (target instanceof PackageGroup) { visitPackageGroup(node, (PackageGroup) target, rootCauses, outgoingEdges.get(null)); } else { @@ -154,33 +161,41 @@ public abstract class DependencyResolver { return outgoingEdges; } - private ListMultimap<Attribute, LabelAndConfiguration> resolveAttributes( - Rule rule, AspectDefinition aspect, BuildConfiguration configuration, - BuildConfiguration hostConfiguration, - ImmutableMap<Label, ConfigMatchingProvider> configConditions) + private void visitRule( + TargetAndConfiguration node, + BuildConfiguration hostConfig, + @Nullable Aspect aspect, + ImmutableMap<Label, ConfigMatchingProvider> configConditions, + NestedSetBuilder<Label> rootCauses, + ListMultimap<Attribute, Dependency> outgoingEdges) throws EvalException, InterruptedException { + Preconditions.checkArgument(node.getTarget() instanceof Rule); + BuildConfiguration ruleConfig = Preconditions.checkNotNull(node.getConfiguration()); + Rule rule = (Rule) node.getTarget(); + ConfiguredAttributeMapper attributeMap = ConfiguredAttributeMapper.of(rule, configConditions); attributeMap.validateAttributes(); - List<Attribute> attributes; - if (aspect == null) { - attributes = rule.getRuleClassObject().getAttributes(); - } else { - attributes = new ArrayList<>(); - attributes.addAll(rule.getRuleClassObject().getAttributes()); - attributes.addAll(aspect.getAttributes().values()); - } + RuleResolver depResolver = + new RuleResolver(rule, ruleConfig, aspect, attributeMap, rootCauses, outgoingEdges); + + visitTargetVisibility(node, rootCauses, outgoingEdges.get(null)); + resolveEarlyBoundAttributes(depResolver); + resolveLateBoundAttributes(depResolver, ruleConfig, hostConfig); + } - ImmutableSortedKeyListMultimap.Builder<Attribute, LabelAndConfiguration> result = - ImmutableSortedKeyListMultimap.builder(); + /** + * Resolves the dependencies for all attributes in this rule except late-bound attributes + * (which require special processing: see {@link #resolveLateBoundAttributes}). + */ + private void resolveEarlyBoundAttributes(RuleResolver depResolver) + throws EvalException, InterruptedException { + Rule rule = depResolver.rule; - resolveExplicitAttributes(configuration, attributeMap, result); - resolveImplicitAttributes(rule, configuration, attributeMap, attributes, result); - resolveLateBoundAttributes(rule, configuration, hostConfiguration, attributeMap, attributes, - result); + resolveExplicitAttributes(depResolver); + resolveImplicitAttributes(depResolver); // Add the rule's visibility labels (which may come from the rule or from package defaults). - addExplicitDeps(result, rule, "visibility", rule.getVisibility().getDependencyLabels(), - configuration); + addExplicitDeps(depResolver, "visibility", rule.getVisibility().getDependencyLabels()); // Add package default constraints when the rule doesn't explicitly declare them. // @@ -206,46 +221,17 @@ public abstract class DependencyResolver { // above). But within the scope of a single package it seems better to keep the model simple and // make the user responsible for resolving ambiguities. if (!rule.isAttributeValueExplicitlySpecified(RuleClass.COMPATIBLE_ENVIRONMENT_ATTR)) { - addExplicitDeps(result, rule, RuleClass.COMPATIBLE_ENVIRONMENT_ATTR, - rule.getPackage().getDefaultCompatibleWith(), configuration); + addExplicitDeps(depResolver, RuleClass.COMPATIBLE_ENVIRONMENT_ATTR, + rule.getPackage().getDefaultCompatibleWith()); } if (!rule.isAttributeValueExplicitlySpecified(RuleClass.RESTRICTED_ENVIRONMENT_ATTR)) { - addExplicitDeps(result, rule, RuleClass.RESTRICTED_ENVIRONMENT_ATTR, - rule.getPackage().getDefaultRestrictedTo(), configuration); + addExplicitDeps(depResolver, RuleClass.RESTRICTED_ENVIRONMENT_ATTR, + rule.getPackage().getDefaultRestrictedTo()); } - - return result.build(); } - /** - * Adds new dependencies to the given rule under the given attribute name - * - * @param result the builder for the attribute --> dependency labels map - * @param rule the rule being evaluated - * @param attrName the name of the attribute to add dependency labels to - * @param labels the dependencies to add - * @param configuration the configuration to apply to those dependencies - */ - private void addExplicitDeps( - ImmutableSortedKeyListMultimap.Builder<Attribute, LabelAndConfiguration> result, Rule rule, - String attrName, Iterable<Label> labels, BuildConfiguration configuration) { - if (!rule.isAttrDefined(attrName, BuildType.LABEL_LIST) - && !rule.isAttrDefined(attrName, BuildType.NODEP_LABEL_LIST)) { - return; - } - Attribute attribute = rule.getRuleClassObject().getAttributeByName(attrName); - for (Label label : labels) { - // The configuration must be the configuration after the first transition step (applying - // split configurations). The proper configuration (null) for package groups will be set - // later. - result.put(attribute, LabelAndConfiguration.of(label, configuration)); - } - } - - private void resolveExplicitAttributes(final BuildConfiguration configuration, - AttributeMap attributes, - final ImmutableSortedKeyListMultimap.Builder<Attribute, LabelAndConfiguration> builder) { - attributes.visitLabels( + private void resolveExplicitAttributes(final RuleResolver depResolver) { + depResolver.attributeMap.visitLabels( new AttributeMap.AcceptsLabelAttribute() { @Override public void acceptLabelAttribute(Label label, Attribute attribute) { @@ -253,21 +239,24 @@ public abstract class DependencyResolver { || attribute.isLateBound()) { return; } - builder.put(attribute, LabelAndConfiguration.of(label, configuration)); + depResolver.resolveDep(attribute, label); } }); } - private void resolveImplicitAttributes(Rule rule, BuildConfiguration configuration, - AttributeMap attributeMap, Iterable<Attribute> attributes, - ImmutableSortedKeyListMultimap.Builder<Attribute, LabelAndConfiguration> builder) { + /** + * Resolves the dependencies for all implicit attributes in this rule. + */ + private void resolveImplicitAttributes(RuleResolver depResolver) { // Since the attributes that come from aspects do not appear in attributeMap, we have to get // their values from somewhere else. This incidentally means that aspects attributes are not // configurable. It would be nice if that wasn't the case, but we'd have to revamp how // attribute mapping works, which is a large chunk of work. + Rule rule = depResolver.rule; Label ruleLabel = rule.getLabel(); + ConfiguredAttributeMapper attributeMap = depResolver.attributeMap; ImmutableSet<String> mappedAttributes = ImmutableSet.copyOf(attributeMap.getAttributeNames()); - for (Attribute attribute : attributes) { + for (Attribute attribute : depResolver.attributes) { if (!attribute.isImplicit() || !attribute.getCondition().apply(attributeMap)) { continue; } @@ -279,7 +268,7 @@ public abstract class DependencyResolver { if (label != null) { label = ruleLabel.resolveRepositoryRelative(label); - builder.put(attribute, LabelAndConfiguration.of(label, configuration)); + depResolver.resolveDep(attribute, label); } } else if (attribute.getType() == BuildType.LABEL_LIST) { List<Label> labelList; @@ -291,85 +280,160 @@ public abstract class DependencyResolver { } else { labelList = BuildType.LABEL_LIST.cast(attribute.getDefaultValue(rule)); } - for (Label label : labelList) { - label = ruleLabel.resolveRepositoryRelative(label); - builder.put(attribute, LabelAndConfiguration.of(label, configuration)); + depResolver.resolveDep(attribute, ruleLabel.resolveRepositoryRelative(label)); } } } } + /** + * Resolves the dependencies for all late-bound attributes in this rule. + * + * <p>Late-bound attributes need special handling because they require configuration + * transitions to determine their values. + * + * <p>In other words, the normal process of dependency resolution is: + * <ol> + * <li>Find every label value in the rule's attributes</li> + * <li>Apply configuration transitions over each value to get its dep configuration + * <li>Return each value with its dep configuration</li> + * </ol> + * + * This doesn't work for late-bound attributes because you can't get their values without + * knowing the configuration first. And that configuration may not be the owning rule's + * configuration. Specifically, {@link LateBoundDefault#useHostConfiguration()} switches to the + * host config and late-bound split attributes branch into multiple split configs. + * + * <p>This method implements that logic and makes sure the normal configuration + * transition logic mixes with it cleanly. + * + * @param depResolver the resolver for this rule's deps + * @param ruleConfig the rule's configuration + * @param hostConfig the equivalent host configuration + */ private void resolveLateBoundAttributes( - Rule rule, - BuildConfiguration configuration, - BuildConfiguration hostConfiguration, - AttributeMap attributeMap, - Iterable<Attribute> attributes, - ImmutableSortedKeyListMultimap.Builder<Attribute, LabelAndConfiguration> builder) + RuleResolver depResolver, + BuildConfiguration ruleConfig, + BuildConfiguration hostConfig) throws EvalException, InterruptedException { - for (Attribute attribute : attributes) { + ConfiguredAttributeMapper attributeMap = depResolver.attributeMap; + for (Attribute attribute : depResolver.attributes) { if (!attribute.isLateBound() || !attribute.getCondition().apply(attributeMap)) { continue; } - List<BuildConfiguration> actualConfigurations = ImmutableList.of(configuration); - if (attribute.hasSplitConfigurationTransition()) { - Preconditions.checkState(attribute.getConfigurator() == null); - // TODO(bazel-team): This ends up applying the split transition twice, both here and in the - // visitRule method below - this is not currently a problem, because the configuration graph - // never contains nested split transitions, so the second application is idempotent. - actualConfigurations = - configuration.getSplitConfigurations(attribute.getSplitTransition(rule)); - } + @SuppressWarnings("unchecked") + LateBoundDefault<BuildConfiguration> lateBoundDefault = + (LateBoundDefault<BuildConfiguration>) attribute.getLateBoundDefault(); - for (BuildConfiguration actualConfig : actualConfigurations) { - @SuppressWarnings("unchecked") - LateBoundDefault<BuildConfiguration> lateBoundDefault = - (LateBoundDefault<BuildConfiguration>) attribute.getLateBoundDefault(); - if (lateBoundDefault.useHostConfiguration()) { - actualConfig = hostConfiguration; - } - // TODO(bazel-team): This might be too expensive - can we cache this somehow? - if (!lateBoundDefault.getRequiredConfigurationFragments().isEmpty()) { - if (!actualConfig.hasAllFragments(lateBoundDefault.getRequiredConfigurationFragments())) { - continue; + if (attribute.hasSplitConfigurationTransition()) { + // Late-bound attribute with a split transition: + // Since we want to get the same results as BuildConfiguration.evaluateTransition (but + // skip it since we've already applied the split), we want to make sure this logic + // doesn't do anything differently. evaluateTransition has additional logic + // for host configs and attributes with configurators. So we check here that neither of + // of those apply, in the name of keeping the fork as simple as possible. + Verify.verify(attribute.getConfigurator() == null); + Verify.verify(!lateBoundDefault.useHostConfiguration()); + for (BuildConfiguration splitConfig : ruleConfig.getSplitConfigurations( + attribute.getSplitTransition(depResolver.rule))) { + // TODO(gregce): support dynamic split transitions + for (Label dep : resolveLateBoundAttribute( + depResolver.rule, attribute, splitConfig, attributeMap)) { + // Skip the normal config transition pipeline and directly feed the split config. This + // is because the split already had to be applied to determine the attribute's value. + // This makes the split logic in the normal pipeline redundant and potentially + // incorrect. + depResolver.resolveDep(attribute, dep, splitConfig); } } - - // TODO(bazel-team): We should check if the implementation tries to access an undeclared - // fragment. - Object actualValue = lateBoundDefault.resolve(rule, attributeMap, actualConfig); - if (EvalUtils.isNullOrNone(actualValue)) { - continue; + } else { + // Late-bound attribute without a split transition: + for (Label dep : resolveLateBoundAttribute(depResolver.rule, attribute, + lateBoundDefault.useHostConfiguration() ? hostConfig : ruleConfig, attributeMap)) { + // Process this dep like a normal attribute. + depResolver.resolveDep(attribute, dep); } - try { - if (attribute.getType() == BuildType.LABEL) { - Label label = rule.getLabel().resolveRepositoryRelative( - BuildType.LABEL.cast(actualValue)); - builder.put(attribute, LabelAndConfiguration.of(label, actualConfig)); - } else if (attribute.getType() == BuildType.LABEL_LIST) { - for (Label label : BuildType.LABEL_LIST.cast(actualValue)) { - builder.put(attribute, LabelAndConfiguration.of( - rule.getLabel().resolveRepositoryRelative(label), - actualConfig)); - } - } else { - throw new IllegalStateException( - String.format( - "Late bound attribute '%s' is not a label or a label list", - attribute.getName())); - } - } catch (ClassCastException e) { // From either of the cast calls above. - throw new EvalException( - rule.getLocation(), - String.format( - "When computing the default value of %s, expected '%s', got '%s'", - attribute.getName(), - attribute.getType(), - EvalUtils.getDataTypeName(actualValue, true))); + } + } + } + + /** + * Returns the label dependencies for the given late-bound attribute in this rule. + * + * @param rule the rule being evaluated + * @param attribute the attribute to evaluate + * @param config the configuration to evaluate the attribute in + * @param attributeMap mapper to attribute values + */ + private Iterable<Label> resolveLateBoundAttribute( + Rule rule, + Attribute attribute, + BuildConfiguration config, + AttributeMap attributeMap) + throws EvalException, InterruptedException { + Preconditions.checkArgument(attribute.isLateBound()); + + @SuppressWarnings("unchecked") + LateBoundDefault<BuildConfiguration> lateBoundDefault = + (LateBoundDefault<BuildConfiguration>) attribute.getLateBoundDefault(); + + // TODO(bazel-team): This might be too expensive - can we cache this somehow? + if (!lateBoundDefault.getRequiredConfigurationFragments().isEmpty()) { + if (!config.hasAllFragments(lateBoundDefault.getRequiredConfigurationFragments())) { + return ImmutableList.<Label>of(); + } + } + + // TODO(bazel-team): We should check if the implementation tries to access an undeclared + // fragment. + Object actualValue = lateBoundDefault.resolve(rule, attributeMap, config); + if (EvalUtils.isNullOrNone(actualValue)) { + return ImmutableList.<Label>of(); + } + try { + ImmutableList.Builder<Label> deps = ImmutableList.builder(); + if (attribute.getType() == BuildType.LABEL) { + deps.add(rule.getLabel().resolveRepositoryRelative(BuildType.LABEL.cast(actualValue))); + } else if (attribute.getType() == BuildType.LABEL_LIST) { + for (Label label : BuildType.LABEL_LIST.cast(actualValue)) { + deps.add(rule.getLabel().resolveRepositoryRelative(label)); } + } else { + throw new IllegalStateException( + String.format( + "Late bound attribute '%s' is not a label or a label list", + attribute.getName())); } + return deps.build(); + } catch (ClassCastException e) { // From either of the cast calls above. + throw new EvalException( + rule.getLocation(), + String.format( + "When computing the default value of %s, expected '%s', got '%s'", + attribute.getName(), + attribute.getType(), + EvalUtils.getDataTypeName(actualValue, true))); + } + } + + /** + * Adds new dependencies to the given rule under the given attribute name + * + * @param depResolver the resolver for this rule's deps + * @param attrName the name of the attribute to add dependency labels to + * @param labels the dependencies to add + */ + private void addExplicitDeps(RuleResolver depResolver, String attrName, Iterable<Label> labels) { + Rule rule = depResolver.rule; + if (!rule.isAttrDefined(attrName, BuildType.LABEL_LIST) + && !rule.isAttrDefined(attrName, BuildType.NODEP_LABEL_LIST)) { + return; + } + Attribute attribute = rule.getRuleClassObject().getAttributeByName(attrName); + for (Label label : labels) { + depResolver.resolveDep(attribute, label); } } @@ -379,13 +443,18 @@ public abstract class DependencyResolver { * * @throws IllegalArgumentException if the {@code node} does not refer to a {@link Rule} instance */ - public final Collection<Dependency> resolveRuleLabels( - TargetAndConfiguration node, ListMultimap<Attribute, - LabelAndConfiguration> labelMap, NestedSetBuilder<Label> rootCauses) { + public final Collection<Dependency> resolveRuleLabels(TargetAndConfiguration node, + ListMultimap<Attribute, Label> depLabels, NestedSetBuilder<Label> rootCauses) { Preconditions.checkArgument(node.getTarget() instanceof Rule); Rule rule = (Rule) node.getTarget(); ListMultimap<Attribute, Dependency> outgoingEdges = ArrayListMultimap.create(); - visitRule(rule, labelMap, rootCauses, outgoingEdges); + RuleResolver depResolver = new RuleResolver(rule, node.getConfiguration(), /*aspect=*/null, + /*attributeMap=*/null, rootCauses, outgoingEdges); + for (Map.Entry<Attribute, Collection<Label>> entry : depLabels.asMap().entrySet()) { + for (Label depLabel : entry.getValue()) { + depResolver.resolveDep(entry.getKey(), depLabel); + } + } return outgoingEdges.values(); } @@ -409,7 +478,7 @@ public abstract class DependencyResolver { } private ImmutableSet<AspectDescriptor> requiredAspects( - Aspect aspect, Attribute attribute, final Target target, Rule originalRule) { + @Nullable Aspect aspect, Attribute attribute, final Target target, Rule originalRule) { if (!(target instanceof Rule)) { return ImmutableSet.of(); } @@ -432,7 +501,7 @@ public abstract class DependencyResolver { } private static Iterable<Aspect> extractAspectCandidates( - Aspect aspect, Attribute attribute, Rule originalRule) { + @Nullable Aspect aspect, Attribute attribute, Rule originalRule) { // The order of this set will be deterministic. This is necessary because this order eventually // influences the order in which aspects are merged into the main configured target, which in // turn influences which aspect takes precedence if two emit the same provider (maybe this @@ -458,58 +527,101 @@ public abstract class DependencyResolver { return aspectCandidates; } - private void visitRule(Rule rule, - ListMultimap<Attribute, LabelAndConfiguration> labelMap, NestedSetBuilder<Label> rootCauses, - ListMultimap<Attribute, Dependency> outgoingEdges) { - visitRule(rule, /*aspect=*/ null, labelMap, rootCauses, outgoingEdges); - } + /** + * Supplies the logic for translating <Attribute, Label> pairs for a rule into the + * <Attribute, Dependency> pairs DependencyResolver ultimately returns. + * + * <p>The main difference between the two is that the latter applies configuration transitions, + * i.e. it specifies not just which deps a rule has but also the configurations those deps + * should take. + */ + private class RuleResolver { + private final Rule rule; + private final BuildConfiguration ruleConfig; + private final Aspect aspect; + private final ConfiguredAttributeMapper attributeMap; + private final NestedSetBuilder<Label> rootCauses; + private final ListMultimap<Attribute, Dependency> outgoingEdges; + private final List<Attribute> attributes; + + /** + * Constructs a new dependency resolver for the specified rule context. + * + * @param rule the rule being evaluated + * @param ruleConfig the rule's configuration + * @param aspect the aspect applied to this rule (if any) + * @param attributeMap mapper for the rule's attribute values + * @param rootCauses output collector for dep labels that can't be (loading phase) loaded + * @param outgoingEdges output collector for the resolved dependencies + */ + RuleResolver(Rule rule, BuildConfiguration ruleConfig, Aspect aspect, + ConfiguredAttributeMapper attributeMap, NestedSetBuilder<Label> rootCauses, + ListMultimap<Attribute, Dependency> outgoingEdges) { + this.rule = rule; + this.ruleConfig = ruleConfig; + this.aspect = aspect; + this.attributeMap = attributeMap; + this.rootCauses = rootCauses; + this.outgoingEdges = outgoingEdges; + this.attributes = getAttributes(rule, aspect); + } - private void visitRule( - Rule rule, - Aspect aspect, - ListMultimap<Attribute, LabelAndConfiguration> labelMap, - NestedSetBuilder<Label> rootCauses, - ListMultimap<Attribute, Dependency> outgoingEdges) { - Preconditions.checkNotNull(labelMap); - for (Map.Entry<Attribute, Collection<LabelAndConfiguration>> entry : - labelMap.asMap().entrySet()) { - Attribute attribute = entry.getKey(); - for (LabelAndConfiguration dep : entry.getValue()) { - Label label = dep.getLabel(); - BuildConfiguration config = dep.getConfiguration(); - - Target toTarget = getTarget(rule, label, rootCauses); - if (toTarget == null) { - continue; - } - BuildConfiguration.TransitionApplier transitionApplier = config.getTransitionApplier(); - if (config.useDynamicConfigurations() && config.isHostConfiguration() - && !BuildConfiguration.usesNullConfiguration(toTarget)) { - // This condition is needed because resolveLateBoundAttributes may switch config to - // the host configuration, which is the only case DependencyResolver applies a - // configuration transition outside of this method. We need to reflect that - // transition in the results of this method, but config.evaluateTransition is hard-set - // to return a NONE transition when the input is a host config. Since the outside - // caller originally passed the *original* value of config (before the possible - // switch), it can mistakenly interpret the result as a NONE transition from the - // original value of config. This condition fixes that. Another fix would be to have - // config.evaluateTransition return a HOST transition when the input config is a host, - // but since this blemish is specific to DependencyResolver it seems best to keep the - // fix here. - // TODO(bazel-team): eliminate this special case by passing transitionApplier to - // resolveLateBoundAttributes, so that method uses the same interface for transitions. - transitionApplier.applyTransition(Attribute.ConfigurationTransition.HOST); - } else { - config.evaluateTransition(rule, attribute, toTarget, transitionApplier); - } - for (Dependency dependency : - transitionApplier.getDependencies( - label, requiredAspects(aspect, attribute, toTarget, rule))) { - outgoingEdges.put( - entry.getKey(), - dependency); - } + /** + * Returns the attributes that should be visited for this rule/aspect combination. + */ + private List<Attribute> getAttributes(Rule rule, @Nullable Aspect aspect) { + List<Attribute> ruleDefs = rule.getRuleClassObject().getAttributes(); + if (aspect == null) { + return ruleDefs; + } else { + return ImmutableList.<Attribute>builder() + .addAll(ruleDefs) + .addAll(aspect.getDefinition().getAttributes().values()) + .build(); + } + } + + /** + * Resolves the given dep for the given attribute, including determining which + * configurations to apply to it. + */ + void resolveDep(Attribute attribute, Label depLabel) { + // Late-bound split attributes are separately handled (see LateBoundSplitResolver). + Verify.verify(!(attribute.isLateBound() && attribute.hasSplitConfigurationTransition())); + Target toTarget = getTarget(rule, depLabel, rootCauses); + if (toTarget == null) { + return; // Skip this round: we still need to Skyframe-evaluate the dep's target. + } + BuildConfiguration.TransitionApplier resolver = ruleConfig.getTransitionApplier(); + ruleConfig.evaluateTransition(rule, attribute, toTarget, resolver); + // An <Attribute, Label> pair can resolve to multiple deps because of split transitions. + for (Dependency dependency : + resolver.getDependencies(depLabel, requiredAspects(aspect, attribute, toTarget, rule))) { + outgoingEdges.put(attribute, dependency); + } + } + + /** + * Resolves the given dep for the given attribute using a pre-prepared configuration. + * + * <p>Use this method with care: it skips Bazel's standard config transition semantics + * ({@link BuildConfiguration#evaluateTransition}). That means attributes passed through here + * won't obey standard rules on which configurations apply to their deps. This should only + * be done for special circumstances that really justify the difference. When in doubt, use + * {@link #resolveDep(Attribute, Label)}. + */ + void resolveDep(Attribute attribute, Label depLabel, BuildConfiguration config) { + Target toTarget = getTarget(rule, depLabel, rootCauses); + if (toTarget == null) { + return; // Skip this round: this is either a loading error or unevaluated Skyframe dep. + } + BuildConfiguration.TransitionApplier transitionApplier = config.getTransitionApplier(); + if (BuildConfiguration.usesNullConfiguration(toTarget)) { + transitionApplier.applyTransition(Attribute.ConfigurationTransition.NULL); } + Dependency dep = Iterables.getOnlyElement(transitionApplier.getDependencies(depLabel, + requiredAspects(aspect, attribute, toTarget, rule))); + outgoingEdges.put(attribute, dep); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 5277aefa33..2ac485195d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -1090,8 +1090,12 @@ public final class Attribute implements Comparable<Attribute> { */ public interface LateBoundDefault<T> { /** - * Whether to look up the label in the host configuration. This is only here for the host JDK - - * we usually need to look up labels in the target configuration. + * Whether to look up the label in the host configuration. This is only here for host + * compilation tools - we usually need to look up labels in the target configuration. + * + * <p>This method only sets the configuration passed to {@link #resolve}. If you want the + * dependency to also be analyzed in the host configuration, use + * {@link ConfigurationTransition#HOST}. */ boolean useHostConfiguration(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index b0a8137c57..18fbe8ce74 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -710,14 +710,12 @@ final class ConfiguredTargetFunction implements SkyFunction { Map<Label, ConfigMatchingProvider> configConditions = new LinkedHashMap<>(); // Collect the labels of the configured targets we need to resolve. - ListMultimap<Attribute, LabelAndConfiguration> configLabelMap = ArrayListMultimap.create(); + ListMultimap<Attribute, Label> configLabelMap = ArrayListMultimap.create(); RawAttributeMapper attributeMap = RawAttributeMapper.of(((Rule) target)); for (Attribute a : ((Rule) target).getAttributes()) { for (Label configLabel : attributeMap.getConfigurabilityKeys(a.getName(), a.getType())) { if (!BuildType.Selector.isReservedLabel(configLabel)) { - configLabelMap.put(a, LabelAndConfiguration.of( - target.getLabel().resolveRepositoryRelative(configLabel), - ctgValue.getConfiguration())); + configLabelMap.put(a, target.getLabel().resolveRepositoryRelative(configLabel)); } } } |