Dynamic configurations: trim top-level targets, too.

Right now, configuration trimming happens in
ConfiguredTargetFunction.computeDependencies. This means only
the deps of other targets get trimmed.

With this change, every ConfiguredTarget gets its configuration
accurately trimmed, regardless of where it comes from or what
it's used for.

In practice, there could still be other code paths that
instantiate ConfiguredTargetValue.key without pre-trimming.
We'll have to tackle those as we hit them.

Also cleaned up some symbol naming in BuildView.update to try
to make the logic flow clearer.

TESTED: BuildViewTest#testNewActionsAreDifferentAndDontConflict now
passes with dynamic configs (among others)

--
MOS_MIGRATED_REVID=123807892

4 files changed, 159 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index d12c1eddb2..3c1aaf918e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.analysis;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Predicate;
+import com.google.common.base.Verify;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
@@ -65,6 +66,7 @@ import com.google.devtools.build.lib.skyframe.ActionLookupValue;
 import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue;
 import com.google.devtools.build.lib.skyframe.CoverageReportValue;
 import com.google.devtools.build.lib.skyframe.SkyframeAnalysisResult;
 import com.google.devtools.build.lib.skyframe.SkyframeBuildView;
@@ -425,10 +427,11 @@ public class BuildView {
     skyframeBuildView.setConfigurations(configurations);
 
     // Determine the configurations.
-    List<TargetAndConfiguration> nodes = nodesForTargets(configurations, targets);
+    List<TargetAndConfiguration> topLevelTargetsWithConfigs =
+        nodesForTopLevelTargets(configurations, targets, eventHandler);
 
-    List<ConfiguredTargetKey> targetSpecs =
-        Lists.transform(nodes, new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
+    List<ConfiguredTargetKey> topLevelCtKeys = Lists.transform(topLevelTargetsWithConfigs,
+        new Function<TargetAndConfiguration, ConfiguredTargetKey>() {
           @Override
           public ConfiguredTargetKey apply(TargetAndConfiguration node) {
             return new ConfiguredTargetKey(node.getLabel(), node.getConfiguration());
@@ -446,7 +449,7 @@ public class BuildView {
         PathFragment bzlFile = new PathFragment("/" + aspect.substring(0, delimiterPosition));
 
         String skylarkFunctionName = aspect.substring(delimiterPosition + 1);
-        for (ConfiguredTargetKey targetSpec : targetSpecs) {
+        for (ConfiguredTargetKey targetSpec : topLevelCtKeys) {
           aspectKeys.add(
               AspectValue.createSkylarkAspectKey(
                   targetSpec.getLabel(),
@@ -461,7 +464,7 @@ public class BuildView {
         final NativeAspectClass aspectFactoryClass =
             ruleClassProvider.getNativeAspectClassMap().get(aspect);
         if (aspectFactoryClass != null) {
-          for (ConfiguredTargetKey targetSpec : targetSpecs) {
+          for (ConfiguredTargetKey targetSpec : topLevelCtKeys) {
             aspectKeys.add(
                 AspectValue.createAspectKey(
                     targetSpec.getLabel(),
@@ -482,13 +485,13 @@ public class BuildView {
     try {
       skyframeAnalysisResult =
           skyframeBuildView.configureTargets(
-              eventHandler, targetSpecs, aspectKeys, eventBus, viewOptions.keepGoing);
+              eventHandler, topLevelCtKeys, aspectKeys, eventBus, viewOptions.keepGoing);
       setArtifactRoots(skyframeAnalysisResult.getPackageRoots(), configurations);
     } finally {
       skyframeBuildView.clearInvalidatedConfiguredTargets();
     }
 
-    int numTargetsToAnalyze = nodes.size();
+    int numTargetsToAnalyze = topLevelTargetsWithConfigs.size();
     int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size();
     if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) {
       String msg = String.format("Analysis succeeded for only %d of %d top-level targets",
@@ -665,9 +668,15 @@ public class BuildView {
     }
   }
 
-  @VisibleForTesting
-  List<TargetAndConfiguration> nodesForTargets(BuildConfigurationCollection configurations,
-      Collection<Target> targets) {
+  /**
+   * Given a set of top-level targets and a configuration collection, returns the appropriate
+   * <Target, Configuration> pair for each target.
+   *
+   * <p>Preserves the original input ordering.
+   */
+  private List<TargetAndConfiguration> nodesForTopLevelTargets(
+      BuildConfigurationCollection configurations, Collection<Target> targets,
+      EventHandler eventHandler) throws InterruptedException {
     // We use a hash set here to remove duplicate nodes; this can happen for input files and package
     // groups.
     LinkedHashSet<TargetAndConfiguration> nodes = new LinkedHashSet<>(targets.size());
@@ -677,9 +686,74 @@ public class BuildView {
             BuildConfigurationCollection.configureTopLevelTarget(config, target)));
       }
     }
-    return ImmutableList.copyOf(nodes);
+    return configurations.useDynamicConfigurations()
+        ? trimConfigurations(nodes, eventHandler)
+        : ImmutableList.copyOf(nodes);
+  }
+
+  /**
+   * 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>Preserves the original input order. 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).
+   */
+  // TODO(bazel-team): error out early for targets that fail - untrimmed configurations should
+  // never make it through analysis (and especially not seed ConfiguredTargetValues)
+  private List<TargetAndConfiguration> trimConfigurations(Iterable<TargetAndConfiguration> inputs,
+      EventHandler eventHandler) throws InterruptedException {
+    Map<Label, TargetAndConfiguration> labelsToTargets = new LinkedHashMap<>();
+    BuildConfiguration topLevelConfig = null;
+    List<Dependency> asDeps = new ArrayList<Dependency>();
+
+    for (TargetAndConfiguration targetAndConfig : inputs) {
+      labelsToTargets.put(targetAndConfig.getLabel(), targetAndConfig);
+      BuildConfiguration targetConfig = targetAndConfig.getConfiguration();
+      if (targetConfig != null) {
+        asDeps.add(Dependency.withTransitionAndAspects(
+            targetAndConfig.getLabel(),
+            Attribute.ConfigurationTransition.NONE,
+            ImmutableSet.<AspectDescriptor>of()));  // TODO(bazel-team): support top-level aspects
+
+        // TODO(bazel-team): support multiple top-level configurations (for, e.g.,
+        // --experimental_multi_cpu). This requires refactoring the getConfigurations() call below.
+        Verify.verify(topLevelConfig == null || topLevelConfig == targetConfig);
+        topLevelConfig = targetConfig;
+      }
+    }
+
+    Map<Label, TargetAndConfiguration> successfullyEvaluatedTargets = new LinkedHashMap<>();
+    if (!asDeps.isEmpty()) {
+      Map<Dependency, BuildConfiguration> trimmedTargets =
+          skyframeExecutor.getConfigurations(eventHandler, topLevelConfig.getOptions(), asDeps);
+      for (Map.Entry<Dependency, BuildConfiguration> trimmedTarget : trimmedTargets.entrySet()) {
+        Label targetLabel = trimmedTarget.getKey().getLabel();
+        successfullyEvaluatedTargets.put(targetLabel,
+            new TargetAndConfiguration(
+                labelsToTargets.get(targetLabel).getTarget(), trimmedTarget.getValue()));
+      }
+    }
+
+    ImmutableList.Builder<TargetAndConfiguration> result =
+        ImmutableList.<TargetAndConfiguration>builder();
+    for (TargetAndConfiguration originalInput : inputs) {
+      if (successfullyEvaluatedTargets.containsKey(originalInput.getLabel())) {
+        // The configuration was successfully trimmed.
+        result.add(successfullyEvaluatedTargets.get(originalInput.getLabel()));
+      } else {
+        // Either the configuration couldn't be determined (e.g. loading phase error) or it's null.
+        result.add(originalInput);
+      }
+    }
+    return result.build();
   }
 
+
   /**
    * Sets the possible artifact roots in the artifact factory. This allows the factory to resolve
    * paths with unknown roots to artifacts.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
index cf55a5426d..3b71ba34f2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
@@ -109,6 +109,13 @@ public final class BuildConfigurationCollection {
     return result;
   }
 
+  /**
+   * Returns whether this build uses dynamic configurations.
+   */
+  public boolean useDynamicConfigurations() {
+    return getTargetConfigurations().get(0).useDynamicConfigurations();
+  }
+
   @Override
   public boolean equals(Object obj) {
     if (this == obj) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 4590afe6e8..8dfa3d54d2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1128,12 +1128,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   /**
    * Returns the {@link ConfiguredTarget}s corresponding to the given keys.
    *
-   * <p>For use for legacy support from {@code BuildView} only.
+   * <p>For use for legacy support and tests calling through {@code BuildView} only.
    *
    * <p>If a requested configured target is in error, the corresponding value is omitted from the
    * returned list.
    */
   @ThreadSafety.ThreadSafe
+  // TODO(bazel-team): rename this and below methods to something that discourages general use
   public ImmutableList<ConfiguredTarget> getConfiguredTargets(
       EventHandler eventHandler, BuildConfiguration originalConfig, Iterable<Dependency> keys,
       boolean useOriginalConfig) {
@@ -1141,6 +1142,15 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         eventHandler, originalConfig, keys, useOriginalConfig).values().asList();
   }
 
+  /**
+   * Returns a map from {@link Dependency} inputs to the {@link ConfiguredTarget}s corresponding
+   * to those dependencies.
+   *
+   * <p>For use for legacy support and tests calling through {@code BuildView} only.
+   *
+   * <p>If a requested configured target is in error, the corresponding value is omitted from the
+   * returned list.
+   */
   @ThreadSafety.ThreadSafe
   public ImmutableMap<Dependency, ConfiguredTarget> getConfiguredTargetMap(
       EventHandler eventHandler, BuildConfiguration originalConfig, Iterable<Dependency> keys,
@@ -1173,6 +1183,11 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
     final List<SkyKey> skyKeys = new ArrayList<>();
     for (Dependency key : keys) {
+      if (!configs.containsKey(key)) {
+        // If we couldn't compute a configuration for this target, the target was in error (e.g.
+        // it couldn't be loaded). Exclude it from the results.
+        continue;
+      }
       skyKeys.add(ConfiguredTargetValue.key(key.getLabel(), configs.get(key)));
       for (AspectDescriptor aspectDescriptor : key.getAspects()) {
         skyKeys.add(
@@ -1188,6 +1203,11 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   DependentNodeLoop:
     for (Dependency key : keys) {
+      if (!configs.containsKey(key)) {
+        // If we couldn't compute a configuration for this target, the target was in error (e.g.
+        // it couldn't be loaded). Exclude it from the results.
+        continue;
+      }
       SkyKey configuredTargetKey = ConfiguredTargetValue.key(
           key.getLabel(), configs.get(key));
       if (result.get(configuredTargetKey) == null) {
@@ -1220,8 +1240,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   /**
    * Retrieves the configurations needed for the given deps, trimming down their fragments
    * to those only needed by their transitive closures.
+   *
+   * <p>Skips targets with loading phase errors.
    */
-  private Map<Dependency, BuildConfiguration> getConfigurations(EventHandler eventHandler,
+  public Map<Dependency, BuildConfiguration> getConfigurations(EventHandler eventHandler,
       BuildOptions fromOptions, Iterable<Dependency> keys) {
     Map<Dependency, BuildConfiguration> builder = new HashMap<>();
     Set<Dependency> depsToEvaluate = new HashSet<>();
@@ -1243,7 +1265,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       }
     }
     EvaluationResult<SkyValue> fragmentsResult = evaluateSkyKeys(
-        eventHandler, transitiveFragmentSkyKeys);
+        eventHandler, transitiveFragmentSkyKeys, /*keepGoing=*/true);
     for (Dependency key : keys) {
       if (!depsToEvaluate.contains(key)) {
         // No fragments to compute here.
@@ -1267,7 +1289,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       configSkyKeys.add(BuildConfigurationValue.key(depFragments,
           getDynamicConfigOptions(key, fromOptions, depFragments)));
     }
-    EvaluationResult<SkyValue> configsResult = evaluateSkyKeys(eventHandler, configSkyKeys);
+    EvaluationResult<SkyValue> configsResult =
+        evaluateSkyKeys(eventHandler, configSkyKeys, /*keepGoing=*/true);
     for (Dependency key : keys) {
       if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) {
         continue;
@@ -1302,10 +1325,20 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   /**
-   * Evaluates the given sky keys, blocks, and returns their evaluation results.
+   * Evaluates the given sky keys, blocks, and returns their evaluation results. Fails fast
+   * on the first evaluation error.
    */
   private EvaluationResult<SkyValue> evaluateSkyKeys(
       final EventHandler eventHandler, final Iterable<SkyKey> skyKeys) {
+    return evaluateSkyKeys(eventHandler, skyKeys, false);
+  }
+
+  /**
+   * Evaluates the given sky keys, blocks, and returns their evaluation results. Enables/disables
+   * "keep going" on evaluation errors as specified.
+   */
+  private EvaluationResult<SkyValue> evaluateSkyKeys(
+      final EventHandler eventHandler, final Iterable<SkyKey> skyKeys, final boolean keepGoing) {
     EvaluationResult<SkyValue> result;
     try {
       result = callUninterruptibly(new Callable<EvaluationResult<SkyValue>>() {
@@ -1314,7 +1347,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
           synchronized (valueLookupLock) {
             try {
               skyframeBuildView.enableAnalysis(true);
-              return buildDriver.evaluate(skyKeys, false, DEFAULT_THREAD_COUNT, eventHandler);
+              return buildDriver.evaluate(skyKeys, keepGoing, DEFAULT_THREAD_COUNT, eventHandler);
             } finally {
               skyframeBuildView.enableAnalysis(false);
             }
@@ -1355,15 +1388,19 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting()) == null) {
       injectWorkspaceStatusData();
     }
+
+    Dependency dep;
+    if (configuration == null) {
+      dep = Dependency.withNullConfiguration(label);
+    } else if (configuration.useDynamicConfigurations()) {
+      dep = Dependency.withTransitionAndAspects(label, Attribute.ConfigurationTransition.NONE,
+          ImmutableSet.<AspectDescriptor>of());
+    } else {
+      dep = Dependency.withConfiguration(label, configuration);
+    }
+
     return Iterables.getFirst(
-        getConfiguredTargets(
-            eventHandler,
-            configuration,
-            ImmutableList.of(
-                configuration != null
-                    ? Dependency.withConfiguration(label, configuration)
-                    : Dependency.withNullConfiguration(label)),
-            true),
+        getConfiguredTargets(eventHandler, configuration, ImmutableList.of(dep), false),
         null);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 6bb032534f..20f47e13c6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -151,8 +151,8 @@ public class BuildViewTest extends BuildViewTestBase {
     OutputFileConfiguredTarget outputCT = (OutputFileConfiguredTarget)
         getConfiguredTarget("//pkg:a.out");
     Artifact outputArtifact = outputCT.getArtifact();
-    assertEquals(getTargetConfiguration().getBinDirectory(), outputArtifact.getRoot());
-    assertEquals(getTargetConfiguration().getBinFragment().getRelative("pkg/a.out"),
+    assertEquals(outputCT.getConfiguration().getBinDirectory(), outputArtifact.getRoot());
+    assertEquals(outputCT.getConfiguration().getBinFragment().getRelative("pkg/a.out"),
         outputArtifact.getExecPath());
     assertEquals(new PathFragment("pkg/a.out"), outputArtifact.getRootRelativePath());
 
@@ -345,7 +345,7 @@ public class BuildViewTest extends BuildViewTestBase {
       fileDependency =
           Dependency.withTransitionAndAspects(
               Label.parseAbsolute("//package:file"),
-              Attribute.ConfigurationTransition.NONE,
+              Attribute.ConfigurationTransition.NULL,
               ImmutableSet.<AspectDescriptor>of());
     } else {
       innerDependency =
@@ -1214,6 +1214,19 @@ public class BuildViewTest extends BuildViewTestBase {
         + "by '//parent:a'", 1);
   }
 
+  @Test
+  public void testTopLevelTargetsAreTrimmedWithDynamicConfigurations() throws Exception {
+    scratch.file("foo/BUILD",
+        "sh_library(name='x', ",
+        "        srcs=['x.sh'])");
+    useConfiguration("--experimental_dynamic_configs");
+    AnalysisResult res = update("//foo:x");
+    ConfiguredTarget topLevelTarget = Iterables.getOnlyElement(res.getTargetsToBuild());
+    assertThat(topLevelTarget.getConfiguration().getAllFragments().keySet()).containsExactly(
+        ruleClassProvider.getUniversalFragment());
+  }
+
+
   /** Runs the same test with the reduced loading phase. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)