Skip to content

Commit

Permalink
explcitly traverse aspects in cquery
Browse files Browse the repository at this point in the history
This CL adds aspect nodes as explicit targets in cquery deps by making
ConfiguredTargetQueryEnvironment generic over a new interface,
CqueryNode, rather than ConfiguredTarget.

CqueryNode is implemented by AspectKey as well as ConfiguredTarget so
that both can be traversed in a deps query.

PiperOrigin-RevId: 595509373
Change-Id: I8a637cd3ed640907d2b1501bcd2b4a4507d183e7
  • Loading branch information
Googler authored and meteorcloudy committed Mar 5, 2024
1 parent 9e07fac commit a305a0d
Show file tree
Hide file tree
Showing 39 changed files with 691 additions and 237 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/profiler/memory:allocationtracker",
"//src/main/java/com/google/devtools/build/lib/query2",
"//src/main/java/com/google/devtools/build/lib/query2:aquery-utils",
"//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
"//src/main/java/com/google/devtools/build/lib/query2/engine",
"//src/main/java/com/google/devtools/build/lib/query2/query/output",
"//src/main/java/com/google/devtools/build/lib/runtime/events",
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/profiler:google-auto-profiler-utils",
"//src/main/java/com/google/devtools/build/lib/profiler/memory:current_rule_tracker",
"//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/skyframe:aspect_key_creator",
"//src/main/java/com/google/devtools/build/lib/skyframe:build_info_collection_value",
Expand Down Expand Up @@ -692,6 +693,7 @@ java_library(
":transitive_info_collection",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
"//src/main/java/com/google/devtools/build/lib/skyframe/config",
"//src/main/java/net/starlark/java/eval",
"//third_party:guava",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.query2.common.CqueryNode;
import com.google.devtools.build.lib.skyframe.config.BuildConfigurationKey;
import javax.annotation.Nullable;
import net.starlark.java.eval.Dict;
Expand All @@ -35,7 +36,7 @@
* their direct dependencies, only the corresponding {@link TransitiveInfoCollection}s. Also, {@link
* ConfiguredTarget} objects should not be accessible from the action graph.
*/
public interface ConfiguredTarget extends TransitiveInfoCollection, Structure {
public interface ConfiguredTarget extends TransitiveInfoCollection, Structure, CqueryNode {

/** All <code>ConfiguredTarget</code>s have a "label" field. */
String LABEL_FIELD = "label";
Expand All @@ -44,13 +45,15 @@ public interface ConfiguredTarget extends TransitiveInfoCollection, Structure {
String FILES_FIELD = "files";

/** Returns a key that may be used to lookup this {@link ConfiguredTarget}. */
@Override
ActionLookupKey getLookupKey();

@Override
default Label getLabel() {
return getLookupKey().getLabel();
}

@Override
@Nullable
default String getConfigurationChecksum() {
return getConfigurationKey() == null ? null : getConfigurationKey().getOptions().checksum();
Expand All @@ -65,8 +68,9 @@ default String getConfigurationChecksum() {
* com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget} for
* which it is always <b>null</b>.
*
* <p>If this changes, {@link AspectResolver#aspecMatchesConfiguredTarget} should be updated.
* <p>If this changes, {@link AspectResolver#aspectMatchesConfiguredTarget} should be updated.
*/
@Override
@Nullable
default BuildConfigurationKey getConfigurationKey() {
return getLookupKey().getConfigurationKey();
Expand All @@ -89,6 +93,7 @@ default BuildConfigurationKey getConfigurationKey() {
* If the configured target is an alias, return the actual target, otherwise return the current
* target. This follows alias chains.
*/
@Override
default ConfiguredTarget getActual() {
return this;
}
Expand All @@ -98,6 +103,7 @@ default ConfiguredTarget getActual() {
* label. This is not the same as {@code getActual().getLabel()}, because it does not follow alias
* chains.
*/
@Override
default Label getOriginalLabel() {
return getLabel();
}
Expand All @@ -106,10 +112,12 @@ default Label getOriginalLabel() {
* The configuration conditions that trigger this configured target's configurable attributes. For
* targets that do not support configurable attributes, this will be an empty map.
*/
@Override
default ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions() {
return ImmutableMap.of();
}

@Override
default boolean isRuleConfiguredTarget() {
return false;
}
Expand All @@ -119,6 +127,7 @@ default boolean isRuleConfiguredTarget() {
*
* <p>Unwrapping is recursive if there are multiple layers.
*/
@Override
default ConfiguredTarget unwrapIfMerged() {
return this;
}
Expand All @@ -128,6 +137,7 @@ default ConfiguredTarget unwrapIfMerged() {
*
* @return a map of provider names to their values, or null if there are no providers
*/
@Override
@Nullable
default Dict<String, Object> getProvidersDictForQuery() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.profiler.AutoProfiler;
import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils;
import com.google.devtools.build.lib.query2.common.CqueryNode;
import java.time.Duration;
import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -243,7 +244,7 @@ static ArtifactsToBuild getAllArtifactsToBuild(
*
* <p>Always returns false for hidden rules and source file targets.
*/
public static boolean shouldConsiderForDisplay(ConfiguredTarget configuredTarget) {
public static boolean shouldConsiderForDisplay(CqueryNode configuredTarget) {
// TODO(bazel-team): this is quite ugly. Add a marker provider for this check.
if (configuredTarget instanceof InputFileConfiguredTarget) {
// Suppress display of source files (because we do no work to build them).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
package com.google.devtools.build.lib.buildtool;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment.TopLevelConfigurations;
import com.google.devtools.build.lib.query2.common.CqueryNode;
import com.google.devtools.build.lib.query2.cquery.ConfiguredTargetQueryEnvironment;
import com.google.devtools.build.lib.query2.cquery.CqueryOptions;
import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction;
Expand All @@ -29,7 +29,7 @@
import net.starlark.java.eval.StarlarkSemantics;

/** Performs {@code cquery} processing. */
public final class CqueryProcessor extends PostAnalysisQueryProcessor<ConfiguredTarget> {
public final class CqueryProcessor extends PostAnalysisQueryProcessor<CqueryNode> {

public CqueryProcessor(
QueryExpression queryExpression, TargetPattern.Parser mainRepoTargetParser) {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/query2/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ java_library(
),
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
Expand Down Expand Up @@ -74,6 +75,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/query2/common:QueryTransitivePackagePreloader",
"//src/main/java/com/google/devtools/build/lib/query2/common:UniverseSkyKey",
"//src/main/java/com/google/devtools/build/lib/query2/common:abstract-blaze-query-env",
"//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
"//src/main/java/com/google/devtools/build/lib/query2/common:options",
"//src/main/java/com/google/devtools/build/lib/query2/common:universe-scope",
"//src/main/java/com/google/devtools/build/lib/query2/compat:fake-load-target",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.analysis.AliasProvider;
import com.google.devtools.build.lib.analysis.AspectValue;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
Expand Down Expand Up @@ -74,6 +76,7 @@
import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.WalkableGraph;
import java.io.IOException;
import java.io.OutputStream;
Expand Down Expand Up @@ -144,7 +147,7 @@ public PostAnalysisQueryEnvironment(

public abstract String getOutputFormat();

protected abstract KeyExtractor<T, ConfiguredTargetKey> getConfiguredTargetKeyExtractor();
protected abstract KeyExtractor<T, ActionLookupKey> getConfiguredTargetKeyExtractor();

@Override
public QueryEvalResult evaluateQuery(
Expand Down Expand Up @@ -217,12 +220,18 @@ public T getOrCreate(T target) {
protected abstract T getNullConfiguredTarget(Label label) throws InterruptedException;

@Nullable
public ConfiguredTargetValue getConfiguredTargetValue(SkyKey key) throws InterruptedException {
return (ConfiguredTargetValue) walkableGraphSupplier.get().getValue(key);
public SkyValue getConfiguredTargetValue(SkyKey key) throws InterruptedException {
return walkableGraphSupplier.get().getValue(key);
}

@Nullable
public AspectValue getAspectValue(SkyKey key) throws InterruptedException {
return (AspectValue) walkableGraphSupplier.get().getValue(key);
}

private boolean isAliasConfiguredTarget(ConfiguredTargetKey key) throws InterruptedException {
return AliasProvider.isAlias(getConfiguredTargetValue(key).getConfiguredTarget());
return AliasProvider.isAlias(
((ConfiguredTargetValue) getConfiguredTargetValue(key)).getConfiguredTarget());
}

public InterruptibleSupplier<ImmutableSet<PathFragment>>
Expand Down Expand Up @@ -362,7 +371,7 @@ private Set<SkyKey> unwindReverseDependencyDelegationLayersIfFound(
if (!rdep.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
continue;
}
ConfiguredTargetKey actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
var actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
if (actualParentKey.equals(child)) {
// The parent has the same value as the child because it is delegating.
foundDelegatingRdep = true;
Expand All @@ -386,7 +395,7 @@ private void unwindReverseDependencyDelegationLayers(
output.add(rdep);
continue;
}
ConfiguredTargetKey actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
var actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
if (!actualParentKey.equals(child)) {
output.add(rdep);
continue;
Expand Down Expand Up @@ -479,19 +488,24 @@ private ImmutableList<ClassifiedDependency<T>> targetifyValues(
}
}

boolean explicitAspects =
settings.containsAll(ImmutableSet.of(Setting.INCLUDE_ASPECTS, Setting.EXPLICIT_ASPECTS));

ImmutableList.Builder<ClassifiedDependency<T>> values = ImmutableList.builder();
// TODO(bazel-team): An even better approach would be to treat aspects and toolchains as
// TODO(bazel-team): The end-goal approach is to treat aspects and toolchains as
// first-class query nodes just like targets. In other words, let query expressions reference
// them (they also have identifying labels) and make the graph connections between targets,
// aspects, and toolchains explicit. That would permit more detailed queries and eliminate the
// per-key-type special casing below. The challenge is to generalize all query code that
// currently assumes its inputs are configured targets. Toolchains may have additional caveats:
// see b/148550864.
// per-key-type special casing below.
// This is being experimentally implemented in phases. Currently support for aspects has been
// implemented behind the --experimental_explicit_aspects flag.
// See https://github.com/bazelbuild/bazel/issues/16310 for details.
for (SkyKey key : dependencies) {
if (knownCtDeps.contains(key)) {
continue;
}
if (key.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
if (key.functionName().equals(SkyFunctions.CONFIGURED_TARGET)
|| (explicitAspects && key.functionName().equals(SkyFunctions.ASPECT))) {
T dependency = getValueFromKey(key);
Preconditions.checkState(
dependency != null,
Expand All @@ -501,17 +515,24 @@ private ImmutableList<ClassifiedDependency<T>> targetifyValues(
+ " configurability team.",
key);

boolean implicit =
boolean implicitConfiguredTarget =
// Check both the original guess key and the second correct key. In the case of the
// target platform, Util.findImplicitDeps also uses the original guess key.
implicitDeps == null
|| implicitDeps.contains(key)
|| implicitDeps.contains(getConfiguredTargetKey(dependency));

boolean implicit =
!(key.argument() instanceof ConfiguredTargetKey) || implicitConfiguredTarget;

values.add(new ClassifiedDependency<>(dependency, implicit));
knownCtDeps.add(key);
} else if (settings.contains(Setting.INCLUDE_ASPECTS)
&& key.functionName().equals(SkyFunctions.ASPECT)
&& !resolvedAspectClasses.contains(((AspectKey) key).getAspectClass())) {
&& key.functionName().equals(SkyFunctions.ASPECT)) {
Preconditions.checkState(!settings.contains(Setting.EXPLICIT_ASPECTS));
if (resolvedAspectClasses.contains(((AspectKey) key).getAspectClass())) {
continue;
}
// When an aspect is attached to an alias configured target, it bypasses standard dependency
// resolution and just Skyframe-loads the same aspect for the alias' referent. That means
// the original aspect's attribute deps aren't Skyframe-resolved through AspectFunction's
Expand Down Expand Up @@ -550,8 +571,8 @@ private Map<SkyKey, ImmutableList<ClassifiedDependency<T>>> targetifyValues(
targetifyValues(
fromTargetsByKey.get(fromKey),
entry.getValue(),
/*knownCtDeps=*/ new HashSet<>(),
/*resolvedAspectClasses=*/ new HashSet<>()));
/* knownCtDeps= */ new HashSet<>(),
/* resolvedAspectClasses= */ new HashSet<>()));
}
return result;
}
Expand Down Expand Up @@ -586,7 +607,7 @@ private static <T> ImmutableList<T> getDependencies(
@Nullable
protected abstract BuildConfigurationValue getConfiguration(T target);

protected abstract ConfiguredTargetKey getConfiguredTargetKey(T target);
protected abstract ActionLookupKey getConfiguredTargetKey(T target);

@Override
public ThreadSafeMutableSet<T> getTransitiveClosure(
Expand Down Expand Up @@ -649,11 +670,13 @@ public static class TopLevelConfigurations {

/** A map of non-null configured top-level targets sorted by configuration checksum. */
private final ImmutableMap<Label, BuildConfigurationValue> nonNulls;

/**
* {@code nonNulls} may often have many duplicate values in its value set so we store a sorted
* set of all the non-null configurations here.
*/
private final ImmutableSortedSet<BuildConfigurationValue> nonNullConfigs;

/** A list of null configured top-level targets. */
private final ImmutableList<Label> nulls;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.util.concurrent.AsyncFunction;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
Expand Down Expand Up @@ -70,8 +71,7 @@ public class ActionGraphQueryEnvironment
private AqueryOptions aqueryOptions;

private AqueryActionFilter actionFilters;
private final KeyExtractor<ConfiguredTargetValue, ConfiguredTargetKey>
configuredTargetKeyExtractor;
private final KeyExtractor<ConfiguredTargetValue, ActionLookupKey> configuredTargetKeyExtractor;
private final ConfiguredTargetValueAccessor accessor;

public ActionGraphQueryEnvironment(
Expand Down Expand Up @@ -187,8 +187,7 @@ public String getOutputFormat() {
}

@Override
protected KeyExtractor<ConfiguredTargetValue, ConfiguredTargetKey>
getConfiguredTargetKeyExtractor() {
protected KeyExtractor<ConfiguredTargetValue, ActionLookupKey> getConfiguredTargetKeyExtractor() {
return configuredTargetKeyExtractor;
}

Expand All @@ -202,7 +201,7 @@ public Label getCorrectLabel(ConfiguredTargetValue configuredTargetValue) {
@Nullable
private ConfiguredTargetValue createConfiguredTargetValueFromKey(ConfiguredTargetKey key)
throws InterruptedException {
ConfiguredTargetValue value = getConfiguredTargetValue(key);
ConfiguredTargetValue value = (ConfiguredTargetValue) getConfiguredTargetValue(key);
if (value == null
|| !Objects.equals(
value.getConfiguredTarget().getConfigurationKey(), key.getConfigurationKey())) {
Expand Down Expand Up @@ -251,7 +250,7 @@ protected ConfiguredTargetValue getNullConfiguredTarget(Label label) throws Inte
@Override
protected ConfiguredTargetValue getValueFromKey(SkyKey key) throws InterruptedException {
Preconditions.checkState(key instanceof ConfiguredTargetKey);
return getConfiguredTargetValue(key);
return (ConfiguredTargetValue) getConfiguredTargetValue(key);
}

@Nullable
Expand Down
Loading

0 comments on commit a305a0d

Please sign in to comment.