Remove LoadingPhaseRunner, inline the only implementation

PiperOrigin-RevId: 200210007

5 files changed, 104 insertions, 182 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 755ea82151..55eed1a5cc 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
@@ -61,7 +61,6 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.pkgcache.LoadingCallback;
 import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
-import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.profiler.ProfilePhase;
 import com.google.devtools.build.lib.profiler.Profiler;
@@ -428,11 +427,8 @@ public class BuildTool {
       }
     };
 
-    LoadingPhaseRunner loadingPhaseRunner =
-        env.getSkyframeExecutor().getLoadingPhaseRunner(
-            runtime.getPackageFactory().getRuleClassNames());
     LoadingResult result =
-        loadingPhaseRunner.execute(
+        env.getSkyframeExecutor().loadTargetPatterns(
             getReporter(),
             request.getTargets(),
             env.getRelativeWorkingDirectory(),
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
deleted file mode 100644
index af09f1ab5a..0000000000
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
+++ /dev/null
@@ -1,89 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.pkgcache;
-
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
-import com.google.devtools.build.lib.cmdline.TargetParsingException;
-import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
-import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.Root;
-import java.util.Collection;
-import java.util.List;
-import javax.annotation.Nullable;
-
-/**
- * Implements the loading phase; responsible for:
- * <ul>
- *   <li>target pattern evaluation
- *   <li>test suite expansion
- * </ul>
- *
- * <p>In order to ensure correctness of incremental loading and of full cache hits, this class is
- * very restrictive about access to its internal state and to its collaborators. In particular, none
- * of the collaborators of this class may change in incompatible ways, such as changing the relative
- * working directory for the target pattern parser, without notifying this class.
- *
- * <p>For full caching, this class tracks the exact values of all inputs to the loading phase. To
- * maximize caching, it is vital that these change as rarely as possible.
- */
-public abstract class LoadingPhaseRunner {
-  /** Performs target pattern evaluation and test suite expansion (if requested). */
-  public abstract LoadingResult execute(
-      ExtendedEventHandler eventHandler,
-      List<String> targetPatterns,
-      PathFragment relativeWorkingDirectory,
-      LoadingOptions options,
-      boolean keepGoing,
-      boolean determineTests,
-      @Nullable LoadingCallback callback)
-      throws TargetParsingException, LoadingFailedException, InterruptedException;
-
-  /** Returns a map of collected package names to root paths. */
-  public static ImmutableMap<PackageIdentifier, Root> collectPackageRoots(
-      Collection<Package> packages) {
-    // Make a map of the package names to their root paths.
-    ImmutableMap.Builder<PackageIdentifier, Root> packageRoots = ImmutableMap.builder();
-    for (Package pkg : packages) {
-      packageRoots.put(pkg.getPackageIdentifier(), pkg.getSourceRoot());
-    }
-    return packageRoots.build();
-  }
-
-  /**
-   * Emit a warning when a deprecated target is mentioned on the command line.
-   *
-   * <p>Note that this does not stop us from emitting "target X depends on deprecated target Y"
-   * style warnings for the same target and it is a good thing; <i>depending</i> on a target and
-   * <i>wanting</i> to build it are different things.
-   */
-  // Public for use by skyframe.TargetPatternPhaseFunction until this class goes away.
-  public static void maybeReportDeprecation(
-      ExtendedEventHandler eventHandler, Collection<Target> targets) {
-    for (Rule rule : Iterables.filter(targets, Rule.class)) {
-      if (rule.isAttributeValueExplicitlySpecified("deprecation")) {
-        eventHandler.handle(Event.warn(rule.getLocation(), String.format(
-            "target '%s' is deprecated: %s", rule.getLabel(),
-            NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING))));
-      }
-    }
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index fefb8fb6cf..131c4912eb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -50,6 +50,7 @@ import com.google.devtools.build.lib.causes.Cause;
 import com.google.devtools.build.lib.causes.LabelCause;
 import com.google.devtools.build.lib.causes.LoadingFailedCause;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
@@ -60,7 +61,6 @@ import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent;
-import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException;
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException;
@@ -248,8 +248,7 @@ public final class SkyframeBuildView {
     }
     PackageRoots packageRoots =
         singleSourceRoot == null
-            ? new MapAsPackageRoots(
-                LoadingPhaseRunner.collectPackageRoots(packages.build().toCollection()))
+            ? new MapAsPackageRoots(collectPackageRoots(packages.build().toCollection()))
             : new PackageRootsNoSymlinkCreation(singleSourceRoot);
 
     if (!result.hasError() && badActions.isEmpty()) {
@@ -416,6 +415,17 @@ public final class SkyframeBuildView {
         packageRoots);
   }
 
+  /** Returns a map of collected package names to root paths. */
+  private static ImmutableMap<PackageIdentifier, Root> collectPackageRoots(
+      Collection<Package> packages) {
+    // Make a map of the package names to their root paths.
+    ImmutableMap.Builder<PackageIdentifier, Root> packageRoots = ImmutableMap.builder();
+    for (Package pkg : packages) {
+      packageRoots.put(pkg.getPackageIdentifier(), pkg.getSourceRoot());
+    }
+    return packageRoots.build();
+  }
+
   @Nullable
   private static Label maybeGetConfiguredTargetCycleCulprit(
       Label labelToLoad, Iterable<CycleInfo> cycleInfos) {
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 69a9b436ed..1e0a4f0677 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
@@ -110,7 +110,6 @@ import com.google.devtools.build.lib.pkgcache.LoadingCallback;
 import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
 import com.google.devtools.build.lib.pkgcache.LoadingOptions;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseCompleteEvent;
-import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
@@ -2211,93 +2210,76 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    */
   public abstract void deleteOldNodes(long versionWindowForDirtyGc);
 
-  public LoadingPhaseRunner getLoadingPhaseRunner(Set<String> ruleClassNames) {
-    return new SkyframeLoadingPhaseRunner(ruleClassNames);
-  }
-
-  /**
-   * Skyframe-based implementation of {@link LoadingPhaseRunner} based on {@link
-   * TargetPatternPhaseFunction}.
-   */
-  final class SkyframeLoadingPhaseRunner extends LoadingPhaseRunner {
-    private final Set<String> ruleClassNames;
-
-    public SkyframeLoadingPhaseRunner(Set<String> ruleClassNames) {
-      this.ruleClassNames = ruleClassNames;
-    }
-
-    @Override
-    public LoadingResult execute(
-        ExtendedEventHandler eventHandler,
-        List<String> targetPatterns,
-        PathFragment relativeWorkingDirectory,
-        LoadingOptions options,
-        boolean keepGoing,
-        boolean determineTests,
-        @Nullable LoadingCallback callback)
-        throws TargetParsingException, LoadingFailedException, InterruptedException {
-      Stopwatch timer = Stopwatch.createStarted();
-      SkyKey key =
-          TargetPatternPhaseValue.key(
-              ImmutableList.copyOf(targetPatterns),
-              relativeWorkingDirectory.getPathString(),
-              options.compileOneDependency,
-              options.buildTestsOnly,
-              determineTests,
-              ImmutableList.copyOf(options.buildTagFilterList),
-              options.buildManualTests,
-              options.expandTestSuites,
-              TestFilter.forOptions(options, eventHandler, ruleClassNames));
-      EvaluationResult<TargetPatternPhaseValue> evalResult;
-      eventHandler.post(new LoadingPhaseStartedEvent(packageProgress));
-      evalResult =
-          buildDriver.evaluate(
-              ImmutableList.of(key), keepGoing, /*numThreads=*/ DEFAULT_THREAD_COUNT, eventHandler);
-      if (evalResult.hasError()) {
-        ErrorInfo errorInfo = evalResult.getError(key);
-        TargetParsingException exc;
-        if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
-          exc = new TargetParsingException("cycles detected during target parsing");
-          getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler);
-          // Fallback: we don't know which patterns failed, specifically, so we report the entire
-          // set as being in error.
+  public LoadingResult loadTargetPatterns(
+      ExtendedEventHandler eventHandler,
+      List<String> targetPatterns,
+      PathFragment relativeWorkingDirectory,
+      LoadingOptions options,
+      boolean keepGoing,
+      boolean determineTests,
+      @Nullable LoadingCallback callback)
+      throws TargetParsingException, LoadingFailedException, InterruptedException {
+    Stopwatch timer = Stopwatch.createStarted();
+    SkyKey key =
+        TargetPatternPhaseValue.key(
+            ImmutableList.copyOf(targetPatterns),
+            relativeWorkingDirectory.getPathString(),
+            options.compileOneDependency,
+            options.buildTestsOnly,
+            determineTests,
+            ImmutableList.copyOf(options.buildTagFilterList),
+            options.buildManualTests,
+            options.expandTestSuites,
+            TestFilter.forOptions(options, eventHandler, pkgFactory.getRuleClassNames()));
+    EvaluationResult<TargetPatternPhaseValue> evalResult;
+    eventHandler.post(new LoadingPhaseStartedEvent(packageProgress));
+    evalResult =
+        buildDriver.evaluate(
+            ImmutableList.of(key), keepGoing, /*numThreads=*/ DEFAULT_THREAD_COUNT, eventHandler);
+    if (evalResult.hasError()) {
+      ErrorInfo errorInfo = evalResult.getError(key);
+      TargetParsingException exc;
+      if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
+        exc = new TargetParsingException("cycles detected during target parsing");
+        getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler);
+        // Fallback: we don't know which patterns failed, specifically, so we report the entire
+        // set as being in error.
+        eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage()));
+      } else {
+        // TargetPatternPhaseFunction never directly throws. Thus, the only way
+        // evalResult.hasError() && keepGoing can hold is if there are cycles, which is handled
+        // above.
+        Preconditions.checkState(!keepGoing);
+        // Following SkyframeTargetPatternEvaluator, we convert any exception into a
+        // TargetParsingException.
+        Exception e = Preconditions.checkNotNull(errorInfo.getException());
+        exc =
+            (e instanceof TargetParsingException)
+                ? (TargetParsingException) e
+                : new TargetParsingException(e.getMessage(), e);
+        if (!(e instanceof TargetParsingException)) {
+          // If it's a TargetParsingException, then the TargetPatternPhaseFunction has already
+          // reported the error, so we don't need to report it again.
           eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage()));
-        } else {
-          // TargetPatternPhaseFunction never directly throws. Thus, the only way
-          // evalResult.hasError() && keepGoing can hold is if there are cycles, which is handled
-          // above.
-          Preconditions.checkState(!keepGoing);
-          // Following SkyframeTargetPatternEvaluator, we convert any exception into a
-          // TargetParsingException.
-          Exception e = Preconditions.checkNotNull(errorInfo.getException());
-          exc =
-              (e instanceof TargetParsingException)
-                  ? (TargetParsingException) e
-                  : new TargetParsingException(e.getMessage(), e);
-          if (!(e instanceof TargetParsingException)) {
-            // If it's a TargetParsingException, then the TargetPatternPhaseFunction has already
-            // reported the error, so we don't need to report it again.
-            eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage()));
-          }
         }
-        throw exc;
       }
