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, 177 insertions, 37 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
index f6bdfc69f8..e46776d94d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
@@ -17,6 +17,7 @@ import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
 import com.google.common.collect.Collections2;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.events.Event;
@@ -24,9 +25,13 @@ import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.
 import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFileFactory;
 import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.TraversalRequest;
 import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -34,6 +39,8 @@ import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -58,7 +65,7 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
           String.format(
               "Generated directory %s conflicts with package under the same path. "
                   + "Additional info: %s",
-              traversal.path.getRootRelativePath().getPathString(),
+              traversal.root.asRootedPath().getRootRelativePath().getPathString(),
               traversal.errorInfo != null ? traversal.errorInfo : traversal.toString()));
     }
   }
@@ -126,14 +133,14 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
       if (!rootInfo.type.exists()) {
         // May be a dangling symlink or a non-existent file. Handle gracefully.
         if (rootInfo.type.isSymlink()) {
-          return resultForDanglingSymlink(traversal.path, rootInfo);
+          return resultForDanglingSymlink(traversal.root.asRootedPath(), rootInfo);
         } else {
           return RecursiveFilesystemTraversalValue.EMPTY;
         }
       }
 
       if (rootInfo.type.isFile()) {
-        return resultForFileRoot(traversal.path, rootInfo);
+        return resultForFileRoot(traversal.root.asRootedPath(), rootInfo);
       }
 
       // Otherwise the root is a directory or a symlink to one.
@@ -150,7 +157,7 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
         String msg =
             traversal.errorInfo
                 + " crosses package boundary into package rooted at "
