Fix Fileset incrementality bug when Fileset consumes a generated file. The native skyframe implementation was actually quite incorrect in this case: It was adding a skyframe dependency on a FileValue for the output file. Without a transitive dependency on the source files and actions that determine the output file's state, this could never work (and explains why the incremental build would fail). Instead, we now depend on the Artifact corresponding to the output file instead.

This change updates the business logic RecursiveFilesystemTraversalFunction. This approach keeps the business logic of Fileset filesystem traversal centralized in RFTF.

To avoid making weird recursive Skyframe nodes in the output tree, we inline Skyframe dependencies and do direct filesystem operations over the output tree.

There are now three states we can be in when looking up a file:
1. Source file: As before, make a skyframe dep on the corresponding file
2. Top-level file of an output tree: Make a dep on the corresponding Artifact
3. Recursive file under an output directory: Do direct filesystem operations. It doesn't make sense to make Skyframe nodes corresponding to these files. In the future, I think we should consider failing fast on this case.

RELNOTES: None
PiperOrigin-RevId: 184556044

1 files changed, 15 insertions, 12 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
index 247e546335..7a1a269808 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
@@ -17,6 +17,7 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Objects;
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot;
 import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -53,8 +54,7 @@ import javax.annotation.Nullable;
  */
 public final class RecursiveFilesystemTraversalValue implements SkyValue {
   static final RecursiveFilesystemTraversalValue EMPTY = new RecursiveFilesystemTraversalValue(
-      Optional.<ResolvedFile>absent(),
-      NestedSetBuilder.<ResolvedFile>emptySet(Order.STABLE_ORDER));
+      Optional.absent(), NestedSetBuilder.emptySet(Order.STABLE_ORDER));
 
   /** The root of the traversal. May only be absent for the {@link #EMPTY} instance. */
   private final Optional<ResolvedFile> resolvedRoot;
@@ -110,7 +110,7 @@ public final class RecursiveFilesystemTraversalValue implements SkyValue {
   public static final class TraversalRequest {
 
     /** The path to start the traversal from; may be a file, a directory or a symlink. */
-    final RootedPath path;
+    final DirectTraversalRoot root;
 
     /**
      * Whether the path is in the output tree.
@@ -135,24 +135,25 @@ public final class RecursiveFilesystemTraversalValue implements SkyValue {
     /** Information to be attached to any error messages that may be reported. */
     @Nullable final String errorInfo;
 
-    public TraversalRequest(RootedPath path, boolean isRootGenerated,
+    public TraversalRequest(DirectTraversalRoot root, boolean isRootGenerated,
         PackageBoundaryMode crossPkgBoundaries, boolean skipTestingForSubpackage,
         @Nullable String errorInfo) {
-      this.path = path;
+      this.root = root;
       this.isGenerated = isRootGenerated;
       this.crossPkgBoundaries = crossPkgBoundaries;
       this.skipTestingForSubpackage = skipTestingForSubpackage;
       this.errorInfo = errorInfo;
     }
 
-    private TraversalRequest duplicate(RootedPath newRoot, boolean newSkipTestingForSubpackage) {
+    private TraversalRequest duplicate(DirectTraversalRoot newRoot,
+        boolean newSkipTestingForSubpackage) {
       return new TraversalRequest(newRoot, isGenerated, crossPkgBoundaries,
           newSkipTestingForSubpackage, errorInfo);
     }
 
     /** Creates a new request to traverse a child element in the current directory (the root). */
     TraversalRequest forChildEntry(RootedPath newPath) {
-      return duplicate(newPath, false);
+      return duplicate(DirectTraversalRoot.forRootedPath(newPath), false);
     }
 
     /**
@@ -163,7 +164,9 @@ public final class RecursiveFilesystemTraversalValue implements SkyValue {
      */
     TraversalRequest forChangedRootPath(Root newRoot) {
       return duplicate(
-          RootedPath.toRootedPath(newRoot, path.getRootRelativePath()), skipTestingForSubpackage);
+          DirectTraversalRoot.forRootedPath(
+              RootedPath.toRootedPath(newRoot, root.asRootedPath().getRootRelativePath())),
+              skipTestingForSubpackage);
     }
 
     @Override
@@ -175,21 +178,21 @@ public final class RecursiveFilesystemTraversalValue implements SkyValue {
         return false;
       }
       TraversalRequest o = (TraversalRequest) obj;
-      return path.equals(o.path) && isGenerated == o.isGenerated
+      return root.equals(o.root) && isGenerated == o.isGenerated
           && crossPkgBoundaries == o.crossPkgBoundaries
           && skipTestingForSubpackage == o.skipTestingForSubpackage;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hashCode(path, isGenerated, crossPkgBoundaries, skipTestingForSubpackage);
+      return Objects.hashCode(root, isGenerated, crossPkgBoundaries, skipTestingForSubpackage);
     }
 
     @Override
     public String toString() {
       return String.format(
           "TraversalParams(root=%s, is_generated=%d, skip_testing_for_subpkg=%d,"
-          + " pkg_boundaries=%s)", path, isGenerated ? 1 : 0,
+          + " pkg_boundaries=%s)", root, isGenerated ? 1 : 0,
           skipTestingForSubpackage ? 1 : 0, crossPkgBoundaries);
     }
   }
@@ -660,7 +663,7 @@ public final class RecursiveFilesystemTraversalValue implements SkyValue {
    * <p>The object stores things such as the absolute path of the file or symlink, its exact type
    * and, if it's a symlink, the resolved and unresolved link target paths.
    */
-  public static interface ResolvedFile {
+  public interface ResolvedFile {
     /** Type of the entity under {@link #getPath()}. */
     FileType getType();