Code cleanup for interleaving target pattern eval and config creation.

--
MOS_MIGRATED_REVID=139209942

7 files changed, 27 insertions, 41 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 522f753586..8eafd9d6fc 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -150,10 +150,7 @@ public final class BuildTool {
         executionTool.init();
       }
 
-      // Loading phase.
-      loadingResult = runLoadingPhase(request, validator);
-
-      // Create the build configurations.
+      // Error out early if multi_cpus is set, but we're not in build or test command.
       if (!request.getMultiCpus().isEmpty()) {
         getReporter().handle(Event.warn(
             "The --experimental_multi_cpu option is _very_ experimental and only intended for "
@@ -165,9 +162,25 @@ public final class BuildTool {
               + "'test' right now!");
         }
       }
-      configurations = env.getSkyframeExecutor().createConfigurations(
-            env.getReporter(), runtime.getConfigurationFactory(), buildOptions,
-            request.getMultiCpus(), request.getViewOptions().keepGoing);
+
+      // Exit if there are any pending exceptions from modules.
+      env.throwPendingException();
+
+      // Target pattern evaluation.
+      loadingResult = evaluateTargetPatterns(request, validator);
+
+      // Exit if there are any pending exceptions from modules.
+      env.throwPendingException();
+
+      // Configuration creation.
+      configurations =
+          env.getSkyframeExecutor()
+              .createConfigurations(
+                  env.getReporter(),
+                  runtime.getConfigurationFactory(),
+                  buildOptions,
+                  request.getMultiCpus(),
+                  request.getViewOptions().keepGoing);
 
       env.throwPendingException();
       if (configurations.getTargetConfigurations().size() == 1) {
@@ -389,14 +402,10 @@ public final class BuildTool {
     return !(request.getViewOptions().keepGoing && request.getExecutionOptions().testKeepGoing);
   }
 
-  @VisibleForTesting
-  protected final LoadingResult runLoadingPhase(final BuildRequest request,
-                                                final TargetValidator validator)
-          throws LoadingFailedException, TargetParsingException, InterruptedException,
-          AbruptExitException {
+  private final LoadingResult evaluateTargetPatterns(
+      final BuildRequest request, final TargetValidator validator)
+      throws LoadingFailedException, TargetParsingException, InterruptedException {
     Profiler.instance().markPhase(ProfilePhase.LOAD);
-    env.throwPendingException();
-
     initializeOutputFilter(request);
 
     final boolean keepGoing = request.getViewOptions().keepGoing;
@@ -420,11 +429,9 @@ public final class BuildTool {
             request.getTargets(),
             env.getRelativeWorkingDirectory(),
             request.getLoadingOptions(),
-            runtime.createBuildOptions(request).getAllLabels(),
             keepGoing,
             request.shouldRunTests(),
             callback);
-    env.throwPendingException();
     return result;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
index 448faf661e..018e03208d 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LegacyLoadingPhaseRunner.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.pkgcache;
 
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Sets;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -101,7 +100,6 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
       List<String> targetPatterns,
       PathFragment relativeWorkingDirectory,
       LoadingOptions options,
-      ListMultimap<String, Label> labelsToLoadUnconditionally,
       boolean keepGoing,
       boolean determineTests,
       @Nullable LoadingCallback callback)
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
index 7c7fbcad1d..1ef720f110 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
@@ -15,9 +15,7 @@ package com.google.devtools.build.lib.pkgcache;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.ListMultimap;
 import com.google.common.eventbus.EventBus;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.events.Event;
@@ -38,9 +36,6 @@ import javax.annotation.Nullable;
  * <ul>
  *   <li>target pattern evaluation
  *   <li>test suite expansion
- *   <li>loading the labels needed to construct the build configuration
- *   <li>loading the labels needed for the analysis with the build configuration
- *   <li>loading the transitive closure of the targets and the configuration labels
  * </ul>
  *
  * <p>In order to ensure correctness of incremental loading and of full cache hits, this class is
@@ -52,18 +47,13 @@ import javax.annotation.Nullable;
  * maximize caching, it is vital that these change as rarely as possible.
  */
 public abstract class LoadingPhaseRunner {
-  /**
-   * Performs target pattern evaluation, test suite expansion (if requested), and loads the
-   * transitive closure of the resulting targets as well as of the targets needed to use the given
-   * build configuration provider.
-   */
+  /** Performs target pattern evaluation and test suite expansion (if requested). */
   public abstract LoadingResult execute(
       EventHandler eventHandler,
       EventBus eventBus,
       List<String> targetPatterns,
       PathFragment relativeWorkingDirectory,
       LoadingOptions options,
-      ListMultimap<String, Label> labelsToLoadUnconditionally,
       boolean keepGoing,
       boolean determineTests,
       @Nullable LoadingCallback callback)
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 b4cb3059fd..a8fa1ae0b0 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
@@ -29,7 +29,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Range;
 import com.google.common.eventbus.EventBus;
@@ -1040,8 +1039,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
     SkyKey skyKey = ConfigurationCollectionValue.key(
         buildOptions, ImmutableSortedSet.copyOf(multiCpu));
-    EvaluationResult<ConfigurationCollectionValue> result = buildDriver.evaluate(
-            Arrays.asList(skyKey), keepGoing, DEFAULT_THREAD_COUNT, eventHandler);
+    EvaluationResult<ConfigurationCollectionValue> result =
+        buildDriver.evaluate(Arrays.asList(skyKey), keepGoing, DEFAULT_THREAD_COUNT, eventHandler);
     if (result.hasError()) {
       Throwable e = result.getError(skyKey).getException();
       // Wrap loading failed exceptions
@@ -1052,10 +1051,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       throw new IllegalStateException(
           "Unknown error during ConfigurationCollectionValue evaluation", e);
     }
-    Preconditions.checkState(result.values().size() == 1,
-        "Result of evaluate() must contain exactly one value %s", result);
-    ConfigurationCollectionValue configurationValue =
-        Iterables.getOnlyElement(result.values());
+    ConfigurationCollectionValue configurationValue = result.get(skyKey);
     return configurationValue.getConfigurationCollection();
   }
 
@@ -1845,7 +1841,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         List<String> targetPatterns,
         PathFragment relativeWorkingDirectory,
         LoadingOptions options,
-        ListMultimap<String, Label> labelsToLoadUnconditionally,
         boolean keepGoing,
         boolean determineTests,
         @Nullable LoadingCallback callback)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 25322fde31..7208a22aa2 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -309,7 +309,6 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
             ImmutableList.copyOf(labels),
             PathFragment.EMPTY_FRAGMENT,
             loadingOptions,
-            buildOptions.getAllLabels(),
             viewOptions.keepGoing,
             /*determineTests=*/false,
             /*callback=*/null);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index f5b15a85c8..3852494e7e 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1538,7 +1538,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
             targets,
             PathFragment.EMPTY_FRAGMENT,
             loadingOptions,
-            getTargetConfiguration().getAllLabels(),
             viewOptions.keepGoing,
             /*determineTests=*/false,
             /*callback=*/null);
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index d917969a67..79e097688e 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -25,7 +25,6 @@ import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -676,7 +675,6 @@ public class LoadingPhaseRunnerTest {
                 ImmutableList.copyOf(patterns),
                 PathFragment.EMPTY_FRAGMENT,
                 options,
-                ImmutableListMultimap.<String, Label>of(),
                 keepGoing,
                 determineTests,
                 loadingCallback);