[7.2.0] aquery: interpret "//foo:bar" as "all configured targets with…

… label //foo:bar" (#22135) 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 Commit 0662df0 Co-authored-by: Googler <[email protected]>
bazelbuild · Apr 25, 2024 · ddc37e5 · ddc37e5
1 parent 69186f8
commit ddc37e5
Show file tree

Hide file tree

Showing 13 changed files with 139 additions and 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
@@ -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<ConfiguredTargetValue> getQueryEnvironmen
       BuildRequest request,
       CommandEnvironment env,
       TopLevelConfigurations topLevelConfigurations,
-      Collection<SkyKey> transitiveConfigurationKeys,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       WalkableGraph walkableGraph) {
     ImmutableList<QueryFunction> extraFunctions =
         new ImmutableList.Builder<QueryFunction>()
@@ -157,6 +157,7 @@ protected PostAnalysisQueryEnvironment<ConfiguredTargetValue> 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
@@ -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<SkyKey> transitiveConfigurationKeys,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       WalkableGraph walkableGraph)
       throws InterruptedException {
     ImmutableList<QueryFunction> 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
@@ -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<T> getQueryEnvironment(
       BuildRequest request,
       CommandEnvironment env,
       TopLevelConfigurations topLevelConfigurations,
-      Collection<SkyKey> transitiveConfigurationKeys,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       WalkableGraph walkableGraph)
       throws InterruptedException;
 
+  private static ImmutableMap<String, BuildConfigurationValue> getTransitiveConfigurations(
+      Collection<SkyKey> 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<String, BuildConfigurationValue> transitiveConfigurations =
+        getTransitiveConfigurations(transitiveConfigurationKeys, walkableGraph);
 
     PostAnalysisQueryEnvironment<T> postAnalysisQueryEnvironment =
         getQueryEnvironment(
-            request,
-            env,
-            topLevelConfigurations,
-            transitiveConfigurationKeys,
-            walkableGraph);
+            request, env, topLevelConfigurations, transitiveConfigurations, walkableGraph);
 
     Iterable<NamedThreadSafeOutputFormatterCallback<T>> 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
@@ -115,20 +115,39 @@ public abstract class PostAnalysisQueryEnvironment<T> extends AbstractBlazeQuery
   private final Supplier<WalkableGraph> 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.
+   *
+   * <p>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 <i>some</i>
+   * configuration, then querying the {@link WalkableGraph} to find the matching configured target.
+   *
+   * <p>Having this map lets cquery choose from all available configurations in the graph,
+   * particularly including configurations that aren't the top-level.
+   *
+   * <p>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<String, BuildConfigurationValue> transitiveConfigurations;
+
   protected RecursivePackageProviderBackedTargetPatternResolver resolver;
 
   public PostAnalysisQueryEnvironment(
       boolean keepGoing,
       ExtendedEventHandler eventHandler,
       Iterable<QueryFunction> extraFunctions,
       TopLevelConfigurations topLevelConfigurations,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       TargetPattern.Parser mainRepoTargetParser,
       PathPackageLocator pkgPath,
       Supplier<WalkableGraph> walkableGraphSupplier,
       Set<Setting> settings,
       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
@@ -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<QueryFunction> extraFunctions,
       TopLevelConfigurations topLevelConfigurations,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       TargetPattern.Parser mainRepoTargetParser,
       PathPackageLocator pkgPath,
       Supplier<WalkableGraph> walkableGraphSupplier,
@@ -89,6 +91,7 @@ public ActionGraphQueryEnvironment(
         eventHandler,
         extraFunctions,
         topLevelConfigurations,
+        transitiveConfigurations,
         mainRepoTargetParser,
         pkgPath,
         walkableGraphSupplier,
@@ -105,6 +108,7 @@ public ActionGraphQueryEnvironment(
       ExtendedEventHandler eventHandler,
       Iterable<QueryFunction> extraFunctions,
       TopLevelConfigurations topLevelConfigurations,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       TargetPattern.Parser mainRepoTargetParser,
       PathPackageLocator pkgPath,
       Supplier<WalkableGraph> walkableGraphSupplier,
@@ -115,6 +119,7 @@ public ActionGraphQueryEnvironment(
         eventHandler,
         extraFunctions,
         topLevelConfigurations,
+        transitiveConfigurations,
         mainRepoTargetParser,
         pkgPath,
         walkableGraphSupplier,
@@ -313,11 +318,7 @@ public QueryTaskFuture<Void> getTargetsMatchingPattern(
                     partialResult -> {
                       List<ConfiguredTargetValue> 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<Void> 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.
+   *
+   * <p>If there are no matches, returns an empty list.
+   */
+  private ImmutableList<ConfiguredTargetValue> getConfiguredTargetsForLabel(Label label)
+      throws InterruptedException {
+    ImmutableList.Builder<ConfiguredTargetValue> 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/...in/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java b/...in/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.
-   *
-   * <p>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 <i>some</i>
-   * configuration, then querying the {@link WalkableGraph} to find the matching configured target.
-   *
-   * <p>Having this map lets cquery choose from all available configurations in the graph,
-   * particularly including configurations that aren't the top-level.
-   *
-   * <p>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<String, BuildConfigurationValue> transitiveConfigurations;
-
   @Override
   protected KeyExtractor<CqueryNode, ActionLookupKey> getConfiguredTargetKeyExtractor() {
     return configuredTargetKeyExtractor;
@@ -121,7 +99,7 @@ public ConfiguredTargetQueryEnvironment(
       ExtendedEventHandler eventHandler,
       Iterable<QueryFunction> extraFunctions,
       TopLevelConfigurations topLevelConfigurations,
-      Collection<SkyKey> transitiveConfigurationKeys,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       TargetPattern.Parser mainRepoTargetParser,
       PathPackageLocator pkgPath,
       Supplier<WalkableGraph> walkableGraphSupplier,
@@ -134,15 +112,14 @@ public ConfiguredTargetQueryEnvironment(
         eventHandler,
         extraFunctions,
         topLevelConfigurations,
+        transitiveConfigurations,
         mainRepoTargetParser,
         pkgPath,
         walkableGraphSupplier,
         settings,
         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<QueryFunction> extraFunctions,
       TopLevelConfigurations topLevelConfigurations,
-      Collection<SkyKey> transitiveConfigurationKeys,
+      ImmutableMap<String, BuildConfigurationValue> transitiveConfigurations,
       TargetPattern.Parser mainRepoTargetParser,
       PathPackageLocator pkgPath,
       Supplier<WalkableGraph> walkableGraphSupplier,
@@ -164,7 +141,7 @@ public ConfiguredTargetQueryEnvironment(
         eventHandler,
         extraFunctions,
         topLevelConfigurations,
-        transitiveConfigurationKeys,
+        transitiveConfigurations,
         mainRepoTargetParser,
         pkgPath,
         walkableGraphSupplier,
@@ -185,17 +162,6 @@ private static ImmutableList<QueryFunction> getCqueryFunctions() {
     return ImmutableList.of(new ConfigFunction());
   }
 
-  private static ImmutableMap<String, BuildConfigurationValue> getTransitiveConfigurations(
-      Collection<SkyKey> 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<NamedThreadSafeOutputFormatterCallback<CqueryNode>>
       getDefaultOutputFormatters(
@@ -314,8 +280,7 @@ public QueryTaskFuture<Void> getTargetsMatchingPattern(
                     partialResult -> {
                       List<CqueryNode> 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 {
    *
    * <p>If there are no matches, returns an empty list.
    */
-  private ImmutableList<CqueryNode> getConfiguredTargetsForConfigFunction(Label label)
+  private ImmutableList<CqueryNode> getConfiguredTargetsForLabel(Label label)
       throws InterruptedException {
     ImmutableList.Builder<CqueryNode> ans = ImmutableList.builder();
     for (BuildConfigurationValue config : transitiveConfigurations.values()) {