Respect --noexperimental_check_output_files in FileSystemValueChecker. FileStateValues for output files can make their way into the Skyframe graph if a source file is symlink to an output file.

Also fix a bug where ExternalFilesHelper#isExternalFileSeen would always return true after returning true once in the past. This meant if an external file ever made its way into the Skyframe graph, we would always do a full graph scan at the beginning of each build (iow, we would always waste some CPU time doing nothing interesting).

--
MOS_MIGRATED_REVID=118190190

1 files changed, 108 insertions, 33 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
index 6126197456..d55a0297f4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
@@ -14,8 +14,11 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
+import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -28,9 +31,10 @@ public class ExternalFilesHelper {
   private final AtomicReference<PathPackageLocator> pkgLocator;
   private final ExternalFileAction externalFileAction;
 
-  // This variable is set to true from multiple threads, but only read once, in the main thread.
+  // These variables are set to true from multiple threads, but only read in the main thread.
   // So volatility or an AtomicBoolean is not needed.
-  private boolean externalFileSeen = false;
+  private boolean anyOutputFilesSeen = false;
+  private boolean anyNonOutputExternalFilesSeen = false;
 
   /**
    * @param pkgLocator an {@link AtomicReference} to a {@link PathPackageLocator} used to
@@ -39,32 +43,106 @@ public class ExternalFilesHelper {
    */
   public ExternalFilesHelper(
       AtomicReference<PathPackageLocator> pkgLocator, boolean errorOnExternalFiles) {
+    this(
+        pkgLocator,
+        errorOnExternalFiles
+            ? ExternalFileAction.ERROR_OUT
+            : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES);
+  }
+
+  private ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator,
+      ExternalFileAction externalFileAction) {
     this.pkgLocator = pkgLocator;
-    if (errorOnExternalFiles) {
-      this.externalFileAction = ExternalFileAction.ERROR_OUT;
-    } else {
-      this.externalFileAction = ExternalFileAction.DEPEND_ON_EXTERNAL_PKG;
-    }
+    this.externalFileAction = externalFileAction;
   }
 
   private enum ExternalFileAction {
-    // Re-check the files when the WORKSPACE file changes.
-    DEPEND_ON_EXTERNAL_PKG,
+    /** Re-check the files when the WORKSPACE file changes. */
+    DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
 
-    // Throw an exception if there is an external file.
+    /** Throw an exception if there is an external file. */
     ERROR_OUT,
   }
 
-  boolean isExternalFileSeen() {
-    return externalFileSeen;
+  enum FileType {
+    /** A file inside the package roots or in an external repository. */
+    INTERNAL,
+
+    /** A file outside the package roots about which we may make no other assumptions. */
+    EXTERNAL_MUTABLE,
+
+    /**
+     * A file in Bazel's output tree that's a proper output of an action (*not* a source file in an
+     * external repository). Such files are theoretically mutable, but certain Blaze flags may tell
+     * Blaze to assume these files are immutable.
+     *
+     * Note that {@link ExternalFilesHelper#maybeHandleExternalFile} is only used for
+     * {@link FileStateValue} and {@link DirectoryStateValue}, and also note that output files do
+     * not normally have corresponding {@link FileValue} instances (and thus also
+     * {@link FileStateValue} instances) in the Skyframe graph ({@link ArtifactFunction} only uses
+     * {@link FileValue}s for source files). But {@link FileStateValue}s for output files can still
+     * make their way into the Skyframe graph if e.g. a source file is a symlink to an output file.
+     */
+    // TODO(nharmata): Consider an alternative design where we have an OutputFileDiffAwareness. This
+    // could work but would first require that we clean up all RootedPath usage.
+    OUTPUT,
+
+    /**
+     * A file in the part of Bazel's output tree that contains (/ symlinks to) to external
+     * repositories.
+     */
+    EXTERNAL_REPO,
+  }
+
+  static class ExternalFilesKnowledge {
+    final boolean anyOutputFilesSeen;
+    final boolean anyNonOutputExternalFilesSeen;
+
+    private ExternalFilesKnowledge(boolean anyOutputFilesSeen,
+        boolean anyNonOutputExternalFilesSeen) {
+      this.anyOutputFilesSeen = anyOutputFilesSeen;
+      this.anyNonOutputExternalFilesSeen = anyNonOutputExternalFilesSeen;
+    }
+  }
+
+  @ThreadCompatible
+  ExternalFilesKnowledge getExternalFilesKnowledge() {
+    return new ExternalFilesKnowledge(anyOutputFilesSeen, anyNonOutputExternalFilesSeen);
+  }
+
+  @ThreadCompatible
+  void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) {
+    anyOutputFilesSeen = externalFilesKnowledge.anyOutputFilesSeen;
+    anyNonOutputExternalFilesSeen = externalFilesKnowledge.anyNonOutputExternalFilesSeen;
   }
 