-      long timeMillis = timer.stop().elapsed(TimeUnit.MILLISECONDS);
+      throw exc;
+    }
+    long timeMillis = timer.stop().elapsed(TimeUnit.MILLISECONDS);
 
-      TargetPatternPhaseValue patternParsingValue = evalResult.get(key);
-      eventHandler.post(new TargetParsingPhaseTimeEvent(timeMillis));
-      ImmutableSet<Target> targets = patternParsingValue.getTargets(eventHandler, packageManager);
-      if (callback != null) {
-        callback.notifyTargets(targets);
-      }
-      eventHandler.post(
-          new LoadingPhaseCompleteEvent(
-              targets,
-              patternParsingValue.getRemovedTargets(eventHandler, packageManager),
-              PackageManagerStatistics.ZERO,
-              /*timeInMs=*/ 0));
-      return patternParsingValue.toLoadingResult(eventHandler, packageManager);
+    TargetPatternPhaseValue patternParsingValue = evalResult.get(key);
+    eventHandler.post(new TargetParsingPhaseTimeEvent(timeMillis));
+    ImmutableSet<Target> targets = patternParsingValue.getTargets(eventHandler, packageManager);
+    if (callback != null) {
+      callback.notifyTargets(targets);
     }
+    eventHandler.post(
+        new LoadingPhaseCompleteEvent(
+            targets,
+            patternParsingValue.getRemovedTargets(eventHandler, packageManager),
+            PackageManagerStatistics.ZERO,
+            /*timeInMs=*/ 0));
+    return patternParsingValue.toLoadingResult(eventHandler, packageManager);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index 2598dd8e9c..1bcf79ee11 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -16,18 +16,21 @@ package com.google.devtools.build.lib.skyframe;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.pkgcache.AbstractRecursivePackageProvider.MissingDepException;
 import com.google.devtools.build.lib.pkgcache.CompileOneDependencyTransformer;
 import com.google.devtools.build.lib.pkgcache.FilteringPolicies;
