Include broken packages in rbuildfiles response

Also allow overrides for SkyQuery's
getBuildFileTargetsForPackageKeysAndProcessViaCallback.

Lastly, some cleanup in ParallelSkyQueryUtils.

RELNOTES: SkyQuery's rbuildfiles now returns targets corresponding to broken packages.
PiperOrigin-RevId: 196868692

2 files changed, 13 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/query2/ParallelSkyQueryUtils.java b/src/main/java/com/google/devtools/build/lib/query2/ParallelSkyQueryUtils.java
index 1e1ff15eac..b14f69bb11 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/ParallelSkyQueryUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/ParallelSkyQueryUtils.java
@@ -208,11 +208,14 @@ public class ParallelSkyQueryUtils {
 
   /** A helper class that computes 'rbuildfiles(<blah>)' via BFS. */
   private static class RBuildFilesVisitor extends AbstractSkyKeyParallelVisitor<Target> {
+
     // Each target in the full output of 'rbuildfiles' corresponds to BUILD file InputFile of a
     // unique package. So the processResultsBatchSize we choose to pass to the ParallelVisitor ctor
     // influences how many packages each leaf task doing processPartialResults will have to
     // deal with at once. A value of 100 was chosen experimentally.
     private static final int PROCESS_RESULTS_BATCH_SIZE = 100;
+    private static final SkyKey EXTERNAL_PACKAGE_KEY =
+        PackageValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER);
     private final SkyQueryEnvironment env;
 
     private RBuildFilesVisitor(
@@ -232,7 +235,7 @@ public class ParallelSkyQueryUtils {
         if (rdep.functionName().equals(SkyFunctions.PACKAGE)) {
           keysToUseForResult.add(rdep);
           // Every package has a dep on the external package, so we need to include those edges too.
-          if (rdep.equals(PackageValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER))) {
+          if (rdep.equals(EXTERNAL_PACKAGE_KEY)) {
             keysToVisitNext.add(rdep);
           }
         } else if (!rdep.functionName().equals(SkyFunctions.PACKAGE_LOOKUP)
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 8e102d8730..ea07744ab9 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -986,6 +986,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
       PathFragment currentPathFragment) {
     if (originalFileFragment.equals(currentPathFragment)
         && originalFileFragment.equals(Label.WORKSPACE_FILE_NAME)) {
+      // TODO(mschaller): this should not be checked at runtime. These are constants!
       Preconditions.checkState(
           Label.WORKSPACE_FILE_NAME.getParentDirectory().equals(PathFragment.EMPTY_FRAGMENT),
           Label.WORKSPACE_FILE_NAME);
@@ -995,9 +996,9 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
     }
     PathFragment parentPathFragment = currentPathFragment.getParentDirectory();
     return parentPathFragment == null
-        ? ImmutableList.<SkyKey>of()
-        : ImmutableList.of(PackageLookupValue.key(
-            PackageIdentifier.createInMainRepo(parentPathFragment)));
+        ? ImmutableList.of()
+        : ImmutableList.of(
+            PackageLookupValue.key(PackageIdentifier.createInMainRepo(parentPathFragment)));
   }
 
   /**
@@ -1060,7 +1061,7 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
     return result;
   }
 
-  void getBuildFileTargetsForPackageKeysAndProcessViaCallback(
+  protected void getBuildFileTargetsForPackageKeysAndProcessViaCallback(
       Iterable<SkyKey> packageKeys, Callback<Target> callback)
       throws QueryException, InterruptedException {
     Set<PackageIdentifier> pkgIds =
@@ -1069,24 +1070,16 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
               .collect(toImmutableSet());
     packageSemaphore.acquireAll(pkgIds);
     try {
-      Iterable<SkyValue> packageValues = graph.getSuccessfulValues(packageKeys).values();
-      Iterable<Target> buildFileTargets = getBuildFileTargetsFromPackageValues(packageValues);
+      Iterable<Target> buildFileTargets =
+          Iterables.transform(
+              graph.getSuccessfulValues(packageKeys).values(),
+              skyValue -> ((PackageValue) skyValue).getPackage().getBuildFile());
       callback.process(buildFileTargets);
     } finally {
       packageSemaphore.releaseAll(pkgIds);
     }
   }
 
-  protected Iterable<Target> getBuildFileTargetsFromPackageValues(
-      Iterable<SkyValue> packageValues) {
-    // TODO(laurentlb): Use streams?
-    return Iterables.transform(
-        Iterables.filter(
-            Iterables.transform(packageValues, skyValue -> ((PackageValue) skyValue).getPackage()),
-            pkg -> !pkg.containsErrors()),
-        Package::getBuildFile);
-  }
-
   /**
    * Calculates the set of packages that transitively depend on, via load statements, the specified
    * paths. The emitted {@link Target}s are BUILD file targets.