From 687b2371173ee320ea14305f735ef7ad795a7811 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 28 Mar 2024 16:31:29 -0700 Subject: [PATCH] aquery: interpret "//foo:bar" as "all configured targets with label //foo:bar" cquery acquired this behavior 4 years ago in unknown commit, but it was never ported to aquery. This fixes some potentially surprising behavior when multiple configured targets corresponding to the same label are present (see added test cases). RELNOTES: aquery: `//foo:bar` now means "all configured targets with label `//foo:bar`" instead of "choose an arbitrary configured target with label `//foo:bar`". This is in line with cquery behavior. PiperOrigin-RevId: 620091100 Change-Id: Ib5c5ee33e35fe7ac30bc31f703b119dec40185b7 --- .../build/lib/buildtool/AqueryProcessor.java | 7 +-- .../build/lib/buildtool/CqueryProcessor.java | 8 ++-- .../buildtool/PostAnalysisQueryProcessor.java | 27 ++++++++--- .../query2/PostAnalysisQueryEnvironment.java | 19 ++++++++ .../aquery/ActionGraphQueryEnvironment.java | 38 ++++++++++----- .../ConfiguredTargetQueryEnvironment.java | 47 +++---------------- .../query2/aquery/ActionGraphQueryHelper.java | 7 +-- .../devtools/build/lib/query2/aquery/BUILD | 2 +- .../devtools/build/lib/query2/cquery/BUILD | 2 +- .../cquery/ConfiguredTargetQueryHelper.java | 8 ++-- .../cquery/ConfiguredTargetQueryTest.java | 6 --- .../testutil/PostAnalysisQueryHelper.java | 27 +++++++++-- .../testutil/PostAnalysisQueryTest.java | 29 ++++++++++-- 13 files changed, 139 insertions(+), 88 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java index 303d791a2ed1b7..c263d8629dd8cf 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java @@ -14,9 +14,11 @@ package com.google.devtools.build.lib.buildtool; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.analysis.actions.TemplateExpansionException; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -45,11 +47,9 @@ import com.google.devtools.build.lib.skyframe.actiongraph.v2.AqueryOutputHandler; import com.google.devtools.build.lib.skyframe.actiongraph.v2.AqueryOutputHandler.OutputType; import com.google.devtools.build.lib.skyframe.actiongraph.v2.InvalidAqueryOutputFormatException; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.WalkableGraph; import java.io.IOException; import java.io.PrintStream; -import java.util.Collection; import java.util.Optional; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; @@ -139,7 +139,7 @@ protected PostAnalysisQueryEnvironment getQueryEnvironmen BuildRequest request, CommandEnvironment env, TopLevelConfigurations topLevelConfigurations, - Collection transitiveConfigurationKeys, + ImmutableMap transitiveConfigurations, WalkableGraph walkableGraph) { ImmutableList extraFunctions = new ImmutableList.Builder() @@ -157,6 +157,7 @@ protected PostAnalysisQueryEnvironment getQueryEnvironmen env.getReporter(), extraFunctions, topLevelConfigurations, + transitiveConfigurations, mainRepoTargetParser, env.getPackageManager().getPackagePath(), () -> walkableGraph, diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/CqueryProcessor.java b/src/main/java/com/google/devtools/build/lib/buildtool/CqueryProcessor.java index fa0ab52e861629..2ad4bc43e41242 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/CqueryProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/CqueryProcessor.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.buildtool; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; 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; @@ -23,9 +25,7 @@ import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction; import com.google.devtools.build.lib.query2.engine.QueryExpression; import com.google.devtools.build.lib.runtime.CommandEnvironment; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.WalkableGraph; -import java.util.Collection; import net.starlark.java.eval.StarlarkSemantics; /** Performs {@code cquery} processing. */ @@ -41,7 +41,7 @@ protected ConfiguredTargetQueryEnvironment getQueryEnvironment( BuildRequest request, CommandEnvironment env, TopLevelConfigurations configurations, - Collection transitiveConfigurationKeys, + ImmutableMap transitiveConfigurations, WalkableGraph walkableGraph) throws InterruptedException { ImmutableList extraFunctions = @@ -58,7 +58,7 @@ protected ConfiguredTargetQueryEnvironment getQueryEnvironment( env.getReporter(), extraFunctions, configurations, - transitiveConfigurationKeys, + transitiveConfigurations, mainRepoTargetParser, env.getPackageManager().getPackagePath(), () -> walkableGraph, diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryProcessor.java b/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryProcessor.java index 2d3dff5ff9cb66..ec896274df845d 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryProcessor.java @@ -13,8 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.buildtool; +import static com.google.common.collect.ImmutableMap.toImmutableMap; + +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.AnalysisResult; import com.google.devtools.build.lib.analysis.ViewCreationFailedException; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.buildtool.BuildTool.ExitException; import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.events.Event; @@ -42,7 +46,9 @@ import com.google.devtools.common.options.OptionsParsingException; import java.io.IOException; import java.util.Collection; +import java.util.Comparator; import java.util.Set; +import java.util.function.Function; /** * Version of {@link BuildTool} that handles all work for queries based on results from the analysis @@ -129,10 +135,21 @@ protected abstract PostAnalysisQueryEnvironment getQueryEnvironment( BuildRequest request, CommandEnvironment env, TopLevelConfigurations topLevelConfigurations, - Collection transitiveConfigurationKeys, + ImmutableMap transitiveConfigurations, WalkableGraph walkableGraph) throws InterruptedException; + private static ImmutableMap getTransitiveConfigurations( + Collection transitiveConfigurationKeys, WalkableGraph graph) + throws InterruptedException { + // BuildConfigurationKey and BuildConfigurationValue should be 1:1 + // so merge function intentionally omitted + return graph.getSuccessfulValues(transitiveConfigurationKeys).values().stream() + .map(BuildConfigurationValue.class::cast) + .sorted(Comparator.comparing(BuildConfigurationValue::checksum)) + .collect(toImmutableMap(BuildConfigurationValue::checksum, Function.identity())); + } + private void doPostAnalysisQuery( BuildRequest request, CommandEnvironment env, @@ -145,14 +162,12 @@ private void doPostAnalysisQuery( OptionsParsingException { WalkableGraph walkableGraph = SkyframeExecutorWrappingWalkableGraph.of(env.getSkyframeExecutor()); + ImmutableMap transitiveConfigurations = + getTransitiveConfigurations(transitiveConfigurationKeys, walkableGraph); PostAnalysisQueryEnvironment postAnalysisQueryEnvironment = getQueryEnvironment( - request, - env, - topLevelConfigurations, - transitiveConfigurationKeys, - walkableGraph); + request, env, topLevelConfigurations, transitiveConfigurations, walkableGraph); Iterable> callbacks = postAnalysisQueryEnvironment.getDefaultOutputFormatters( diff --git a/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java index 0b9c1af92a7717..a9815fdd5d96ab 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java @@ -115,6 +115,23 @@ public abstract class PostAnalysisQueryEnvironment extends AbstractBlazeQuery private final Supplier walkableGraphSupplier; protected WalkableGraph graph; + /** + * Stores every configuration in the transitive closure of the build graph as a map from its + * user-friendly hash to the configuration itself. + * + *

This is used to find configured targets in, e.g. {@code somepath} queries. Given {@code + * somepath(//foo, //bar)}, cquery finds the configured targets for {@code //foo} and {@code + * //bar} by creating a {@link ConfiguredTargetKey} from their labels and some + * configuration, then querying the {@link WalkableGraph} to find the matching configured target. + * + *

Having this map lets cquery choose from all available configurations in the graph, + * particularly including configurations that aren't the top-level. + * + *

This can also be used in cquery's {@code config} function to match against explicitly + * specified configs. This, in particular, is where having user-friendly hashes is invaluable. + */ + protected final ImmutableMap transitiveConfigurations; + protected RecursivePackageProviderBackedTargetPatternResolver resolver; public PostAnalysisQueryEnvironment( @@ -122,6 +139,7 @@ public PostAnalysisQueryEnvironment( ExtendedEventHandler eventHandler, Iterable extraFunctions, TopLevelConfigurations topLevelConfigurations, + ImmutableMap transitiveConfigurations, TargetPattern.Parser mainRepoTargetParser, PathPackageLocator pkgPath, Supplier walkableGraphSupplier, @@ -129,6 +147,7 @@ public PostAnalysisQueryEnvironment( LabelPrinter labelPrinter) { super(keepGoing, true, Rule.ALL_LABELS, eventHandler, settings, extraFunctions, labelPrinter); this.topLevelConfigurations = topLevelConfigurations; + this.transitiveConfigurations = transitiveConfigurations; this.mainRepoTargetParser = mainRepoTargetParser; this.pkgPath = pkgPath; this.walkableGraphSupplier = walkableGraphSupplier; diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java index 4e8bcc0836c691..72e183cf6a9fa2 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java @@ -15,6 +15,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.common.util.concurrent.AsyncFunction; import com.google.common.util.concurrent.Futures; @@ -79,6 +80,7 @@ public ActionGraphQueryEnvironment( ExtendedEventHandler eventHandler, Iterable extraFunctions, TopLevelConfigurations topLevelConfigurations, + ImmutableMap transitiveConfigurations, TargetPattern.Parser mainRepoTargetParser, PathPackageLocator pkgPath, Supplier walkableGraphSupplier, @@ -89,6 +91,7 @@ public ActionGraphQueryEnvironment( eventHandler, extraFunctions, topLevelConfigurations, + transitiveConfigurations, mainRepoTargetParser, pkgPath, walkableGraphSupplier, @@ -105,6 +108,7 @@ public ActionGraphQueryEnvironment( ExtendedEventHandler eventHandler, Iterable extraFunctions, TopLevelConfigurations topLevelConfigurations, + ImmutableMap transitiveConfigurations, TargetPattern.Parser mainRepoTargetParser, PathPackageLocator pkgPath, Supplier walkableGraphSupplier, @@ -115,6 +119,7 @@ public ActionGraphQueryEnvironment( eventHandler, extraFunctions, topLevelConfigurations, + transitiveConfigurations, mainRepoTargetParser, pkgPath, walkableGraphSupplier, @@ -313,11 +318,7 @@ public QueryTaskFuture getTargetsMatchingPattern( partialResult -> { List transformedResult = new ArrayList<>(); for (Target target : partialResult) { - ConfiguredTargetValue configuredTargetValue = - getConfiguredTargetValue(target.getLabel()); - if (configuredTargetValue != null) { - transformedResult.add(configuredTargetValue); - } + transformedResult.addAll(getConfiguredTargetsForLabel(target.getLabel())); } callback.process(transformedResult); }, @@ -327,14 +328,27 @@ public QueryTaskFuture getTargetsMatchingPattern( MoreExecutors.directExecutor())); } - private ConfiguredTargetValue getConfiguredTargetValue(Label label) throws InterruptedException { - // Try with target configuration. - ConfiguredTargetValue configuredTargetValue = getTargetConfiguredTarget(label); - if (configuredTargetValue != null) { - return configuredTargetValue; + /** + * Returns all configured targets in Skyframe with the given label. + * + *

If there are no matches, returns an empty list. + */ + private ImmutableList getConfiguredTargetsForLabel(Label label) + throws InterruptedException { + ImmutableList.Builder ans = ImmutableList.builder(); + for (BuildConfigurationValue config : transitiveConfigurations.values()) { + ConfiguredTargetValue configuredTargetValue = + createConfiguredTargetValueFromKey( + ConfiguredTargetKey.builder().setLabel(label).setConfiguration(config).build()); + if (configuredTargetValue != null) { + ans.add(configuredTargetValue); + } + } + ConfiguredTargetValue nullConfiguredTarget = getNullConfiguredTarget(label); + if (nullConfiguredTarget != null) { + ans.add(nullConfiguredTarget); } - // Last chance: source file. - return getNullConfiguredTarget(label); + return ans.build(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java index ad410755b752b4..8a4f21d64d87e7 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java @@ -13,8 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.query2.cquery; -import static com.google.common.collect.ImmutableMap.toImmutableMap; - import com.google.common.base.Joiner; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; @@ -62,12 +60,9 @@ import com.google.devtools.build.skyframe.WalkableGraph; import java.io.OutputStream; import java.util.ArrayList; -import java.util.Collection; -import java.util.Comparator; import java.util.List; import java.util.Objects; import java.util.Set; -import java.util.function.Function; import java.util.function.Supplier; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -94,23 +89,6 @@ public class ConfiguredTargetQueryEnvironment extends PostAnalysisQueryEnvironme private final ConfiguredTargetAccessor accessor; - /** - * F Stores every configuration in the transitive closure of the build graph as a map from its - * user-friendly hash to the configuration itself. - * - *

This is used to find configured targets in, e.g. {@code somepath} queries. Given {@code - * somepath(//foo, //bar)}, cquery finds the configured targets for {@code //foo} and {@code - * //bar} by creating a {@link ConfiguredTargetKey} from their labels and some - * configuration, then querying the {@link WalkableGraph} to find the matching configured target. - * - *

Having this map lets cquery choose from all available configurations in the graph, - * particularly including configurations that aren't the top-level. - * - *

This can also be used in cquery's {@code config} function to match against explicitly - * specified configs. This, in particular, is where having user-friendly hashes is invaluable. - */ - private final ImmutableMap transitiveConfigurations; - @Override protected KeyExtractor getConfiguredTargetKeyExtractor() { return configuredTargetKeyExtractor; @@ -121,7 +99,7 @@ public ConfiguredTargetQueryEnvironment( ExtendedEventHandler eventHandler, Iterable extraFunctions, TopLevelConfigurations topLevelConfigurations, - Collection transitiveConfigurationKeys, + ImmutableMap transitiveConfigurations, TargetPattern.Parser mainRepoTargetParser, PathPackageLocator pkgPath, Supplier walkableGraphSupplier, @@ -134,6 +112,7 @@ public ConfiguredTargetQueryEnvironment( eventHandler, extraFunctions, topLevelConfigurations, + transitiveConfigurations, mainRepoTargetParser, pkgPath, walkableGraphSupplier, @@ -141,8 +120,6 @@ public ConfiguredTargetQueryEnvironment( labelPrinter); this.accessor = new ConfiguredTargetAccessor(walkableGraphSupplier.get(), this); this.configuredTargetKeyExtractor = CqueryNode::getLookupKey; - this.transitiveConfigurations = - getTransitiveConfigurations(transitiveConfigurationKeys, walkableGraphSupplier.get()); this.topLevelArtifactContext = topLevelArtifactContext; } @@ -151,7 +128,7 @@ public ConfiguredTargetQueryEnvironment( ExtendedEventHandler eventHandler, Iterable extraFunctions, TopLevelConfigurations topLevelConfigurations, - Collection transitiveConfigurationKeys, + ImmutableMap transitiveConfigurations, TargetPattern.Parser mainRepoTargetParser, PathPackageLocator pkgPath, Supplier walkableGraphSupplier, @@ -164,7 +141,7 @@ public ConfiguredTargetQueryEnvironment( eventHandler, extraFunctions, topLevelConfigurations, - transitiveConfigurationKeys, + transitiveConfigurations, mainRepoTargetParser, pkgPath, walkableGraphSupplier, @@ -185,17 +162,6 @@ private static ImmutableList getCqueryFunctions() { return ImmutableList.of(new ConfigFunction()); } - private static ImmutableMap getTransitiveConfigurations( - Collection transitiveConfigurationKeys, WalkableGraph graph) - throws InterruptedException { - // BuildConfigurationKey and BuildConfigurationValue should be 1:1 - // so merge function intentionally omitted - return graph.getSuccessfulValues(transitiveConfigurationKeys).values().stream() - .map(BuildConfigurationValue.class::cast) - .sorted(Comparator.comparing(BuildConfigurationValue::checksum)) - .collect(toImmutableMap(BuildConfigurationValue::checksum, Function.identity())); - } - @Override public ImmutableList> getDefaultOutputFormatters( @@ -314,8 +280,7 @@ public QueryTaskFuture getTargetsMatchingPattern( partialResult -> { List transformedResult = new ArrayList<>(); for (Target target : partialResult) { - transformedResult.addAll( - getConfiguredTargetsForConfigFunction(target.getLabel())); + transformedResult.addAll(getConfiguredTargetsForLabel(target.getLabel())); } callback.process(transformedResult); }, @@ -372,7 +337,7 @@ protected CqueryNode getValueFromKey(SkyKey key) throws InterruptedException { * *

If there are no matches, returns an empty list. */ - private ImmutableList getConfiguredTargetsForConfigFunction(Label label) + private ImmutableList getConfiguredTargetsForLabel(Label label) throws InterruptedException { ImmutableList.Builder ans = ImmutableList.builder(); for (BuildConfigurationValue config : transitiveConfigurations.values()) { diff --git a/src/test/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryHelper.java index 28bf640fa15e44..0811f918a7a6a9 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryHelper.java +++ b/src/test/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryHelper.java @@ -14,15 +14,15 @@ package com.google.devtools.build.lib.query2.aquery; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.packages.LabelPrinter; import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment; import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment.TopLevelConfigurations; import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction; import com.google.devtools.build.lib.query2.testutil.PostAnalysisQueryHelper; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.WalkableGraph; -import java.util.Collection; /** Helper class for aquery test */ public class ActionGraphQueryHelper extends PostAnalysisQueryHelper { @@ -31,7 +31,7 @@ public class ActionGraphQueryHelper extends PostAnalysisQueryHelper getPostAnalysisQueryEnvironment( WalkableGraph walkableGraph, TopLevelConfigurations topLevelConfigurations, - Collection transitiveConfigurationKeys) { + ImmutableMap transitiveConfigurations) { ImmutableList extraFunctions = ImmutableList.copyOf(ActionGraphQueryEnvironment.AQUERY_FUNCTIONS); return new ActionGraphQueryEnvironment( @@ -39,6 +39,7 @@ protected PostAnalysisQueryEnvironment getPostAnalysisQue getReporter(), extraFunctions, topLevelConfigurations, + transitiveConfigurations, mainRepoTargetParser, analysisHelper.getPackageManager().getPackagePath(), () -> walkableGraph, diff --git a/src/test/java/com/google/devtools/build/lib/query2/aquery/BUILD b/src/test/java/com/google/devtools/build/lib/query2/aquery/BUILD index caae09fd7f62f1..b1eb0232541026 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/aquery/BUILD +++ b/src/test/java/com/google/devtools/build/lib/query2/aquery/BUILD @@ -16,12 +16,12 @@ java_library( testonly = 1, srcs = ["ActionGraphQueryHelper.java"], deps = [ + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value", "//src/main/java/com/google/devtools/build/lib/packages:label_printer", "//src/main/java/com/google/devtools/build/lib/query2", "//src/main/java/com/google/devtools/build/lib/query2/engine", "//src/main/java/com/google/devtools/build/skyframe", - "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/test/java/com/google/devtools/build/lib/query2/testutil", "//third_party:guava", ], diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD b/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD index d3bd09d275d5a0..8fd9a3f0592629 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD +++ b/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD @@ -89,12 +89,12 @@ java_library( testonly = 1, srcs = ["ConfiguredTargetQueryHelper.java"], deps = [ + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", "//src/main/java/com/google/devtools/build/lib/packages:label_printer", "//src/main/java/com/google/devtools/build/lib/query2", "//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/skyframe", - "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/query2/testutil", "//third_party:guava", diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryHelper.java index 9fe9d68a291531..862ea21c575d57 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryHelper.java +++ b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryHelper.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.query2.cquery; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; import com.google.devtools.build.lib.packages.LabelPrinter; import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment.TopLevelConfigurations; @@ -21,9 +23,7 @@ import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction; import com.google.devtools.build.lib.query2.testutil.AbstractQueryTest.QueryHelper; import com.google.devtools.build.lib.query2.testutil.PostAnalysisQueryHelper; -import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.WalkableGraph; -import java.util.Collection; /** * {@link QueryHelper} for {@link ConfiguredTargetQueryTest}. Big warts: uses an {@link @@ -37,7 +37,7 @@ public class ConfiguredTargetQueryHelper extends PostAnalysisQueryHelper transitiveConfigurationKeys) + ImmutableMap transitiveConfigurations) throws InterruptedException { ImmutableList extraFunctions = ImmutableList.copyOf(ConfiguredTargetQueryEnvironment.CQUERY_FUNCTIONS); @@ -46,7 +46,7 @@ protected ConfiguredTargetQueryEnvironment getPostAnalysisQueryEnvironment( getReporter(), extraFunctions, topLevelConfigurations, - transitiveConfigurationKeys, + transitiveConfigurations, mainRepoTargetParser, analysisHelper.getPackageManager().getPackagePath(), () -> walkableGraph, diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java index ed8e1af4932b0f..75378c1104a752 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java @@ -107,10 +107,4 @@ public void testMultipleTopLevelConfigurations_nullConfigs() throws Exception { assertThat(getConfiguration(resultIterator.next())).isNull(); } } - - @Override - public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel() { - // When the same target exists in multiple configurations, cquery doesn't guarantee which - // instance is evaluated first. So disable this test. - } } diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryHelper.java index cb193539f54827..e2982224c6f2cb 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryHelper.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryHelper.java @@ -13,9 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.query2.testutil; +import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.devtools.build.lib.testutil.FoundationTestCase.failFastHandler; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.devtools.build.lib.analysis.AnalysisResult; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; @@ -56,8 +58,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Comparator; import java.util.List; import java.util.Set; +import java.util.function.Function; import org.junit.After; import org.junit.Before; @@ -204,11 +208,25 @@ public PostAnalysisQueryEnvironment getPostAnalysisQueryEnvironment( analysisResult = analysisHelper.update(universe.toArray(new String[0])); WalkableGraph walkableGraph = SkyframeExecutorWrappingWalkableGraph.of(analysisHelper.getSkyframeExecutor()); + ImmutableMap transitiveConfigurations = + getTransitiveConfigurations( + analysisHelper.getSkyframeExecutor().getTransitiveConfigurationKeys(), walkableGraph); return getPostAnalysisQueryEnvironment( walkableGraph, new TopLevelConfigurations(analysisResult.getTopLevelTargetsWithConfigs()), - analysisHelper.getSkyframeExecutor().getTransitiveConfigurationKeys()); + transitiveConfigurations); + } + + private static ImmutableMap getTransitiveConfigurations( + Collection transitiveConfigurationKeys, WalkableGraph graph) + throws InterruptedException { + // BuildConfigurationKey and BuildConfigurationValue should be 1:1 + // so merge function intentionally omitted + return graph.getSuccessfulValues(transitiveConfigurationKeys).values().stream() + .map(BuildConfigurationValue.class::cast) + .sorted(Comparator.comparing(BuildConfigurationValue::checksum)) + .collect(toImmutableMap(BuildConfigurationValue::checksum, Function.identity())); } /** @@ -218,13 +236,14 @@ public PostAnalysisQueryEnvironment getPostAnalysisQueryEnvironment( * search over * @param topLevelConfigurations the configurations used to build the top-level targets in a * query's universe scope - * @param transitiveConfigurationKeys all configurations available in the build graph (including - * those produced by configuration transitions in the top-level targets' transitive deps) + * @param transitiveConfigurations all configurations available in the build graph (including + * those produced by configuration transitions in the top-level targets' transitive deps), + * keyed by the configurations' checksums */ protected abstract PostAnalysisQueryEnvironment getPostAnalysisQueryEnvironment( WalkableGraph walkableGraph, TopLevelConfigurations topLevelConfigurations, - Collection transitiveConfigurationKeys) + ImmutableMap transitiveConfigurations) throws InterruptedException; @Override diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java index ab434840e0efda..1f0aed2e44e39c 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java @@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.BuildOptionsView; @@ -606,8 +605,8 @@ public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel() helper.setUniverseScope("//test:*"); - assertThat(getConfiguration(Iterables.getOnlyElement(eval("//test:dep")))) - .isNotEqualTo(getConfiguration(Iterables.getOnlyElement(eval("//test:top-level")))); + // `//test:dep` has two configurations. + assertThat(eval("//test:dep")).hasSize(2); } @Test @@ -632,6 +631,9 @@ public void inconsistentSkyQueryIncremental() throws Exception { .isEqualTo(FailureDetails.TargetPatterns.Code.CANNOT_DETERMINE_TARGET_FROM_FILENAME); } + @Test + public void labelPointsToMultipleConfiguredTargets() throws Exception {} + private void writeSimpleTarget() throws Exception { MockRule simpleRule = () -> @@ -642,6 +644,27 @@ private void writeSimpleTarget() throws Exception { writeFile("test/BUILD", "simple_rule(name = 'target')"); } + @Test + public void aliasMinus() throws Exception { + MockRule simpleRule = + () -> + MockRule.define( + "simple_rule", attr("dep", LABEL).allowedFileTypes(FileTypeSet.ANY_FILE)); + helper.useRuleClassProvider(setRuleClassProviders(simpleRule).build()); + + writeFile( + "p/BUILD", + "simple_rule(name = 'dep')", + "alias(name = 'alias', actual = 'dep')", + "simple_rule(name = 'user', dep = ':alias')"); + assertThat(evalToString("deps(//p:alias) - deps(//p:dep)")).isEqualTo("//p:alias"); + // The following assertion fails if the expression `//p:alias` doesn't represent two configured + // targets -- one configured without TestOptions (trimmed) and the other configured with one + // (untrimmed). The untrimmed configured target is from the top-level expression, whereas the + // trimmed one is from `//p:user`'s dependency. + assertThat(evalToString("deps(//p:user) - deps(//p:alias)")).isEqualTo("//p:user"); + } + @Test public void testVisibleFunctionDoesNotWork() throws Exception { writeSimpleTarget();