Rollback of commit 0c7a42a09d85ddffd9b860bcb31e4c43a00632c1.

*** Reason for rollback ***

[]

*** Original change description ***

Slight refactor of ExternalFilesHelper:
-Make FileType and ExternalFileAction public.
-Have producers use ExternalFileAction, rather than a boolean, to specify the desired behavior.

And a big change in semantics (doesn't affect Bazel):
-Replace ExternalFileAction.ERROR_OUT with ExternalFileAction.ASSUME_NON_EXISTENT_AND_IMMUTABLE, which does what it sounds like. This new action, like the old ERROR_OUT, is _not_ used in Bazel.

--
MOS_MIGRATED_REVID=136206810

1 files changed, 40 insertions, 53 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 e757c27574..664cf9888a 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
@@ -26,7 +26,7 @@ import com.google.devtools.build.skyframe.SkyFunction;
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicReference;
 
-/** Common utilities for dealing with paths outside the package roots. */
+/** Common utilities for dealing with files outside the package roots. */
 public class ExternalFilesHelper {
   private final AtomicReference<PathPackageLocator> pkgLocator;
   private final ExternalFileAction externalFileAction;
@@ -37,8 +37,23 @@ public class ExternalFilesHelper {
   private boolean anyOutputFilesSeen = false;
   private boolean anyNonOutputExternalFilesSeen = false;
 
+  /**
+   * @param pkgLocator an {@link AtomicReference} to a {@link PathPackageLocator} used to
+   *    determine what files are internal.
+   * @param errorOnExternalFiles If files outside of package paths should be allowed.
+   */
   public ExternalFilesHelper(
-      AtomicReference<PathPackageLocator> pkgLocator,
+      AtomicReference<PathPackageLocator> pkgLocator, boolean errorOnExternalFiles,
+      BlazeDirectories directories) {
+    this(
+        pkgLocator,
+        errorOnExternalFiles
+            ? ExternalFileAction.ERROR_OUT
+            : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
+        directories);
+  }
+
+  private ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator,
       ExternalFileAction externalFileAction,
       BlazeDirectories directories) {
     this.pkgLocator = pkgLocator;
@@ -46,42 +61,25 @@ public class ExternalFilesHelper {
     this.directories = directories;
   }
 
-  /**
-   * The action to take when an external path is encountered. See {@link FileType} for the
-   * definition of "external".
-   */
-  public enum ExternalFileAction {
-    /**
-     * For paths of type {@link FileType#EXTERNAL_REPO}, introduce a Skyframe dependency on the
-     * 'external' package.
-     *
-     * <p>This is the default for Bazel, since it's required for correctness of the external
-     * repositories feature.
-     */
-    DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
+  private enum ExternalFileAction {
+    /** Re-check the files when the WORKSPACE file changes. */
+    DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
 
-    /**
-     * For paths of type {@link FileType#EXTERNAL} or {@link FileType#OUTPUT}, assume the path does
-     * not exist and will never exist.
-     */
-    ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS,
+    /** Throw an exception if there is an external file. */
+    ERROR_OUT,
   }
 
-  /** Classification of a path encountered by Bazel. */
-  public enum FileType {
-    /** A path inside the package roots or in an external repository. */
+  enum FileType {
+    /** A file inside the package roots or in an external repository. */
     INTERNAL,
 
-    /**
-     * A non {@link #EXTERNAL_REPO} path outside the package roots about which we may make no other
-     * assumptions.
-     */
-    EXTERNAL,
+    /** A file outside the package roots about which we may make no other assumptions. */
+    EXTERNAL_MUTABLE,
 
     /**
-     * A path 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 Bazel flags may tell
-     * Bazel to assume these paths are immutable.
+     * 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.
      *
      * <p>Note that {@link ExternalFilesHelper#maybeHandleExternalFile} is only used for {@link
      * FileStateValue} and {@link DirectoryListingStateValue}, and also note that output files do
@@ -95,19 +93,12 @@ public class ExternalFilesHelper {
     OUTPUT,
 
     /**
-     * A path in the part of Bazel's output tree that contains (/ symlinks to) to external
+     * A file in the part of Bazel's output tree that contains (/ symlinks to) to external
      * repositories.
      */
     EXTERNAL_REPO,
   }
 
-  /**
-   * Thrown by {@link #maybeHandleExternalFile} when an applicable path is processed (see
-   * {@link ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}.
-   */
-  static class NonexistentImmutableExternalFileException extends Exception {
-  }
-
   static class ExternalFilesKnowledge {
     final boolean anyOutputFilesSeen;
     final boolean anyNonOutputExternalFilesSeen;
@@ -143,7 +134,7 @@ public class ExternalFilesHelper {
     Path outputBase = packageLocator.getOutputBase();
     if (outputBase == null) {
       anyNonOutputExternalFilesSeen = true;
-      return FileType.EXTERNAL;
+      return FileType.EXTERNAL_MUTABLE;
     }
     if (rootedPath.asPath().startsWith(outputBase)) {
       Path externalRepoDir = outputBase.getRelative(Label.EXTERNAL_PACKAGE_NAME);
@@ -156,33 +147,29 @@ public class ExternalFilesHelper {
       }
     }
     anyNonOutputExternalFilesSeen = true;
-    return FileType.EXTERNAL;
+    return FileType.EXTERNAL_MUTABLE;
   }
 
   /**
-   * If this instance is configured with
-   * {@link ExternalFileAction#DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS} and
-   * {@code rootedPath} isn't under a package root then this adds a dependency on the //external
-   * package. If the action is
-   * {@link ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}, it will throw
-   * a {@link NonexistentImmutableExternalFileException} instead.
+   * If this instance is configured with DEPEND_ON_EXTERNAL_PKG and rootedPath is a file that isn't
+   * 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
   void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
-      throws NonexistentImmutableExternalFileException, IOException, InterruptedException {
+      throws IOException, InterruptedException {
     FileType fileType = getAndNoteFileType(rootedPath);
     if (fileType == FileType.INTERNAL) {
       return;
     }
-    if (fileType == FileType.OUTPUT || fileType == FileType.EXTERNAL) {
-      if (externalFileAction
-          == ExternalFileAction.ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS) {
-        throw new NonexistentImmutableExternalFileException();
+    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_PATHS,
+        externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
         externalFileAction);
     RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env);
   }