Skip to content

Commit

Permalink
Fix toolchain deps for --noimplicit_deps cquery.
Browse files Browse the repository at this point in the history 
    Toolchain deps were not being caught by implicit deps filtering because there's no way to tell them apart from regular deps. Thus, we add (back) logic to do it during ruleContext building. This was caught by a user in unknown commit when they fixed our test for us. The test fix in included in this CL. Fixing the test triggered the bug.

    Fixes bazelbuild/bazel#11993

    PiperOrigin-RevId: 333367049
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent d782801 commit 2245bd2
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 183 deletions.
80 changes: 26 additions & 54 deletions ...lbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.AspectCollection.AspectCycleOnPathException;
import com.google.devtools.build.lib.analysis.DependencyKind.AttributeDependencyKind;
import com.google.devtools.build.lib.analysis.DependencyKind.ToolchainDependencyKind;
Expand All @@ -39,6 +40,7 @@
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.AspectDescriptor;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault;
Expand All @@ -61,7 +63,8 @@
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.syntax.Location;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;

/**
* Resolver for dependencies between configured targets.
Expand Down Expand Up @@ -183,7 +186,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
boolean useToolchainTransition,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory)
throws Failure, InterruptedException, InconsistentAspectOrderException {
throws EvalException, InterruptedException, InconsistentAspectOrderException {
NestedSetBuilder<Cause> rootCauses = NestedSetBuilder.stableOrder();
OrderedSetMultimap<DependencyKind, DependencyKey> outgoingEdges =
dependentNodeMap(
Expand All @@ -210,7 +213,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
* representing the given target and configuration.
*
* <p>Otherwise {@code aspects} represents an aspect path. The function returns dependent nodes of
* the entire path applied to given target and configuration. These are the dependent nodes of the
* the entire path applied to given target and configuration. These are the depenent nodes of the
* last aspect in the path.
*
* <p>This also implements the first step of applying configuration transitions, namely, split
Expand Down Expand Up @@ -241,7 +244,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
boolean useToolchainTransition,
NestedSetBuilder<Cause> rootCauses,
@Nullable TransitionFactory<Rule> trimmingTransitionFactory)
throws Failure, InterruptedException, InconsistentAspectOrderException {
throws EvalException, InterruptedException, InconsistentAspectOrderException {
Target target = node.getTarget();
BuildConfiguration config = node.getConfiguration();
OrderedSetMultimap<DependencyKind, Label> outgoingLabels = OrderedSetMultimap.create();
Expand Down Expand Up @@ -303,11 +306,11 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
partiallyResolveDependencies(
OrderedSetMultimap<DependencyKind, Label> outgoingLabels,
@Nullable Rule fromRule,
@Nullable ConfiguredAttributeMapper attributeMap,
ConfiguredAttributeMapper attributeMap,
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
boolean useToolchainTransition,
Iterable<Aspect> aspects)
throws Failure {
throws EvalException {
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps =
OrderedSetMultimap.create();

Expand Down Expand Up @@ -368,19 +371,6 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
continue;
}

// Compute the set of aspects that could be applied to a dependency. This is composed of two
// parts:
//
// 1. The aspects are visible to this aspect being evaluated, if any (if another aspect is
// visible on the configured target for this one, it should also be visible on the
// dependencies for consistency). This is the argument "aspects".
// 2. The aspects propagated by the attributes of this configured target / aspect. This is
// computed by collectPropagatingAspects().
//
// The presence of an aspect here does not necessarily mean that it will be available on a
// dependency: it can still be filtered out because it requires a provider that the configured
// target it should be attached to it doesn't advertise. This is taken into account in
// computeAspectCollections() once the Target instances for the dependencies are known.
Attribute attribute = entry.getKey().getAttribute();
ImmutableList.Builder<Aspect> propagatingAspects = ImmutableList.builder();
propagatingAspects.addAll(attribute.getAspects(fromRule));
Expand All @@ -393,11 +383,15 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
String execGroup =
((ExecutionTransitionFactory) attribute.getTransitionFactory()).getExecGroup();
if (!toolchainContexts.hasToolchainContext(execGroup)) {
throw new Failure(
fromRule != null ? fromRule.getLocation() : null,
String error =
String.format(
"Attr '%s' declares a transition for non-existent exec group '%s'",
attribute.getName(), execGroup));
attribute.getName(), execGroup);
if (fromRule != null) {
throw new EvalException(fromRule.getLocation(), error);
} else {
throw Starlark.errorf("%s", error);
}
}
if (toolchainContexts.getToolchainContext(execGroup).executionPlatform() != null) {
executionPlatformLabel =
Expand Down Expand Up @@ -461,7 +455,7 @@ private OrderedSetMultimap<DependencyKind, DependencyKey> fullyResolveDependenci
trimmingTransitionFactory);

AspectCollection requiredAspects =
computeAspectCollections(partiallyResolvedDependency.getPropagatingAspects(), toTarget);
filterPropagatingAspects(partiallyResolvedDependency.getPropagatingAspects(), toTarget);

DependencyKey.Builder dependencyKeyBuilder =
partiallyResolvedDependency.getDependencyKeyBuilder();
Expand All @@ -472,39 +466,19 @@ private OrderedSetMultimap<DependencyKind, DependencyKey> fullyResolveDependenci
return outgoingEdges;
}

/** A DependencyResolver.Failure indicates a failure during dependency resolution. */
public static class Failure extends Exception {
@Nullable private final Location location;

private Failure(Location location, String message) {
super(message);
this.location = location;
}

/** Returns the location of the error, if known. */
@Nullable
public Location getLocation() {
return location;
}
}

