Replace the per-`SkyKey` caches used by `PackageFunction` with the new `SkyKeyComputeState` mechanism.
The new mechanism is just as good but doesn't have the downsides of the explicit per-`SkyKey` caches. See the "Background and motivation" section of the description of https://github.com/bazelbuild/bazel/commit/ed279ab4fa2d4356be00b54266f56edcc5aeae78.
This CL is expected to be performance neutral in all situations.
N.B. `PackageFunctionTest#testDiscrepancyBetweenGlobbingErrors` was ironically [very subtly] making use of the fact that the entry inside of `packageFunctionCache` for a `PackageValue` node with a dep in error does *not* get cleared inside of a `--nokeep_going` evaluation. Since this CL changes that behavior I had to change the test case to use a `--keep_going` evaluation in order to exercise the situation it wants to exercise.
PiperOrigin-RevId: 415564015
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 74ae317..9d24aa5 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
@@ -16,7 +16,6 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
-import com.github.benmanes.caffeine.cache.Cache;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -107,8 +106,6 @@
private final PackageFactory packageFactory;
private final CachingPackageLocator packageLocator;
- private final Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache;
- private final Cache<PackageIdentifier, CompiledBuildFile> compiledBuildFileCache;
private final AtomicBoolean showLoadingProgress;
private final AtomicInteger numPackagesLoaded;
@Nullable private final PackageProgressReceiver packageProgress;
@@ -128,9 +125,7 @@
* call site to its {@code generator_name} attribute value.
*/
// TODO(adonovan): when we split PackageCompileFunction out, move this there, and make it
- // non-public. The only reason it is public is because various places want to construct a
- // cache of them, but that hack can go away when it's a first-class Skyframe function.
- // (Since CompiledBuildFile contains a Module (the prelude), when we split it out,
+ // non-public. (Since CompiledBuildFile contains a Module (the prelude), when we split it out,
// the code path that requests it will have to support inlining a la BzlLoadFunction.)
public static class CompiledBuildFile {
// Either errors is null, or all the other fields are.
@@ -175,8 +170,6 @@
PackageFactory packageFactory,
CachingPackageLocator pkgLocator,
AtomicBoolean showLoadingProgress,
- Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache,
- Cache<PackageIdentifier, CompiledBuildFile> compiledBuildFileCache,
AtomicInteger numPackagesLoaded,
@Nullable BzlLoadFunction bzlLoadFunctionForInlining,
@Nullable PackageProgressReceiver packageProgress,
@@ -187,8 +180,6 @@
this.packageFactory = packageFactory;
this.packageLocator = pkgLocator;
this.showLoadingProgress = showLoadingProgress;
- this.packageFunctionCache = packageFunctionCache;
- this.compiledBuildFileCache = compiledBuildFileCache;
this.numPackagesLoaded = numPackagesLoaded;
this.packageProgress = packageProgress;
this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile;
@@ -240,20 +231,6 @@
}
}
- /** An entry in {@link PackageFunction} internal cache. */
- public static class LoadedPackageCacheEntry {
- private final Package.Builder builder;
- private final Set<SkyKey> globDepKeys;
- private final long loadTimeNanos;
-
- private LoadedPackageCacheEntry(
- Package.Builder builder, Set<SkyKey> globDepKeys, long loadTimeNanos) {
- this.builder = builder;
- this.globDepKeys = globDepKeys;
- this.loadTimeNanos = loadTimeNanos;
- }
- }
-
/**
* A declaration to {@link PackageFunction} about how it will be used, for the sake of making
* use-case-driven performance optimizations.
@@ -380,15 +357,48 @@
}
@Override
+ public boolean supportsSkyKeyComputeState() {
+ return true;
+ }
+
+ @Override
+ public SkyKeyComputeState createNewSkyKeyComputeState() {
+ return new State();
+ }
+
+ private static class LoadedPackage {
+ private final Package.Builder builder;
+ private final Set<SkyKey> globDepKeys;
+ private final long loadTimeNanos;
+
+ private LoadedPackage(Package.Builder builder, Set<SkyKey> globDepKeys, long loadTimeNanos) {
+ this.builder = builder;
+ this.globDepKeys = globDepKeys;
+ this.loadTimeNanos = loadTimeNanos;
+ }
+ }
+
+ private static class State implements SkyKeyComputeState {
+ @Nullable private CompiledBuildFile compiledBuildFile;
+ @Nullable private LoadedPackage loadedPackage;
+ }
+
+ @Override
public SkyValue compute(SkyKey key, Environment env)
throws PackageFunctionException, InterruptedException {
+ return compute(key, createNewSkyKeyComputeState(), env);
+ }
+
+ @Override
+ public SkyValue compute(SkyKey key, SkyKeyComputeState skyKeyComputeState, Environment env)
+ throws PackageFunctionException, InterruptedException {
PackageIdentifier packageId = (PackageIdentifier) key.argument();
if (packageId.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) {
return getExternalPackage(env);
}
- // TODO(adonovan): opt: can't all the following statements be moved
- // into the packageFunctionCache.getIfPresent cache-miss case?
+ // TODO(b/209701268): opt: can't all the following statements be moved
+ // into the state.loadedPackage cache-miss case?
SkyKey packageLookupKey = PackageLookupValue.key(packageId);
PackageLookupValue packageLookupValue;
@@ -515,9 +525,11 @@
// TODO(adonovan): put BUILD compilation from BUILD execution in separate Skyframe functions
// like we do for .bzl files, so that we don't need to recompile BUILD files each time their
// .bzl dependencies change.
- LoadedPackageCacheEntry packageCacheEntry = packageFunctionCache.getIfPresent(packageId);
- if (packageCacheEntry == null) {
- packageCacheEntry =
+
+ State state = (State) skyKeyComputeState;
+ LoadedPackage loadedPackage = state.loadedPackage;
+ if (loadedPackage == null) {
+ loadedPackage =
loadPackage(
workspaceName,
repositoryMapping,
@@ -531,14 +543,15 @@
preludeLabel,
packageLookupValue.getRoot(),
env,
- key);
- if (packageCacheEntry == null) {
+ key,
+ state);
+ if (loadedPackage == null) {
return null; // skyframe restart
}
- packageFunctionCache.put(packageId, packageCacheEntry);
+ state.loadedPackage = loadedPackage;
}
PackageFunctionException pfeFromNonSkyframeGlobbing = null;
- Package.Builder pkgBuilder = packageCacheEntry.builder;
+ Package.Builder pkgBuilder = loadedPackage.builder;
try {
pkgBuilder.buildPartial();
// Since the Skyframe dependencies we request below in
@@ -564,18 +577,15 @@
? Transience.PERSISTENT
: Transience.TRANSIENT);
} catch (InternalInconsistentFilesystemException e) {
- packageFunctionCache.invalidate(packageId);
throw e.throwPackageFunctionException();
}
- Set<SkyKey> globKeys = packageCacheEntry.globDepKeys;
+ Set<SkyKey> globKeys = loadedPackage.globDepKeys;
try {
handleGlobDepsAndPropagateFilesystemExceptions(
packageId, globKeys, env, pkgBuilder.containsErrors());
} catch (InternalInconsistentFilesystemException e) {
- packageFunctionCache.invalidate(packageId);
throw e.throwPackageFunctionException();
} catch (FileSymlinkException e) {
- packageFunctionCache.invalidate(packageId);
String message = "Symlink issue while evaluating globs: " + e.getUserFriendlyMessage();
throw PackageFunctionException.builder()
.setType(PackageFunctionException.Type.NO_SUCH_PACKAGE)
@@ -589,7 +599,6 @@
if (pfeFromNonSkyframeGlobbing != null) {
// Throw before checking for missing values, since this may be our last chance to throw if in
// nokeep-going error bubbling.
- packageFunctionCache.invalidate(packageId);
throw pfeFromNonSkyframeGlobbing;
}
@@ -597,9 +606,6 @@
return null;
}
- // We know this SkyFunction will not be called again, so we can remove the cache entry.
- packageFunctionCache.invalidate(packageId);
-
Package pkg = pkgBuilder.finishBuild();
Event.replayEventsOn(env.getListener(), pkgBuilder.getEvents());
@@ -611,7 +617,7 @@
packageFactory.afterDoneLoadingPackage(
pkg,
starlarkBuiltinsValue.starlarkSemantics,
- packageCacheEntry.loadTimeNanos,
+ loadedPackage.loadTimeNanos,
env.getListener());
} catch (InvalidPackageException e) {
throw new PackageFunctionException(e, Transience.PERSISTENT);
@@ -1224,7 +1230,7 @@
* <p>May return null if the computation has to be restarted.
*/
@Nullable
- private LoadedPackageCacheEntry loadPackage(
+ private LoadedPackage loadPackage(
String workspaceName,
RepositoryMapping repositoryMapping,
ImmutableSet<PathFragment> repositoryIgnoredPatterns,
@@ -1237,7 +1243,8 @@
@Nullable Label preludeLabel,
Root packageRoot,
Environment env,
- SkyKey keyForMetrics)
+ SkyKey keyForMetrics,
+ State state)
throws InterruptedException, PackageFunctionException {
// TODO(adonovan): opt: evaluate splitting this part out as a separate Skyframe
@@ -1254,11 +1261,7 @@
boolean committed = false;
try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.CREATE_PACKAGE, packageId.toString())) {
- // Using separate get/put operations on this cache is safe because there will
- // never be two concurrent calls for the same packageId. The cache is designed
- // to save work across a sequence of restarted Skyframe PackageFunction invocations
- // for the same key.
- CompiledBuildFile compiled = compiledBuildFileCache.getIfPresent(packageId);
+ CompiledBuildFile compiled = state.compiledBuildFile;
if (compiled == null) {
if (showLoadingProgress.get()) {
env.getListener().handle(Event.progress("Loading package: " + packageId));
@@ -1269,9 +1272,7 @@
if (compiled == null) {
return null; // skyframe restart
}
-
- // Cache this first step so we needn't redo it if .bzl loading fails.
- compiledBuildFileCache.put(packageId, compiled);
+ state.compiledBuildFile = compiled;
}
// ^^ ---- end PackageCompileFunction ---- ^^
@@ -1316,7 +1317,7 @@
// From this point on, no matter whether the function returns
// successfully or throws an exception, there will be no more
- // Skyframe restarts, so we can dispense with the cache entry.
+ // Skyframe restarts.
committed = true;
long startTimeNanos = BlazeClock.nanoTime();
@@ -1376,14 +1377,14 @@
numPackagesLoaded.incrementAndGet();
long loadTimeNanos = Math.max(BlazeClock.nanoTime() - startTimeNanos, 0L);
- return new LoadedPackageCacheEntry(pkgBuilder, globDepKeys, loadTimeNanos);
+ return new LoadedPackage(pkgBuilder, globDepKeys, loadTimeNanos);
} finally {
- // Discard the cache entry and declare completion
- // only if we reached the point of no return.
if (committed) {
- compiledBuildFileCache.invalidate(packageId);
+ // We're done executing the BUILD file. Therefore, we can discard the compiled BUILD file...
+ state.compiledBuildFile = null;
if (packageProgress != null) {
+ // ... and also note that we're done.
packageProgress.doneReadPackage(packageId);
}
}
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 4b3abcf..4f08836 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
@@ -170,7 +170,6 @@
import com.google.devtools.build.lib.skyframe.MetadataConsumerForMetrics.FilesMetricConsumer;
import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
import com.google.devtools.build.lib.skyframe.PackageFunction.IncrementalityIntent;
-import com.google.devtools.build.lib.skyframe.PackageFunction.LoadedPackageCacheEntry;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionCompletedReceiver;
import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ProgressSupplier;
@@ -284,16 +283,6 @@
// target parsing exception.
private static final int EXCEPTION_TRAVERSAL_LIMIT = 10;
- // Cache of partially constructed Package instances, stored between reruns of the PackageFunction
- // (because of missing dependencies, within the same evaluate() run) to avoid loading the same
- // package twice (first time loading to find load()ed bzl files and declare Skyframe
- // dependencies).
- private final Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache =
- Caffeine.newBuilder().build();
- // Cache of parsed BUILD files, for PackageFunction. Same motivation as above.
- private final Cache<PackageIdentifier, PackageFunction.CompiledBuildFile> compiledBuildFileCache =
- Caffeine.newBuilder().build();
-
// Cache of parsed bzl files, for use when we're inlining BzlCompileFunction in
// BzlLoadFunction. See the comments in BzlLoadFunction for motivations and details.
private final Cache<BzlCompileValue.Key, BzlCompileValue> bzlCompileCache =
@@ -547,8 +536,6 @@
pkgFactory,
packageManager,
showLoadingProgress,
- packageFunctionCache,
- compiledBuildFileCache,
numPackagesLoaded,
bzlLoadFunctionForInliningPackageAndWorkspaceNodes,
packageProgress,
@@ -1448,8 +1435,6 @@
// Clear internal caches used by SkyFunctions used for package loading. If the SkyFunctions
// never had a chance to restart (e.g. due to user interrupt, or an error in a --nokeep_going
// build), these may have stale entries.
- packageFunctionCache.invalidateAll();
- compiledBuildFileCache.invalidateAll();
bzlCompileCache.invalidateAll();
numPackagesLoaded.set(0);
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 11818be..f8760d0 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
@@ -15,7 +15,6 @@
import static com.google.common.base.Preconditions.checkState;
-import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
@@ -72,7 +71,6 @@
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.IncrementalityIntent;
-import com.google.devtools.build.lib.skyframe.PackageFunction.LoadedPackageCacheEntry;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.skyframe.PackageValue;
@@ -443,8 +441,6 @@
private ImmutableMap<SkyFunctionName, SkyFunction> makeFreshSkyFunctions() {
AtomicReference<TimestampGranularityMonitor> tsgm =
new AtomicReference<>(new TimestampGranularityMonitor(BlazeClock.instance()));
- Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache =
- Caffeine.newBuilder().build();
AtomicReference<FilesystemCalls> syscallCacheRef =
new AtomicReference<>(
PerBuildSyscallCache.newBuilder()
@@ -513,8 +509,6 @@
pkgFactory,
cachingPackageLocator,
/*showLoadingProgress=*/ new AtomicBoolean(false),
- packageFunctionCache,
- /*compiledBuildFileCache=*/ Caffeine.newBuilder().build(),
/*numPackagesLoaded=*/ new AtomicInteger(0),
/*bzlLoadFunctionForInlining=*/ null,
/*packageProgress=*/ null,
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleFunctionTest.java
index 9b40ace..627d621 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleFunctionTest.java
@@ -148,8 +148,6 @@
/*packageFactory=*/ null,
/*pkgLocator=*/ null,
/*showLoadingProgress=*/ null,
- /*packageFunctionCache=*/ null,
- /*compiledBuildFileCache=*/ null,
/*numPackagesLoaded=*/ null,
/*bzlLoadFunctionForInlining=*/ null,
/*packageProgress=*/ null,
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
index aa72101..84659c6 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
@@ -178,8 +178,6 @@
/*packageFactory=*/ null,
/*pkgLocator=*/ null,
/*showLoadingProgress=*/ null,
- /*packageFunctionCache=*/ null,
- /*compiledBuildFileCache=*/ null,
/*numPackagesLoaded=*/ null,
/*bzlLoadFunctionForInlining=*/ null,
/*packageProgress=*/ null,
diff --git a/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java b/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java
index 949959c..2ea1058 100644
--- a/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java
@@ -162,8 +162,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE,
PackageFunction.IncrementalityIntent.INCREMENTAL,
diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
index 094689a2..8b59dc2 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
@@ -192,8 +192,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException
.INSTANCE,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
index 5d0f271..648b3d3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
@@ -119,8 +119,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException
.INSTANCE,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
index 4092351..c05f551 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
@@ -119,8 +119,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE,
PackageFunction.IncrementalityIntent.INCREMENTAL,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index dc3f06a..77ad3f3 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -187,8 +187,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException
.INSTANCE,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
index 0689a2b..fb7fc00 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -156,8 +156,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE,
PackageFunction.IncrementalityIntent.INCREMENTAL,
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 c959539..d7c0550 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
@@ -182,18 +182,21 @@
return value.getPackage();
}
+ private Exception evaluatePackageToException(String pkg) throws Exception {
+ return evaluatePackageToException(pkg, /*keepGoing=*/ false);
+ }
+
/**
* Helper that evaluates the given package and returns the expected exception.
*
* <p>Disables the failFastHandler as a side-effect.
*/
- private Exception evaluatePackageToException(String pkg) throws Exception {
+ private Exception evaluatePackageToException(String pkg, boolean keepGoing) throws Exception {
reporter.removeHandler(failFastHandler);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse(pkg));
EvaluationResult<PackageValue> result =
- SkyframeExecutorTestUtils.evaluate(
- getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
+ SkyframeExecutorTestUtils.evaluate(getSkyframeExecutor(), skyKey, keepGoing, reporter);
assertThat(result.hasError()).isTrue();
return result.getError(skyKey).getException();
}
@@ -401,7 +404,18 @@
scratch.file("foo/bar/baz.sh");
fs.scheduleMakeUnreadableAfterReaddir(barDir);
- Exception ex = evaluatePackageToException("@//foo");
+ Exception ex =
+ evaluatePackageToException(
+ "@//foo",
+ // Use --keep_going, not --nokeep_going, semantics so as to exercise the situation we
+ // want to exercise.
+ //
+ // In --nokeep_going semantics, the GlobValue node's error would halt normal evaluation
+ // and trigger error bubbling. Then, during error bubbling we would freshly compute the
+ // PackageValue node again, meaning we would do non-Skyframe globbing except this time
+ // non-Skyframe globbing would encounter the io error, meaning there actually wouldn't
+ // be a discrepancy.
+ /*keepGoing=*/ true);
String msg = ex.getMessage();
assertThat(msg).contains("Inconsistent filesystem operations");
assertThat(msg).contains("Encountered error '/workspace/foo/bar (Permission denied)'");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
index c4a0baa..baca435 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
@@ -127,8 +127,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE,
PackageFunction.IncrementalityIntent.INCREMENTAL,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 7d4752b..3f35d57 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -174,8 +174,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE,
PackageFunction.IncrementalityIntent.INCREMENTAL,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index a96156f..a8a5cf9 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -286,8 +286,6 @@
null,
null,
null,
- null,
- null,
/*packageProgress=*/ null,
PackageFunction.ActionOnIOExceptionReadingBuildFile.UseOriginalIOException
.INSTANCE,
