Loop in top-level transition logic to configurable query to give more accurate results. Some refactoring done to get custom query code out of BuildView and into resolvers.

PiperOrigin-RevId: 172647838

2 files changed, 110 insertions, 15 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index 02916941ef..56a11d2ce9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -29,16 +29,18 @@ import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
+import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction;
+import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.skyframe.TransitiveTargetValue;
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.ValueOrException;
-
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -46,6 +48,7 @@ import java.util.Comparator;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -513,4 +516,71 @@ public final class ConfigurationResolver {
     }
     return result;
   }
+
+  /**
+   * This method allows resolution of configurations outside of a skyfunction call.
+   *
+   * <p>If {@link BuildConfiguration.Options#trimConfigurations()} is true, transforms a collection
+   * of <Target, Configuration> pairs by trimming each target's configuration to only the fragments
+   * the target and its transitive dependencies need.
+   *
+   * <p>Else returns configurations that unconditionally include all fragments.
+   *
+   * <p>Preserves the original input order (but merges duplicate nodes that might occur due to
+   * top-level configuration transitions) . Uses original (untrimmed) configurations for targets
+   * that can't be evaluated (e.g. due to loading phase errors).
+   *
+   * <p>This is suitable for feeding {@link ConfiguredTargetValue} keys: as general principle {@link
+   * ConfiguredTarget}s should have exactly as much information in their configurations as they need
+   * to evaluate and no more (e.g. there's no need for Android settings in a C++ configured target).
+   *
+   * @param inputs the original targets and configurations
+   * @param asDeps the inputs repackaged as dependencies
+   * @param eventHandler
+   * @param skyframeExecutor
+   */
+  // TODO(bazel-team): error out early for targets that fail - untrimmed configurations should
+  // never make it through analysis (and especially not seed ConfiguredTargetValues)
+  public static LinkedHashSet<TargetAndConfiguration> getConfigurationsFromExecutor(
+      Iterable<TargetAndConfiguration> inputs,
+      Multimap<BuildConfiguration, Dependency> asDeps,
+      ExtendedEventHandler eventHandler,
+      SkyframeExecutor skyframeExecutor)
+      throws InterruptedException {
+
+    Map<Label, Target> labelsToTargets = new LinkedHashMap<>();
+    for (TargetAndConfiguration targetAndConfig : inputs) {
+      labelsToTargets.put(targetAndConfig.getLabel(), targetAndConfig.getTarget());
+    }
+
+    // Maps <target, originalConfig> pairs to <target, finalConfig> pairs for targets that
+    // could be successfully Skyframe-evaluated.
+    Map<TargetAndConfiguration, TargetAndConfiguration> successfullyEvaluatedTargets =
+        new LinkedHashMap<>();
+    if (!asDeps.isEmpty()) {
+      for (BuildConfiguration fromConfig : asDeps.keySet()) {
+        Multimap<Dependency, BuildConfiguration> trimmedTargets =
+            skyframeExecutor.getConfigurations(
+                eventHandler, fromConfig.getOptions(), asDeps.get(fromConfig));
+        for (Map.Entry<Dependency, BuildConfiguration> trimmedTarget : trimmedTargets.entries()) {
+          Target target = labelsToTargets.get(trimmedTarget.getKey().getLabel());
+          successfullyEvaluatedTargets.put(
+              new TargetAndConfiguration(target, fromConfig),
+              new TargetAndConfiguration(target, trimmedTarget.getValue()));
+        }
+      }
+    }
+
+    LinkedHashSet<TargetAndConfiguration> result = new LinkedHashSet<>();
+    for (TargetAndConfiguration originalInput : inputs) {
+      if (successfullyEvaluatedTargets.containsKey(originalInput)) {
+        // The configuration was successfully trimmed.
+        result.add(successfullyEvaluatedTargets.get(originalInput));
+      } else {
+        // Either the configuration couldn't be determined (e.g. loading phase error) or it's null.
+        result.add(originalInput);
+      }
+    }
+    return result;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
index 289f4453f6..77be709aa3 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis.config;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.SplitTransition;
@@ -28,16 +29,15 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.util.Preconditions;
 
 /**
- * Determines the {@link Attribute.Transition}s dependencies should apply to their parents'
- * configurations.
+ * Tool for evaluating which {@link Attribute.Transition}(s) should be applied to given targets.
  *
- * <p>For the work of turning these transitions into actual configurations, see
- * {@link ConfigurationResolver}.
+ * <p>For the work of turning these transitions into actual configurations, see {@link
+ * ConfigurationResolver}.
  *
  * <p>This is the "generic engine" for configuration selection. It doesn't know anything about
- * specific rules or their requirements. Rule writers decide those with appropriately placed
- * {@link PatchTransition} declarations. This class then processes those declarations to determine
- * final transitions.
+ * specific rules or their requirements. Rule writers decide those with appropriately placed {@link
+ * PatchTransition} declarations. This class then processes those declarations to determine final
+ * transitions.
  */
 public final class TransitionResolver {
   private final DynamicTransitionMapper transitionMapper;
@@ -121,7 +121,31 @@ public final class TransitionResolver {
           attribute.getConfigurationTransition());
     }
 
-    return applyConfigurationHook(currentTransition, toTarget);
+    // IV. Applies any rule transitions associated with the dep target, composes their transitions
+    // with a passed-in existing transition, and returns the composed result.
+    return applyRuleTransition(currentTransition, toTarget, transitionMapper);
+  }
+
+  /**
+   * Same as evaluateTransition except does not check for transitions coming from parents and
+   * enables support for rule-triggered top-level configuration hooks.
+   */
+  public static Attribute.Transition evaluateTopLevelTransition(
+      TargetAndConfiguration targetAndConfig, DynamicTransitionMapper dynamicTransitionMapper) {
+    Target target = targetAndConfig.getTarget();
+    BuildConfiguration fromConfig = targetAndConfig.getConfiguration();
+
+    // Top-level transitions (chosen by configuration fragments):
+    Transition topLevelTransition = fromConfig.topLevelConfigurationHook(target);
+    if (topLevelTransition == null) {
+      topLevelTransition = ConfigurationTransition.NONE;
+    }
+
+    // Rule class transitions (chosen by rule class definitions):
+    if (target.getAssociatedRule() == null) {
+      return topLevelTransition;
+    }
+    return applyRuleTransition(topLevelTransition, target, dynamicTransitionMapper);
   }
 
   /**
@@ -183,10 +207,14 @@ public final class TransitionResolver {
   }
 
   /**
-   * Applies any configuration hooks associated with the dep target, composes their transitions
-   * after an existing transition, and returns the composed result.
+   * @param currentTransition a pre-existing transition to be composed with
+   * @param toTarget rule to examine for transitions
+   * @param transitionMapper only needed because of Attribute.ConfigurationTransition.DATA: this is
+   *     C++-specific but non-C++ rules declare it. So they can't directly provide the C++-specific
+   *     patch transition that implements it.
    */
-  private Transition applyConfigurationHook(Transition currentTransition, Target toTarget) {
+  private static Transition applyRuleTransition(
+      Transition currentTransition, Target toTarget, DynamicTransitionMapper transitionMapper) {
     if (isFinal(currentTransition)) {
       return currentTransition;
     }
@@ -194,9 +222,6 @@ public final class TransitionResolver {
     RuleTransitionFactory transitionFactory =
         associatedRule.getRuleClassObject().getTransitionFactory();
     if (transitionFactory != null) {
-      // transitionMapper is only needed because of Attribute.ConfigurationTransition.DATA: this is
-      // C++-specific but non-C++ rules declare it. So they can't directly provide the C++-specific
-      // patch transition that implements it.
       PatchTransition ruleClassTransition = (PatchTransition)
           transitionMapper.map(transitionFactory.buildTransitionFor(associatedRule));
       if (ruleClassTransition != null) {