private void visitRule(
TargetAndConfiguration node,
BuildConfiguration hostConfig,
Iterable<Aspect> aspects,
ConfiguredAttributeMapper attributeMap,
@Nullable ToolchainCollection<ToolchainContext> toolchainContexts,
OrderedSetMultimap<DependencyKind, Label> outgoingLabels)
throws Failure {
throws EvalException {
Preconditions.checkArgument(node.getTarget() instanceof Rule, node);
BuildConfiguration ruleConfig = Preconditions.checkNotNull(node.getConfiguration(), node);
Rule rule = (Rule) node.getTarget();

try {
attributeMap.validateAttributes();
} catch (ConfiguredAttributeMapper.ValidationException ex) {
throw new Failure(rule.getLocation(), ex.getMessage());
}
attributeMap.validateAttributes();

visitTargetVisibility(node, outgoingLabels);
resolveAttributes(outgoingLabels, rule, attributeMap, aspects, ruleConfig, hostConfig);
Expand Down Expand Up @@ -743,15 +717,11 @@ private List<AttributeDependencyKind> getAttributes(Rule rule, Iterable<Aspect>
}

/**
* Compute the way aspects should be computed for the direct dependencies.
*
* <p>This is done by filtering the aspects that can be propagated on any attribute according to
* the providers advertised by direct dependencies and by creating the {@link AspectCollection}
* that tells how to compute the final set of providers based on the interdependencies between the
* propagating aspects.
* Filter the set of aspects that are to be propagated according to the dependency type and the
* set of advertised providers of the dependency.
*/
private static AspectCollection computeAspectCollections(
ImmutableList<Aspect> aspects, Target toTarget) throws InconsistentAspectOrderException {
private AspectCollection filterPropagatingAspects(ImmutableList<Aspect> aspects, Target toTarget)
throws InconsistentAspectOrderException {
if (toTarget instanceof OutputFile) {
aspects =
aspects.stream()
Expand All @@ -766,17 +736,19 @@ private static AspectCollection computeAspectCollections(

Rule toRule = (Rule) toTarget;
ImmutableList.Builder<Aspect> filteredAspectPath = ImmutableList.builder();
ImmutableSet.Builder<AspectDescriptor> visibleAspects = ImmutableSet.builder();

for (Aspect aspect : aspects) {
if (aspect
.getDefinition()
.getRequiredProviders()
.isSatisfiedBy(toRule.getRuleClassObject().getAdvertisedProviders())) {
filteredAspectPath.add(aspect);
visibleAspects.add(aspect.getDescriptor());
}
}
try {
return AspectCollection.create(filteredAspectPath.build());
return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build());
} catch (AspectCycleOnPathException e) {
throw new InconsistentAspectOrderException(toTarget, e);
}
Expand Down
91 changes: 82 additions & 9 deletions ...tHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/Util.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,26 @@

package com.google.devtools.build.lib.analysis;

import com.google.common.collect.ImmutableSet;
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.cmdline.Label;
import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.List;
import java.util.Set;

/**
* Utility methods for use by ConfiguredTarget implementations.
*/
/** Utility methods for use by ConfiguredTarget implementations. */
public abstract class Util {

private Util() {}

//---------- Label and Target related methods
// ---------- Label and Target related methods

/**
* Returns the workspace-relative path of the specified target (file or rule).
Expand All @@ -46,19 +54,84 @@ public static PathFragment getWorkspaceRelativePath(Label label) {
}

/**
* Returns the workspace-relative path of the specified target (file or rule),
* prepending a prefix and appending a suffix.
* Returns the workspace-relative path of the specified target (file or rule), prepending a prefix
* and appending a suffix.
*
* <p>For example, "//foo/bar:wiz" and "//foo:bar/wiz" both result in "foo/bar/wiz".
*/
public static PathFragment getWorkspaceRelativePath(Target target, String prefix, String suffix) {
return target.getLabel().getPackageFragment().getRelative(prefix + target.getName() + suffix);
}

/**
* Checks if a PathFragment contains a '-'.
*/
/** Checks if a PathFragment contains a '-'. */
public static boolean containsHyphen(PathFragment path) {
return path.getPathString().indexOf('-') >= 0;
}

// ---------- Implicit dependency extractor

/*
* Given a RuleContext, find all the implicit attribute deps aka deps that weren't explicitly set
* in the build file but are attached behind the scenes to some attribute. This means this
* function does *not* cover deps attached other ways e.g. toolchain-related implicit deps
* (see {@link PostAnalysisQueryEnvironment#targetifyValues} for more info on further implicit
* deps filtering).
* note: nodes that are depended on both implicitly and explicitly are considered explicit.
*/
public static ImmutableSet<ConfiguredTargetKey> findImplicitDeps(RuleContext ruleContext) {
Set<ConfiguredTargetKey> maybeImplicitDeps = CompactHashSet.create();
Set<ConfiguredTargetKey> explicitDeps = CompactHashSet.create();
// Consider rule attribute dependencies.
AttributeMap attributes = ruleContext.attributes();
ListMultimap<String, ConfiguredTargetAndData> targetMap =
ruleContext.getConfiguredTargetAndDataMap();
for (String attrName : attributes.getAttributeNames()) {
List<ConfiguredTargetAndData> attrValues = targetMap.get(attrName);
if (attrValues != null && !attrValues.isEmpty()) {
if (attributes.isAttributeValueExplicitlySpecified(attrName)) {
addLabelsAndConfigs(explicitDeps, attrValues);
} else {
addLabelsAndConfigs(maybeImplicitDeps, attrValues);
}
}
}
// Consider toolchain dependencies.
ToolchainContext toolchainContext = ruleContext.getToolchainContext();
if (toolchainContext != null) {
// This logic should stay up to date with the dep creation logic in
// DependencyResolver#partiallyResolveDependencies.
BuildConfiguration targetConfiguration = ruleContext.getConfiguration();
BuildConfiguration hostConfiguration = ruleContext.getHostConfiguration();
for (Label toolchain : toolchainContext.resolvedToolchainLabels()) {
if (DependencyResolver.shouldUseToolchainTransition(
targetConfiguration, ruleContext.getRule())) {
maybeImplicitDeps.add(
ConfiguredTargetKey.builder()
.setLabel(toolchain)
.setConfiguration(targetConfiguration)
.setToolchainContextKey(toolchainContext.key())
.build());
} else {
maybeImplicitDeps.add(
ConfiguredTargetKey.builder()
.setLabel(toolchain)
.setConfiguration(hostConfiguration)
.build());
}
}
}
return ImmutableSet.copyOf(Sets.difference(maybeImplicitDeps, explicitDeps));
}

private static void addLabelsAndConfigs(
Set<ConfiguredTargetKey> set, List<ConfiguredTargetAndData> deps) {
for (ConfiguredTargetAndData dep : deps) {
// Dereference any aliases that might be present.
set.add(
ConfiguredTargetKey.builder()
.setLabel(dep.getConfiguredTarget().getOriginalLabel())
.setConfiguration(dep.getConfiguration())
.build());
}
}
}
Loading

0 comments on commit 2245bd2

Please sign in to comment.