Properly handle exceptions that RecursivePkgFunction can encounter, instead of silently swallowing them.
The old behavior was simply incorrect on --keep_going builds because it meant any directory with an uncaught error caused all of its ancestors to not have any values. It wasn't noticed because SkyframeTargetPatternEvaluator was overly permissive in the errors it expects.
We also use a singleton for the empty RecursivePkgValue which might have a negligible (beneficial) memory impact.
--
MOS_MIGRATED_REVID=95037551
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index c3faec8..68cb59a 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -354,6 +354,10 @@
}
result.put(pattern, targetsBuilder.build());
} else {
+ // Because the graph was always initialized via a keep_going build, we know that the
+ // exception stored here must be a TargetParsingException. Thus the comment in
+ // SkyframeTargetPatternEvaluator#parseTargetPatternKeys describing the situation in which
+ // the exception acceptance must be looser does not apply here.
targetParsingException =
(TargetParsingException)
Preconditions.checkNotNull(graph.getException(patternKey), pattern);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 3aeda39..938c8d4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -93,8 +93,11 @@
if (lookup == null) {
// Typically a null value from Environment.getValue(k) means that either the key k is missing
// a dependency or an exception was thrown during evaluation of k. Here, if this getValue
- // call returns null, it can only mean a missing dependency, because
+ // call returns null in a keep_going build, it can only mean a missing dependency, because
// RecursivePkgFunction#compute never throws.
+ // In a nokeep_going build, a lower-level exception that RecursivePkgFunction ignored may
+ // bubble up to here, but we ignore it and depend on the top-level caller to be flexible in
+ // the exception types it can accept.
throw new MissingDepException();
}
// TODO(bazel-team): Make RecursivePkgValue return NestedSet<PathFragment> so this transform is
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
index b814319..30ad2bb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
@@ -18,6 +18,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.PackageIdentifier;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
@@ -31,6 +32,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -57,25 +59,36 @@
Set<PathFragment> excludedPaths = recursivePkgKey.getExcludedPaths();
SkyKey fileKey = FileValue.key(rootedPath);
- FileValue fileValue = (FileValue) env.getValue(fileKey);
+ FileValue fileValue = null;
+ try {
+ fileValue = (FileValue) env.getValueOrThrow(fileKey, InconsistentFilesystemException.class,
+ FileSymlinkCycleException.class, IOException.class);
+ } catch (InconsistentFilesystemException | FileSymlinkCycleException | IOException e) {
+ return reportErrorAndReturn(e, rootRelativePath, env.getListener());
+ }
if (fileValue == null) {
return null;
}
if (!fileValue.isDirectory()) {
- return new RecursivePkgValue(NestedSetBuilder.<String>emptySet(ORDER));
+ return RecursivePkgValue.EMPTY;
}
if (fileValue.isSymlink()) {
// We do not follow directory symlinks when we look recursively for packages. It also
// prevents symlink loops.
- return new RecursivePkgValue(NestedSetBuilder.<String>emptySet(ORDER));
+ return RecursivePkgValue.EMPTY;
}
PackageIdentifier packageId = PackageIdentifier.createInDefaultRepo(
rootRelativePath.getPathString());
- PackageLookupValue pkgLookupValue =
- (PackageLookupValue) env.getValue(PackageLookupValue.key(packageId));
+ PackageLookupValue pkgLookupValue;
+ try {
+ pkgLookupValue = (PackageLookupValue) env.getValueOrThrow(PackageLookupValue.key(packageId),
+ NoSuchPackageException.class, InconsistentFilesystemException.class);
+ } catch (NoSuchPackageException | InconsistentFilesystemException e) {
+ return reportErrorAndReturn(e, rootRelativePath, env.getListener());
+ }
if (pkgLookupValue == null) {
return null;
}
@@ -171,7 +184,15 @@
packages.addTransitive(((RecursivePkgValue) childValue).getPackages());
}
}
- return new RecursivePkgValue(packages.build());
+ return RecursivePkgValue.create(packages);
+ }
+
+ // Ignore all errors in traversal and just say there are no packages here.
+ private static RecursivePkgValue reportErrorAndReturn(Exception e, PathFragment rootRelativePath,
+ EventHandler handler) {
+ handler.handle(Event.warn("Finding packages under " + rootRelativePath + " failed, skipping: "
+ + e.getMessage()));
+ return RecursivePkgValue.EMPTY;
}
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
index 56ed0e5..721ebc8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
@@ -16,6 +16,8 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -32,14 +34,23 @@
*/
@Immutable
@ThreadSafe
-public class RecursivePkgValue implements SkyValue {
+class RecursivePkgValue implements SkyValue {
+ static final RecursivePkgValue EMPTY =
+ new RecursivePkgValue(NestedSetBuilder.<String>emptySet(Order.STABLE_ORDER));
private final NestedSet<String> packages;
- public RecursivePkgValue(NestedSet<String> packages) {
+ private RecursivePkgValue(NestedSet<String> packages) {
this.packages = packages;
}
+ static RecursivePkgValue create(NestedSetBuilder<String> packages) {
+ if (packages.isEmpty()) {
+ return EMPTY;
+ }
+ return new RecursivePkgValue(packages.build());
+ }
+
/**
* Create a transitive package lookup request.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
index d166126..e504f88 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
@@ -180,6 +180,11 @@
continue;
}
if (error.getException() != null) {
+ // This exception may not be a TargetParsingException because in a nokeep_going build, the
+ // target pattern parser may swallow a NoSuchPackageException but the framework will
+ // bubble it up anyway.
+ Preconditions.checkArgument(!keepGoing
+ || error.getException() instanceof TargetParsingException, error);
errorMessage = error.getException().getMessage();
} else if (!Iterables.isEmpty(error.getCycleInfo())) {
errorMessage = "cycles detected during target parsing";
