Make PackageFunction's strategy for handling unreadable BUILD files c…

…onfigurable. Add a test for the current behavior of treating an unreadable BUILD file as a package loading error.

RELNOTES: None
PiperOrigin-RevId: 158314187

src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 
@@ -27,4 +28,8 @@ private BazelSkyframeExecutorConstants() {
 
   public static final ImmutableList<BuildFileName> BUILD_FILES_BY_PRIORITY =
       ImmutableList.of(BuildFileName.BUILD_DOT_BAZEL, BuildFileName.BUILD);
+
+  public static final ActionOnIOExceptionReadingBuildFile
+      ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE =
+          ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE;
 }

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -101,6 +101,8 @@ public class PackageFunction implements SkyFunction {
   // Not final only for testing.
   @Nullable private SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining;
 
+  private final ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile;
+
   static final PathFragment DEFAULTS_PACKAGE_NAME = PathFragment.create("tools/defaults");
 
   public PackageFunction(
@@ -111,7 +113,8 @@ public PackageFunction(
       Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache,
       AtomicInteger numPackagesLoaded,
       @Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining,
-      @Nullable PackageProgressReceiver packageProgress) {
+      @Nullable PackageProgressReceiver packageProgress,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) {
     this.skylarkImportLookupFunctionForInlining = skylarkImportLookupFunctionForInlining;
     // Can be null in tests.
     this.preludeLabel = packageFactory == null
@@ -124,6 +127,7 @@ public PackageFunction(
     this.astCache = astCache;
     this.numPackagesLoaded = numPackagesLoaded;
     this.packageProgress = packageProgress;
+    this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile;
   }
 
   public PackageFunction(
@@ -142,14 +146,54 @@ public PackageFunction(
         astCache,
         numPackagesLoaded,
         skylarkImportLookupFunctionForInlining,
-        null);
+        null,
+        ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE);
   }
 
   public void setSkylarkImportLookupFunctionForInliningForTesting(
       SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining) {
     this.skylarkImportLookupFunctionForInlining = skylarkImportLookupFunctionForInlining;
   }
 
+  /**
+   * What to do when encountering an {@link IOException} trying to read the contents of a BUILD
+   * file.
+   *
+   * <p>Any choice besides
+   * {@link ActionOnIOExceptionReadingBuildFile.UseOriginalIOException#INSTANCE} is potentially
+   * incrementally unsound: if the initial {@link IOException} is transient, then Blaze will
+   * "incorrectly" not attempt to redo package loading for this BUILD file on incremental builds.
+   *
+   * <p>The fact that this behavior is configurable and potentially unsound is a concession to
+   * certain desired use cases with fancy filesystems.
+   */
+  public interface ActionOnIOExceptionReadingBuildFile {
+    /**
+     * Given the {@link IOException} encountered when reading the contents of a BUILD file,
+     * returns the contents that should be used, or {@code null} if the original {@link IOException}
+     * should be respected (that is, we should error-out with a package loading error).
+     */
+    @Nullable
+    byte[] maybeGetBuildFileContentsToUse(IOException originalExn);
+
+    /**
+     * A {@link ActionOnIOExceptionReadingBuildFile} whose {@link #maybeGetBuildFileContentsToUse}
+     * has the sensible behavior of always respecting the initial {@link IOException}.
+     */
+    public static class UseOriginalIOException implements ActionOnIOExceptionReadingBuildFile {
+      public static final UseOriginalIOException INSTANCE = new UseOriginalIOException();
+
+      private UseOriginalIOException() {
+      }
+
+      @Override
+      @Nullable
+      public byte[] maybeGetBuildFileContentsToUse(IOException originalExn) {
+        return null;
+      }
+    }
+  }
+
   /** An entry in {@link PackageFunction}'s internal caches. */
   public static class CacheEntryWithGlobDeps<T> {
     private final T value;
@@ -1142,23 +1186,29 @@ private CacheEntryWithGlobDeps<Package.Builder> loadPackage(
           ParserInputSource input;
           if (replacementContents == null) {
             Preconditions.checkNotNull(buildFileValue, packageId);
+            byte[] buildFileBytes = null;
             try {
-              byte[] buildFileBytes =
+              buildFileBytes =
                   buildFileValue.isSpecialFile()
                       ? FileSystemUtils.readContent(buildFilePath)
                       : FileSystemUtils.readWithKnownFileSize(
                           buildFilePath, buildFileValue.getSize());
-              input =
-                  ParserInputSource.create(
-                      FileSystemUtils.convertFromLatin1(buildFileBytes),
-                      buildFilePath.asFragment());
             } catch (IOException e) {
-              // Note that we did this work, so we should conservatively report this error as
-              // transient.
-              throw new PackageFunctionException(
-                  new BuildFileContainsErrorsException(packageId, e.getMessage(), e),
-                  Transience.TRANSIENT);
+              buildFileBytes =
+                  actionOnIOExceptionReadingBuildFile.maybeGetBuildFileContentsToUse(e);
+              if (buildFileBytes == null) {
+                // Note that we did the work that led to this IOException, so we should
+                // conservatively report this error as transient.
+                throw new PackageFunctionException(new BuildFileContainsErrorsException(
+                    packageId, e.getMessage(), e), Transience.TRANSIENT);
+              }
+              // If control flow reaches here, we're in territory that is deliberately unsound.
+              // See the javadoc for ActionOnIOExceptionReadingBuildFile.
             }
+            input =
+                ParserInputSource.create(
+                    FileSystemUtils.convertFromLatin1(buildFileBytes),
+                    buildFilePath.asFragment());
           } else {
             input = ParserInputSource.create(replacementContents, buildFilePath.asFragment());
           }

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java

@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFilesKnowledge;
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.FileType;
+import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.syntax.SkylarkSemanticsOptions;
@@ -126,7 +127,8 @@ private SequencedSkyframeExecutor(
       PathFragment blacklistedPackagePrefixesFile,
       String productName,
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
-      List<BuildFileName> buildFilesByPriority) {
+      List<BuildFileName> buildFilesByPriority,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) {
     super(
         evaluatorSupplier,
         pkgFactory,
@@ -141,7 +143,8 @@ private SequencedSkyframeExecutor(
         blacklistedPackagePrefixesFile,
         productName,
         crossRepositoryLabelViolationStrategy,
-        buildFilesByPriority);
+        buildFilesByPriority,
+        actionOnIOExceptionReadingBuildFile);
     this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories);
     this.customDirtinessCheckers = customDirtinessCheckers;
   }
@@ -160,7 +163,8 @@ private static SequencedSkyframeExecutor create(
       PathFragment blacklistedPackagePrefixesFile,
       String productName,
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
-      List<BuildFileName> buildFilesByPriority) {
+      List<BuildFileName> buildFilesByPriority,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) {
     SequencedSkyframeExecutor skyframeExecutor =
         new SequencedSkyframeExecutor(
             InMemoryMemoizingEvaluator.SUPPLIER,
@@ -177,7 +181,8 @@ private static SequencedSkyframeExecutor create(
             blacklistedPackagePrefixesFile,
             productName,
             crossRepositoryLabelViolationStrategy,
-            buildFilesByPriority);
+            buildFilesByPriority,
+            actionOnIOExceptionReadingBuildFile);
     skyframeExecutor.init();
     return skyframeExecutor;
   }
@@ -195,7 +200,8 @@ public static SequencedSkyframeExecutor create(
       Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
       String productName,
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
-      List<BuildFileName> buildFilesByPriority) {
+      List<BuildFileName> buildFilesByPriority,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) {
     return create(
         pkgFactory,
         directories,
@@ -210,7 +216,8 @@ public static SequencedSkyframeExecutor create(
         /*blacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT,
         productName,
         crossRepositoryLabelViolationStrategy,
-        buildFilesByPriority);
+        buildFilesByPriority,
+        actionOnIOExceptionReadingBuildFile);
   }
 
   @VisibleForTesting
@@ -239,7 +246,8 @@ public static SequencedSkyframeExecutor createForTesting(
         customDirtinessCheckers,
         productName,
         BazelSkyframeExecutorConstants.CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY,
-        BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY);
+        BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY,
+        BazelSkyframeExecutorConstants.ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE);
   }
 
   @VisibleForTesting
@@ -256,7 +264,8 @@ public static SequencedSkyframeExecutor createForTesting(
       Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
       String productName,
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
-      ImmutableList<BuildFileName> buildFilesByPriority) {
+      ImmutableList<BuildFileName> buildFilesByPriority,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) {
     return create(
         pkgFactory,
         directories,
@@ -271,7 +280,8 @@ public static SequencedSkyframeExecutor createForTesting(
         /*blacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT,
         productName,
         crossRepositoryLabelViolationStrategy,
-        buildFilesByPriority);
+        buildFilesByPriority,
+        actionOnIOExceptionReadingBuildFile);
   }
 
   @VisibleForTesting
@@ -298,7 +308,8 @@ public static SequencedSkyframeExecutor createForTesting(
         blacklistedPackagePrefixesFile,
         productName,
         BazelSkyframeExecutorConstants.CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY,
-        BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY);
+        BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY,
+        BazelSkyframeExecutorConstants.ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java

@@ -56,6 +56,7 @@ public SkyframeExecutor create(
         customDirtinessCheckers,
         productName,
         BazelSkyframeExecutorConstants.CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY,
-        BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY);
+        BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY,
+        BazelSkyframeExecutorConstants.ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE);
   }
 }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -108,6 +108,7 @@
 import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
 import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.FileDirtinessChecker;
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
+import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
 import com.google.devtools.build.lib.skyframe.PackageFunction.CacheEntryWithGlobDeps;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
@@ -284,6 +285,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   private final List<BuildFileName> buildFilesByPriority;
 
+  private final ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile;
+
   private PerBuildSyscallCache perBuildSyscallCache;
   private int lastConcurrencyLevel = -1;
 
@@ -303,7 +306,8 @@ protected SkyframeExecutor(
       PathFragment blacklistedPackagePrefixesFile,
       String productName,
       CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
-      List<BuildFileName> buildFilesByPriority) {
+      List<BuildFileName> buildFilesByPriority,
+      ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) {
     // Strictly speaking, these arguments are not required for initialization, but all current
     // callsites have them at hand, so we might as well set them during construction.
     this.evaluatorSupplier = evaluatorSupplier;
@@ -335,6 +339,7 @@ protected SkyframeExecutor(
     this.productName = productName;
     this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy;
     this.buildFilesByPriority = buildFilesByPriority;
+    this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile;
     this.removeActionsAfterEvaluation.set(false);
   }
 
@@ -472,7 +477,8 @@ protected PackageFunction newPackageFunction(
         astCache,
         numPackagesLoaded,
         null,
-        packageProgress);
+        packageProgress,
+        actionOnIOExceptionReadingBuildFile);
   }
 
   protected SkyFunction newSkylarkImportLookupFunction(

src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java

@@ -50,6 +50,7 @@
 import com.google.devtools.build.lib.skyframe.FileSymlinkInfiniteExpansionUniquenessFunction;
 import com.google.devtools.build.lib.skyframe.GlobFunction;
 import com.google.devtools.build.lib.skyframe.PackageFunction;
+import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
 import com.google.devtools.build.lib.skyframe.PackageFunction.CacheEntryWithGlobDeps;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
@@ -310,6 +311,7 @@ private BuildDriver makeFreshDriver() {
   protected abstract CrossRepositoryLabelViolationStrategy
       getCrossRepositoryLabelViolationStrategy();
   protected abstract ImmutableList<BuildFileName> getBuildFilesByPriority();
+  protected abstract ActionOnIOExceptionReadingBuildFile getActionOnIOExceptionReadingBuildFile();
   protected abstract ImmutableMap<SkyFunctionName, SkyFunction> getExtraExtraSkyFunctions();
 
   protected final ImmutableMap<SkyFunctionName, SkyFunction> makeFreshSkyFunctions() {
@@ -368,7 +370,9 @@ protected final ImmutableMap<SkyFunctionName, SkyFunction> makeFreshSkyFunctions
                 packageFunctionCache,
                 astCache,
                 /*numPackagesLoaded=*/ new AtomicInteger(0),
-                null))
+                /*skylarkImportLookupFunctionForInlining=*/ null,
+                /*packageProgress=*/ null,
+                getActionOnIOExceptionReadingBuildFile()))
         .putAll(extraSkyFunctions)
         .putAll(getExtraExtraSkyFunctions());
     return builder.build();

src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java

@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
 import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants;
 import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction;
+import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
@@ -85,6 +86,11 @@ protected ImmutableList<BuildFileName> getBuildFilesByPriority() {
     return BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY;
   }
 
+  @Override
+  protected ActionOnIOExceptionReadingBuildFile getActionOnIOExceptionReadingBuildFile() {
+    return BazelSkyframeExecutorConstants.ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE;
+  }
+
   @Override
   protected ImmutableMap<SkyFunctionName, SkyFunction> getExtraExtraSkyFunctions() {
     return ImmutableMap.<SkyFunctionName, SkyFunction>of(