-import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.ParsingFailedEvent;
 import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
 import com.google.devtools.build.lib.pkgcache.TargetProvider;
@@ -35,11 +38,13 @@ import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue.TargetPatternPhaseKey;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternSkyKeyOrException;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -163,7 +168,7 @@ final class TargetPatternPhaseFunction implements SkyFunction {
       env.getListener().handle(Event.warn("Target pattern parsing failed."));
     }
 
-    LoadingPhaseRunner.maybeReportDeprecation(env.getListener(), targets.getTargets());
+    maybeReportDeprecation(env.getListener(), targets.getTargets());
 
     ResolvedTargets.Builder<Label> expandedLabelsBuilder = ResolvedTargets.builder();
     for (Target target : targets.getTargets()) {
@@ -213,6 +218,24 @@ final class TargetPatternPhaseFunction implements SkyFunction {
   }
 
   /**
+   * Emit a warning when a deprecated target is mentioned on the command line.
+   *
+   * <p>Note that this does not stop us from emitting "target X depends on deprecated target Y"
+   * style warnings for the same target and it is a good thing; <i>depending</i> on a target and
+   * <i>wanting</i> to build it are different things.
+   */
+  private static void maybeReportDeprecation(
+      ExtendedEventHandler eventHandler, Collection<Target> targets) {
+    for (Rule rule : Iterables.filter(targets, Rule.class)) {
+      if (rule.isAttributeValueExplicitlySpecified("deprecation")) {
+        eventHandler.handle(Event.warn(rule.getLocation(), String.format(
+            "target '%s' is deprecated: %s", rule.getLabel(),
+            NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING))));
+      }
+    }
+  }
+
+  /**
    * Interpret the command-line arguments.
    *
    * @param options the command-line arguments in structured form