-                + traversal.path.getRootRelativePath().getPathString();
+                + traversal.root.asRootedPath().getRootRelativePath().getPathString();
         switch (traversal.crossPkgBoundaries) {
           case CROSS:
             // We are free to traverse the subpackage but we need to display a warning.
@@ -170,7 +177,8 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
 
       // We are free to traverse this directory.
       Collection<SkyKey> dependentKeys = createRecursiveTraversalKeys(env, traversal);
-      return resultForDirectory(traversal, rootInfo, traverseChildren(env, dependentKeys));
+      return resultForDirectory(traversal, rootInfo,
+          traverseChildren(env, dependentKeys, /*inline=*/traversal.isGenerated));
     } catch (IOException e) {
       throw new RecursiveFilesystemTraversalFunctionException(
           new FileOperationException("Error while traversing fileset: " + e.getMessage()));
@@ -211,31 +219,82 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
 
   private static FileInfo lookUpFileInfo(Environment env, TraversalRequest traversal)
       throws MissingDepException, IOException, InterruptedException {
-    // Stat the file.
-    FileValue fileValue =
-        (FileValue) env.getValueOrThrow(FileValue.key(traversal.path), IOException.class);
+    if (traversal.isGenerated) {
+      byte[] digest = null;
+      if (traversal.root.getOutputArtifact() != null) {
+        Artifact artifact = traversal.root.getOutputArtifact();
+        SkyKey artifactKey = ArtifactSkyKey.key(artifact, true);
+        SkyValue value = env.getValue(artifactKey);
+        if (env.valuesMissing()) {
+          throw new MissingDepException();
+        }
 
-    if (env.valuesMissing()) {
-      throw new MissingDepException();
-    }
-    if (fileValue.exists()) {
-      // If it exists, it may either be a symlink or a file/directory.
+        if (value instanceof FileArtifactValue) {
+          FileArtifactValue fsVal = (FileArtifactValue) value;
+          digest = fsVal.getDigest();
+        } else {
+          return new FileInfo(
+              FileType.NONEXISTENT, null, null, null);
+        }
+      }
+
+      // FileArtifactValue does not currently track symlinks. If it did, we could potentially remove
+      // some of the filesystem operations we're doing here.
+      Path path = traversal.root.asRootedPath().asPath();
+      FileStatus noFollowStat = path.stat(Symlinks.NOFOLLOW);
+      FileStatus followStat = path.statIfFound(Symlinks.FOLLOW);
+      FileType type;
       PathFragment unresolvedLinkTarget = null;
-      FileType type = null;
-      if (fileValue.isSymlink()) {
-        unresolvedLinkTarget = fileValue.getUnresolvedLinkTarget();
-        type = fileValue.isDirectory() ? FileType.SYMLINK_TO_DIRECTORY : FileType.SYMLINK_TO_FILE;
+      RootedPath realPath = traversal.root.asRootedPath();
+      if (followStat == null) {
+        type = FileType.DANGLING_SYMLINK;
+        if (!noFollowStat.isSymbolicLink()) {
+          throw new IOException("Expected symlink for " + path + ", but got: " + noFollowStat);
+        }
+        unresolvedLinkTarget = path.readSymbolicLink();
+      } else if (noFollowStat.isFile()) {
+        type = FileType.FILE;
+      } else if (noFollowStat.isDirectory()) {
+        type = FileType.DIRECTORY;
       } else {
-        type = fileValue.isDirectory() ? FileType.DIRECTORY : FileType.FILE;
+        unresolvedLinkTarget = path.readSymbolicLink();
+        realPath = RootedPath.toRootedPath(
+            Root.absoluteRoot(path.getFileSystem()),
+            path.resolveSymbolicLinks());
+        type = followStat.isFile() ? FileType.SYMLINK_TO_FILE : FileType.SYMLINK_TO_DIRECTORY;
       }
-      return new FileInfo(type, fileValue.realFileStateValue(),
-          fileValue.realRootedPath(), unresolvedLinkTarget);
+      return new FileInfo(type,
+          FileStateValue.createWithStatNoFollow(traversal.root.asRootedPath(),
+              new StatWithDigest(noFollowStat, digest), null),
+          realPath, unresolvedLinkTarget);
     } else {
-      // If it doesn't exist, or it's a dangling symlink, we still want to handle that gracefully.
-      return new FileInfo(
-          fileValue.isSymlink() ? FileType.DANGLING_SYMLINK : FileType.NONEXISTENT,
-          fileValue.realFileStateValue(), null,
-          fileValue.isSymlink() ? fileValue.getUnresolvedLinkTarget() : null);
+      // Stat the file.
+      FileValue fileValue =
+          (FileValue) env.getValueOrThrow(
+              FileValue.key(traversal.root.asRootedPath()), IOException.class);
+
+      if (env.valuesMissing()) {
+        throw new MissingDepException();
+      }
+      if (fileValue.exists()) {
+        // If it exists, it may either be a symlink or a file/directory.
+        PathFragment unresolvedLinkTarget = null;
+        FileType type;
+        if (fileValue.isSymlink()) {
+          unresolvedLinkTarget = fileValue.getUnresolvedLinkTarget();
+          type = fileValue.isDirectory() ? FileType.SYMLINK_TO_DIRECTORY : FileType.SYMLINK_TO_FILE;
+        } else {
+          type = fileValue.isDirectory() ? FileType.DIRECTORY : FileType.FILE;
+        }
+        return new FileInfo(type, fileValue.realFileStateValue(),
+            fileValue.realRootedPath(), unresolvedLinkTarget);
+      } else {
+        // If it doesn't exist, or it's a dangling symlink, we still want to handle that gracefully.
+        return new FileInfo(
+            fileValue.isSymlink() ? FileType.DANGLING_SYMLINK : FileType.NONEXISTENT,
+            fileValue.realFileStateValue(), null,
+            fileValue.isSymlink() ? fileValue.getUnresolvedLinkTarget() : null);
+      }
     }
   }
 
@@ -297,7 +356,8 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
         "{%s} {%s}", traversal, rootInfo);
     PackageLookupValue pkgLookup =
         (PackageLookupValue)
-            getDependentSkyValue(env, PackageLookupValue.key(traversal.path.getRootRelativePath()));
+            getDependentSkyValue(env,
+                PackageLookupValue.key(traversal.root.asRootedPath().getRootRelativePath()));
 
     if (pkgLookup.packageExists()) {
       if (traversal.isGenerated) {
@@ -307,7 +367,7 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
       } else {
         // The traversal's root was a source directory and it defines a package.
         Root pkgRoot = pkgLookup.getRoot();
-        if (!pkgRoot.equals(traversal.path.getRoot())) {
+        if (!pkgRoot.equals(traversal.root.asRootedPath().getRoot())) {
           // However the root of this package is different from what we expected. stat() the real
           // BUILD file of that package.
           traversal = traversal.forChangedRootPath(pkgRoot);
@@ -330,18 +390,29 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
    */
   private static Collection<SkyKey> createRecursiveTraversalKeys(
       Environment env, TraversalRequest traversal)
-      throws MissingDepException, InterruptedException {
+      throws MissingDepException, InterruptedException, IOException {
     // Use the traversal's path, even if it's a symlink. The contents of the directory, as listed
     // in the result, must be relative to it.
-    DirectoryListingValue dirListing = (DirectoryListingValue) getDependentSkyValue(env,
-        DirectoryListingValue.key(traversal.path));
+    Iterable<Dirent> dirents;
+    if (traversal.isGenerated) {
+      // If we're dealing with an output file, read the directory directly instead of creating
+      // filesystem nodes under the output tree.
+      List<Dirent> direntsCollection =
+          new ArrayList<>(
+              traversal.root.asRootedPath().asPath().readdir(Symlinks.FOLLOW));
+      Collections.sort(direntsCollection);
+      dirents = direntsCollection;
+    } else {
+      dirents = ((DirectoryListingValue) getDependentSkyValue(env,
+          DirectoryListingValue.key(traversal.root.asRootedPath()))).getDirents();
+    }
 
     List<SkyKey> result = new ArrayList<>();
-    for (Dirent dirent : dirListing.getDirents()) {
+    for (Dirent dirent : dirents) {
       RootedPath childPath =
           RootedPath.toRootedPath(
-              traversal.path.getRoot(),
-              traversal.path.getRootRelativePath().getRelative(dirent.getName()));
+              traversal.root.asRootedPath().getRoot(),
+              traversal.root.asRootedPath().getRootRelativePath().getRelative(dirent.getName()));
       TraversalRequest childTraversal = traversal.forChildEntry(childPath);
       result.add(RecursiveFilesystemTraversalValue.key(childTraversal));
     }
@@ -395,7 +466,7 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
       root =
           ResolvedFileFactory.symlinkToDirectory(
               rootInfo.realPath,
-              traversal.path,
+              traversal.root.asRootedPath(),
               rootInfo.unresolvedSymlinkTarget,
               hashDirectorySymlink(children, rootInfo.metadata.hashCode()));
       paths = NestedSetBuilder.<ResolvedFile>stableOrder().addTransitive(children).add(root);
@@ -434,12 +505,25 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
    *
    * <p>The keys must all be {@link SkyFunctions#RECURSIVE_FILESYSTEM_TRAVERSAL} keys.
    */
-  private static Collection<RecursiveFilesystemTraversalValue> traverseChildren(
-      Environment env, Iterable<SkyKey> keys) throws MissingDepException, InterruptedException {
-    Map<SkyKey, SkyValue> values = env.getValues(keys);
+  private Collection<RecursiveFilesystemTraversalValue> traverseChildren(
+      Environment env, Iterable<SkyKey> keys, boolean inline)
+      throws MissingDepException, InterruptedException,
+      RecursiveFilesystemTraversalFunctionException {
+    Map<SkyKey, SkyValue> values;
+    if (inline) {
+      // Don't create Skyframe nodes for a recursive traversal over the output tree.
+      // Instead, inline the recursion in the top-level request.
+      values = new HashMap<>();
+      for (SkyKey depKey : keys) {
+        values.put(depKey, compute(depKey, env));
+      }
+    } else {
+      values = env.getValues(keys);
+    }
     if (env.valuesMissing()) {
       throw new MissingDepException();
     }
+
     return Collections2.transform(values.values(),
         new Function<SkyValue, RecursiveFilesystemTraversalValue>() {
           @Override
@@ -505,4 +589,60 @@ public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
     boolean exists() { return false; }
     @Override public abstract String toString();
   }
+
+  private static class StatWithDigest implements FileStatusWithDigest {
+    private final FileStatus noFollowStat;
+    private final byte[] digest;
+
+    public StatWithDigest(FileStatus noFollowStat, byte[] digest) {
+      this.noFollowStat = noFollowStat;
+      this.digest = digest;
+    }
+
+    @Nullable
+    @Override
+    public byte[] getDigest() throws IOException {
+      return digest;
+    }
+
+    @Override
+    public boolean isFile() {
+      return noFollowStat.isFile();
+    }
+
+    @Override
+    public boolean isDirectory() {
+      return noFollowStat.isDirectory();
+    }
+
+    @Override
+    public boolean isSymbolicLink() {
+      return noFollowStat.isSymbolicLink();
+    }
+
+    @Override
+    public boolean isSpecialFile() {
+      return noFollowStat.isSpecialFile();
+    }
+
+    @Override
+    public long getSize() throws IOException {
+      return noFollowStat.getSize();
+    }
+
+    @Override
+    public long getLastModifiedTime() throws IOException {
+      return noFollowStat.getLastModifiedTime();
+    }
+
+    @Override
+    public long getLastChangeTime() throws IOException {
+      return noFollowStat.getLastChangeTime();
+    }
+
+    @Override
+    public long getNodeId() throws IOException {
+      return noFollowStat.getNodeId();
+    }
+  }
 }