Split PrepareDepsOfTargetsUnderDirectory into two parts, one which does the directory traversal and package loading, and the other which requests deps on all the transitive targets. We need values from the first half, but the second half can fail to evaluate because of a target cycle. By splitting them, we ensure that there will be values in the graph, so we can get the targets below a directory even if there are cycles present.

--
MOS_MIGRATED_REVID=111609889

1 files changed, 52 insertions, 92 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java
index ea210047df..e21ce8b2d2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java
@@ -13,11 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.analysis.BlazeDirectories;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -27,7 +25,6 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.FilteringPolicy;
 import com.google.devtools.build.lib.pkgcache.TargetPatternResolverUtil;
 import com.google.devtools.build.lib.skyframe.PrepareDepsOfTargetsUnderDirectoryValue.PrepareDepsOfTargetsUnderDirectoryKey;
-import com.google.devtools.build.lib.skyframe.RecursivePkgValue.RecursivePkgKey;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -35,6 +32,8 @@ import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 
 import javax.annotation.Nullable;
@@ -43,113 +42,74 @@ import javax.annotation.Nullable;
  * Ensures the graph contains the targets in the directory's package, if any, and in the
  * non-excluded packages in its subdirectories, and all those targets' transitive dependencies,
  * after a successful evaluation.
- *
- * <p>Computes {@link PrepareDepsOfTargetsUnderDirectoryValue} which describes whether the
- * directory is a package and how many non-excluded packages exist below each of the directory's
- * subdirectories.
  */
 public class PrepareDepsOfTargetsUnderDirectoryFunction implements SkyFunction {
-  private final BlazeDirectories directories;
-
-  public PrepareDepsOfTargetsUnderDirectoryFunction(BlazeDirectories directories) {
-    this.directories = directories;
-  }
-
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env) {
     PrepareDepsOfTargetsUnderDirectoryKey argument =
         (PrepareDepsOfTargetsUnderDirectoryKey) skyKey.argument();
     FilteringPolicy filteringPolicy = argument.getFilteringPolicy();
-    RecursivePkgKey recursivePkgKey = argument.getRecursivePkgKey();
-    return new MyTraversalFunction(filteringPolicy).visitDirectory(recursivePkgKey, env);
-  }
-
-  private class MyTraversalFunction
-      extends RecursiveDirectoryTraversalFunction<MyVisitor,
-      PrepareDepsOfTargetsUnderDirectoryValue> {
-
-    private final FilteringPolicy filteringPolicy;
-
-    private MyTraversalFunction(FilteringPolicy filteringPolicy) {
-      super(directories);
-      this.filteringPolicy = filteringPolicy;
-    }
-
-    @Override
-    protected PrepareDepsOfTargetsUnderDirectoryValue getEmptyReturn() {
-      return PrepareDepsOfTargetsUnderDirectoryValue.EMPTY;
-    }
-
-    @Override
-    protected MyVisitor getInitialVisitor() {
-      return new MyVisitor(filteringPolicy);
+    CollectPackagesUnderDirectoryValue collectPackagesUnderDirectoryValue =
+        (CollectPackagesUnderDirectoryValue)
+            env.getValue(CollectPackagesUnderDirectoryValue.key(argument.getRecursivePkgKey()));
+    if (env.valuesMissing()) {
+      return null;
     }
-
-    @Override
-    protected SkyKey getSkyKeyForSubdirectory(RepositoryName repository, RootedPath subdirectory,
-        ImmutableSet<PathFragment> excludedSubdirectoriesBeneathSubdirectory) {
-      return PrepareDepsOfTargetsUnderDirectoryValue.key(repository, subdirectory,
-          excludedSubdirectoriesBeneathSubdirectory, filteringPolicy);
-    }
-
-    @Override
-    protected PrepareDepsOfTargetsUnderDirectoryValue aggregateWithSubdirectorySkyValues(
-        MyVisitor visitor, Map<SkyKey, SkyValue> subdirectorySkyValues) {
-      // Aggregate the child subdirectory package state.
-      ImmutableMap.Builder<RootedPath, Boolean> builder = ImmutableMap.builder();
-      for (SkyKey key : subdirectorySkyValues.keySet()) {
-        PrepareDepsOfTargetsUnderDirectoryKey prepDepsKey =
-            (PrepareDepsOfTargetsUnderDirectoryKey) key.argument();
-        PrepareDepsOfTargetsUnderDirectoryValue prepDepsValue =
-            (PrepareDepsOfTargetsUnderDirectoryValue) subdirectorySkyValues.get(key);
-        boolean packagesInSubdirectory = prepDepsValue.isDirectoryPackage();
-        // If the subdirectory isn't a package, check to see if any of its subdirectories
-        // transitively contain packages.
-        if (!packagesInSubdirectory) {
-          ImmutableCollection<Boolean> subdirectoryValues =
-              prepDepsValue.getSubdirectoryTransitivelyContainsPackages().values();
-          for (Boolean pkgsInSubSub : subdirectoryValues) {
-            if (pkgsInSubSub) {
-              packagesInSubdirectory = true;
-              break;
-            }
-          }
-        }
-        builder.put(prepDepsKey.getRecursivePkgKey().getRootedPath(), packagesInSubdirectory);
+    Map<RootedPath, Boolean> subdirMap =
+        collectPackagesUnderDirectoryValue.getSubdirectoryTransitivelyContainsPackages();
+    List<SkyKey> subdirKeys = new ArrayList<>(subdirMap.size());
+    RepositoryName repositoryName = argument.getRecursivePkgKey().getRepository();
+    ImmutableSet<PathFragment> excludedPaths = argument.getRecursivePkgKey().getExcludedPaths();
+
+    PathFragment baseDir = argument.getRecursivePkgKey().getRootedPath().getRelativePath();
+    for (Map.Entry<RootedPath, Boolean> subdirEntry : subdirMap.entrySet()) {
+      if (subdirEntry.getValue()) {
+        // Keep in rough sync with the logic in RecursiveDirectoryTraversalFunction#visitDirectory.
+        RootedPath subdir = subdirEntry.getKey();
+        PathFragment subdirRelativePath = subdir.getRelativePath().relativeTo(baseDir);
+        ImmutableSet<PathFragment> excludedSubdirectoriesBeneathThisSubdirectory =
+            PathFragment.filterPathsStartingWith(excludedPaths, subdirRelativePath);
+
+        subdirKeys.add(
+            PrepareDepsOfTargetsUnderDirectoryValue.key(
+                repositoryName,
+                subdir,
+                excludedSubdirectoriesBeneathThisSubdirectory,
+                filteringPolicy));
       }
-      return PrepareDepsOfTargetsUnderDirectoryValue.of(visitor.isDirectoryPackage(),
-          builder.build());
     }
-  }
-
-  private static class MyVisitor implements RecursiveDirectoryTraversalFunction.Visitor {
-
-    private final FilteringPolicy filteringPolicy;
-    private boolean isDirectoryPackage;
-
-    private MyVisitor(FilteringPolicy filteringPolicy) {
-      this.filteringPolicy = Preconditions.checkNotNull(filteringPolicy);
-    }
-
-    @Override
-    public void visitPackageValue(Package pkg, Environment env) {
-      isDirectoryPackage = true;
-      loadTransitiveTargets(env, pkg, filteringPolicy);
-    }
-
-    public boolean isDirectoryPackage() {
-      return isDirectoryPackage;
+    if (collectPackagesUnderDirectoryValue.isDirectoryPackage()) {
+      PackageIdentifier packageIdentifier =
+          PackageIdentifier.create(
+              argument.getRecursivePkgKey().getRepository(),
+              argument.getRecursivePkgKey().getRootedPath().getRelativePath());
+      PackageValue pkgValue =
+          (PackageValue)
+              Preconditions.checkNotNull(
+                  env.getValue(PackageValue.key(packageIdentifier)),
+                  collectPackagesUnderDirectoryValue);
+      loadTransitiveTargets(env, pkgValue.getPackage(), filteringPolicy, subdirKeys);
+    } else {
+      env.getValues(subdirKeys);
     }
+    return env.valuesMissing() ? null : PrepareDepsOfTargetsUnderDirectoryValue.INSTANCE;
   }
 
-  private static void loadTransitiveTargets(Environment env, Package pkg,
-      FilteringPolicy filteringPolicy) {
+  // The additionalKeysToRequest argument allows us to batch skyframe dependencies a little more
+  // aggressively. Since the keys computed in this method are independent from any other keys, we
+  // can request our keys together with any other keys that are needed, possibly avoiding a restart.
+  private static void loadTransitiveTargets(
+      Environment env,
+      Package pkg,
+      FilteringPolicy filteringPolicy,
+      Iterable<SkyKey> additionalKeysToRequest) {
     ResolvedTargets<Target> packageTargets =
         TargetPatternResolverUtil.resolvePackageTargets(pkg, filteringPolicy);
     ImmutableList.Builder<SkyKey> builder = ImmutableList.builder();
     for (Target target : packageTargets.getTargets()) {
       builder.add(TransitiveTraversalValue.key(target.getLabel()));
     }
+    builder.addAll(additionalKeysToRequest);
     ImmutableList<SkyKey> skyKeys = builder.build();
     env.getValuesOrThrow(skyKeys, NoSuchPackageException.class, NoSuchTargetException.class);
   }