Make PackageFunction's strategy for handling unreadable BUILD files configurable. Add a test for the current behavior of treating an unreadable BUILD file as a package loading error. RELNOTES: None PiperOrigin-RevId: 158314187
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java index 9fb17e97..6b892a8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java +++ b/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 @@ 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; }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 1e8a955..994472e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -101,6 +101,8 @@ // 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 @@ 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 @@ this.astCache = astCache; this.numPackagesLoaded = numPackagesLoaded; this.packageProgress = packageProgress; + this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile; } public PackageFunction( @@ -142,7 +146,8 @@ astCache, numPackagesLoaded, skylarkImportLookupFunctionForInlining, - null); + null, + ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE); } public void setSkylarkImportLookupFunctionForInliningForTesting( @@ -150,6 +155,45 @@ 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 @@ 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()); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 26e146b..845429e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/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 @@ PathFragment blacklistedPackagePrefixesFile, String productName, CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy, - List<BuildFileName> buildFilesByPriority) { + List<BuildFileName> buildFilesByPriority, + ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) { super( evaluatorSupplier, pkgFactory, @@ -141,7 +143,8 @@ blacklistedPackagePrefixesFile, productName, crossRepositoryLabelViolationStrategy, - buildFilesByPriority); + buildFilesByPriority, + actionOnIOExceptionReadingBuildFile); this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories); this.customDirtinessCheckers = customDirtinessCheckers; } @@ -160,7 +163,8 @@ PathFragment blacklistedPackagePrefixesFile, String productName, CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy, - List<BuildFileName> buildFilesByPriority) { + List<BuildFileName> buildFilesByPriority, + ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) { SequencedSkyframeExecutor skyframeExecutor = new SequencedSkyframeExecutor( InMemoryMemoizingEvaluator.SUPPLIER, @@ -177,7 +181,8 @@ blacklistedPackagePrefixesFile, productName, crossRepositoryLabelViolationStrategy, - buildFilesByPriority); + buildFilesByPriority, + actionOnIOExceptionReadingBuildFile); skyframeExecutor.init(); return skyframeExecutor; } @@ -195,7 +200,8 @@ Iterable<SkyValueDirtinessChecker> customDirtinessCheckers, String productName, CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy, - List<BuildFileName> buildFilesByPriority) { + List<BuildFileName> buildFilesByPriority, + ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) { return create( pkgFactory, directories, @@ -210,7 +216,8 @@ /*blacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT, productName, crossRepositoryLabelViolationStrategy, - buildFilesByPriority); + buildFilesByPriority, + actionOnIOExceptionReadingBuildFile); } @VisibleForTesting @@ -239,7 +246,8 @@ 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 @@ Iterable<SkyValueDirtinessChecker> customDirtinessCheckers, String productName, CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy, - ImmutableList<BuildFileName> buildFilesByPriority) { + ImmutableList<BuildFileName> buildFilesByPriority, + ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile) { return create( pkgFactory, directories, @@ -271,7 +280,8 @@ /*blacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT, productName, crossRepositoryLabelViolationStrategy, - buildFilesByPriority); + buildFilesByPriority, + actionOnIOExceptionReadingBuildFile); } @VisibleForTesting @@ -298,7 +308,8 @@ 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
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java index b6e91fe..3617c13 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
@@ -56,6 +56,7 @@ 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); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 77d8394..69c4bbc 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/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 @@ private final List<BuildFileName> buildFilesByPriority; + private final ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile; + private PerBuildSyscallCache perBuildSyscallCache; private int lastConcurrencyLevel = -1; @@ -303,7 +306,8 @@ 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 @@ this.productName = productName; this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy; this.buildFilesByPriority = buildFilesByPriority; + this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile; this.removeActionsAfterEvaluation.set(false); } @@ -472,7 +477,8 @@ astCache, numPackagesLoaded, null, - packageProgress); + packageProgress, + actionOnIOExceptionReadingBuildFile); } protected SkyFunction newSkylarkImportLookupFunction(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index 9d5a1b0..00a4a12 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/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 @@ 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 @@ packageFunctionCache, astCache, /*numPackagesLoaded=*/ new AtomicInteger(0), - null)) + /*skylarkImportLookupFunctionForInlining=*/ null, + /*packageProgress=*/ null, + getActionOnIOExceptionReadingBuildFile())) .putAll(extraSkyFunctions) .putAll(getExtraExtraSkyFunctions()); return builder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java index aa88fd5..7c388e5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java +++ b/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; @@ -86,6 +87,11 @@ } @Override + protected ActionOnIOExceptionReadingBuildFile getActionOnIOExceptionReadingBuildFile() { + return BazelSkyframeExecutorConstants.ACTION_ON_IO_EXCEPTION_READING_BUILD_FILE; + } + + @Override protected ImmutableMap<SkyFunctionName, SkyFunction> getExtraExtraSkyFunctions() { return ImmutableMap.<SkyFunctionName, SkyFunction>of( SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction());
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java index da738d6..f9ff08e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.ConstantRuleVisibility; +import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.pkgcache.PackageCacheOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -50,6 +51,7 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.common.options.Options; import java.io.IOException; +import java.io.InputStream; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -662,6 +664,23 @@ } } + @Test + public void testPackageLoadingErrorOnIOExceptionReadingBuildFile() throws Exception { + Path fooBuildFilePath = scratch.file("foo/BUILD"); + IOException exn = new IOException("nope"); + fs.throwExceptionOnGetInputStream(fooBuildFilePath, exn); + + SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo")); + EvaluationResult<PackageValue> result = SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skyKey, /*keepGoing=*/false, reporter); + assertThat(result.hasError()).isTrue(); + ErrorInfo errorInfo = result.getError(skyKey); + String errorMessage = errorInfo.getException().getMessage(); + assertThat(errorMessage).contains("nope"); + assertThat(errorInfo.getException()).isInstanceOf(NoSuchPackageException.class); + assertThat(errorInfo.getException()).hasCauseThat().isSameAs(exn); + } + private static class CustomInMemoryFs extends InMemoryFileSystem { private abstract static class FileStatusOrException { abstract FileStatus get() throws IOException; @@ -696,8 +715,9 @@ } } - private Map<Path, FileStatusOrException> stubbedStats = Maps.newHashMap(); - private Set<Path> makeUnreadableAfterReaddir = Sets.newHashSet(); + private final Map<Path, FileStatusOrException> stubbedStats = Maps.newHashMap(); + private final Set<Path> makeUnreadableAfterReaddir = Sets.newHashSet(); + private final Map<Path, IOException> pathsToErrorOnGetInputStream = Maps.newHashMap(); public CustomInMemoryFs(ManualClock manualClock) { super(manualClock); @@ -731,5 +751,18 @@ } return result; } + + public void throwExceptionOnGetInputStream(Path path, IOException exn) { + pathsToErrorOnGetInputStream.put(path, exn); + } + + @Override + protected InputStream getInputStream(Path path) throws IOException { + IOException exnToThrow = pathsToErrorOnGetInputStream.get(path); + if (exnToThrow != null) { + throw exnToThrow; + } + return super.getInputStream(path); + } } }