-  static boolean isInternal(RootedPath rootedPath, PathPackageLocator packageLocator) {
-    // TODO(bazel-team): This is inefficient when there are a lot of package roots or there are a
-    // lot of external directories. Consider either explicitly preventing this case or using a more
-    // efficient approach here (e.g. use a trie for determining if a file is under an external
-    // directory).
-    return packageLocator.getPathEntries().contains(rootedPath.getRoot());
+  ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
+    return new ExternalFilesHelper(pkgLocator, externalFileAction);
+  }
+
+  FileType getAndNoteFileType(RootedPath rootedPath) {
+    PathPackageLocator packageLocator = pkgLocator.get();
+    if (packageLocator.getPathEntries().contains(rootedPath.getRoot())) {
+      return FileType.INTERNAL;
+    }
+    // The outputBase may be null if we're not actually running a build.
+    Path outputBase = packageLocator.getOutputBase();
+    if (outputBase == null) {
+      anyNonOutputExternalFilesSeen = true;
+      return FileType.EXTERNAL_MUTABLE;
+    }
+    if (rootedPath.asPath().startsWith(outputBase)) {
+      Path externalRepoDir = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX);
+      if (rootedPath.asPath().startsWith(externalRepoDir)) {
+        anyNonOutputExternalFilesSeen = true;
+        return FileType.EXTERNAL_REPO;
+      } else {
+        anyOutputFilesSeen = true;
+        return FileType.OUTPUT;
+      }
+    }
+    anyNonOutputExternalFilesSeen = true;
+    return FileType.EXTERNAL_MUTABLE;
   }
 
   /**
@@ -72,26 +150,22 @@ public class ExternalFilesHelper {
    * under a package root then this adds a dependency on the //external package. If the action is
    * ERROR_OUT, it will throw an error instead.
    */
+  @ThreadSafe
   public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
       throws FileOutsidePackageRootsException {
-    if (isInternal(rootedPath, pkgLocator.get())) {
-      return;
-    }
-
-    externalFileSeen = true;
-    if (externalFileAction == ExternalFileAction.ERROR_OUT) {
-      throw new FileOutsidePackageRootsException(rootedPath);
-    }
-
-    // The outputBase may be null if we're not actually running a build.
-    Path outputBase = pkgLocator.get().getOutputBase();
-    if (outputBase == null) {
+    FileType fileType = getAndNoteFileType(rootedPath);
+    if (fileType == FileType.INTERNAL) {
       return;
     }
-    Path relativeExternal = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX);
-    if (!rootedPath.asPath().startsWith(relativeExternal)) {
+    if (fileType == FileType.OUTPUT || fileType == FileType.EXTERNAL_MUTABLE) {
+      if (externalFileAction == ExternalFileAction.ERROR_OUT) {
+        throw new FileOutsidePackageRootsException(rootedPath);
+      }
       return;
     }
+    Preconditions.checkState(
+        externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
+        externalFileAction);
 
     // For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding rule
     // so that if the WORKSPACE file changes, the File/DirectoryStateValue will be re-evaluated.
@@ -105,7 +179,8 @@ public class ExternalFilesHelper {
     // neither want to encourage nor optimize for since it is not common. So the set of external
     // files is small.
 
-    PathFragment repositoryPath = rootedPath.asPath().relativeTo(relativeExternal);
+    Path externalRepoDir = pkgLocator.get().getOutputBase().getRelative(Label.EXTERNAL_PATH_PREFIX);
+    PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir);
     if (repositoryPath.segmentCount() == 0) {
       // We are the top of the repository path (<outputBase>/external), not in an actual external
       // repository path.