Properly handle missing packages and I/O exceptions when parsing target patterns in no-keep-going mode. Additionally, eagerly terminate evaluation on "inconsistent filesystem exceptions" even in keep-going mode, by analogy with a source file changing mid-build, which terminates the build even in keep-going mode. Note that we still don't terminate an evaluation eagerly on all inconsistent filesystem exceptions, but this is incrementally better than before, IMO (I think the remaining holes are bzl files and reading source artifacts for the first time).
TargetPatternFunction can recover from missing packages/other errors in keep-going mode, and so before this change it very rarely threw an exception. However, in a no-keep-going evaluation (or a keep-going evaluation that failed catastrophically), the failure to throw an exception meant that it missed a chance to transform a lower-level exception into a more intelligible one. To allow TargetPatternFunction to distinguish these cases, we add a new method on SkyFunction.Environment.
PiperOrigin-RevId: 415073167
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD
index 9861a47..21a2e04 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD
@@ -29,6 +29,8 @@
"//src/main/java/com/google/devtools/build/docgen/annot",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/concurrent",
+ "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
+ "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetParsingException.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetParsingException.java
index 1b592b6..aadddee 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetParsingException.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetParsingException.java
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.cmdline;
import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns;
import com.google.devtools.build.lib.skyframe.DetailedException;
@@ -40,6 +42,25 @@
this.detailedExitCode = Preconditions.checkNotNull(detailedExitCode);
}
+ public TargetParsingException(String message, DetailedExitCode detailedExitCode) {
+ super(Preconditions.checkNotNull(message));
+ this.detailedExitCode = Preconditions.checkNotNull(detailedExitCode);
+ }
+
+ public TargetParsingException(InconsistentFilesystemException cause) {
+ super(cause.getMessage(), cause);
+ this.detailedExitCode =
+ DetailedExitCode.of(
+ FailureDetails.FailureDetail.newBuilder()
+ .setPackageLoading(
+ FailureDetails.PackageLoading.newBuilder()
+ .setCode(
+ FailureDetails.PackageLoading.Code
+ .TRANSIENT_INCONSISTENT_FILESYSTEM_ERROR))
+ .setMessage(getMessage())
+ .build());
+ }
+
private static FailureDetail createFailureDetail(String message, TargetPatterns.Code code) {
return FailureDetail.newBuilder()
.setMessage(message)
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
index ba41c10..8f83005 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
@@ -29,6 +29,8 @@
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException;
import com.google.devtools.build.lib.cmdline.LabelValidator.PackageAndTarget;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns;
import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns.Code;
import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
@@ -130,9 +132,7 @@
*/
public abstract Type getType();
- /**
- * Return the string that was parsed into this pattern.
- */
+ /** Return the string that was parsed into this pattern. */
public String getOriginalPattern() {
return originalPattern;
}
@@ -141,6 +141,12 @@
* Evaluates the current target pattern, excluding targets under directories in both {@code
* ignoredSubdirectories} and {@code excludedSubdirectories}, and returns the result.
*
+ * @throws InconsistentFilesystemException if {@code resolver} makes Skyframe calls and discovers
+ * a filesystem inconsistency as observed by Skyframe, and this pattern does not have type
+ * {@code Type.TARGETS_BELOW_DIRECTORY}
+ * @throws ProcessPackageDirectoryException if {@code resolver} makes Skyframe calls and discovers
+ * a filesystem inconsistency as observed by Skyframe, and this pattern has type {@code
+ * Type.TARGETS_BELOW_DIRECTORY}
* @throws IllegalArgumentException if either {@code ignoredSubdirectories} or {@code
* excludedSubdirectories} is nonempty and this pattern does not have type {@code
* Type.TARGETS_BELOW_DIRECTORY}.
@@ -151,7 +157,8 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<T, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException;
+ throws TargetParsingException, E, InterruptedException, ProcessPackageDirectoryException,
+ InconsistentFilesystemException;
/**
* Evaluates this {@link TargetPattern} synchronously, feeding the result to the given {@code
@@ -160,6 +167,9 @@
* <p>If the returned {@link ListenableFuture}'s {@link ListenableFuture#get} throws an {@code
* ExecutionException}, the cause will be an instance of either {@link TargetParsingException} or
* the given {@code exceptionClass}.
+ *
+ * <p>This method must not be called from within Skyframe evaluation. Use {@link
+ * com.google.devtools.build.lib.skyframe.TargetPatternFunction} and friends for that.
*/
public final <T, E extends Exception & QueryExceptionMarkerInterface>
ListenableFuture<Void> evalAdaptedForAsync(
@@ -173,6 +183,10 @@
return Futures.immediateFuture(null);
} catch (TargetParsingException e) {
return Futures.immediateFailedFuture(e);
+ } catch (ProcessPackageDirectoryException | InconsistentFilesystemException e) {
+ throw new IllegalStateException(
+ "Cannot throw filesystem-related exceptions outside of Skyframe evaluation for " + this,
+ e);
} catch (InterruptedException e) {
return immediateCancelledFuture();
} catch (Exception e) {
@@ -205,8 +219,8 @@
/**
* For patterns of type {@link Type#PATH_AS_TARGET}, returns the path in question.
*
- * <p>The interpretation of this path, of course, depends on the existence of packages.
- * See {@link InterpretPathAsTarget#eval}.
+ * <p>The interpretation of this path, of course, depends on the existence of packages. See {@link
+ * InterpretPathAsTarget#eval}.
*/
public String getPathForPathAsTarget() {
throw new IllegalStateException();
@@ -240,9 +254,8 @@
public abstract RepositoryName getRepository();
/**
- * Returns {@code true} iff this pattern has type {@code Type.TARGETS_BELOW_DIRECTORY} or
- * {@code Type.TARGETS_IN_PACKAGE} and the target pattern suffix specified it should match
- * rules only.
+ * Returns {@code true} iff this pattern has type {@code Type.TARGETS_BELOW_DIRECTORY} or {@code
+ * Type.TARGETS_IN_PACKAGE} and the target pattern suffix specified it should match rules only.
*/
public abstract boolean getRulesOnly();
@@ -326,12 +339,11 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<T, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException {
+ throws TargetParsingException, E, InterruptedException, InconsistentFilesystemException {
if (resolver.isPackage(PackageIdentifier.createInMainRepo(path))) {
// User has specified a package name. lookout for default target.
callback.process(resolver.getExplicitTarget(label("//" + path)).getTargets());
} else {
-
List<String> pieces = SLASH_SPLITTER.splitToList(path);
// Interprets the label as a file target. This loop stops as soon as the
@@ -350,7 +362,7 @@
throw new TargetParsingException(
"couldn't determine target from filename '" + path + "'",
- TargetPatterns.Code.CANNOT_DETERMINE_TARGET_FROM_FILENAME);
+ Code.CANNOT_DETERMINE_TARGET_FROM_FILENAME);
}
}
@@ -424,7 +436,7 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<T, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException {
+ throws TargetParsingException, E, InterruptedException, InconsistentFilesystemException {
if (checkWildcardConflict) {
ResolvedTargets<T> targets = getWildcardConflict(resolver);
if (targets != null) {
@@ -494,7 +506,7 @@
* is such a target. Otherwise, return null.
*/
private <T> ResolvedTargets<T> getWildcardConflict(TargetPatternResolver<T> resolver)
- throws InterruptedException {
+ throws InconsistentFilesystemException, InterruptedException {
if (!wasOriginallyAbsolute) {
return null;
}
@@ -509,11 +521,12 @@
}
if (target != null) {
- resolver.warn(String.format("The target pattern '%s' is ambiguous: '%s' is " +
- "both a wildcard, and the name of an existing %s; " +
- "using the latter interpretation",
- getOriginalPattern(), ":" + suffix,
- resolver.getTargetKind(target)));
+ resolver.warn(
+ String.format(
+ "The target pattern '%s' is ambiguous: '%s' is "
+ + "both a wildcard, and the name of an existing %s; "
+ + "using the latter interpretation",
+ getOriginalPattern(), ":" + suffix, resolver.getTargetKind(target)));
try {
return resolver.getExplicitTarget(label);
} catch (TargetParsingException e) {
@@ -550,7 +563,7 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<T, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException {
+ throws TargetParsingException, E, InterruptedException, ProcessPackageDirectoryException {
Preconditions.checkState(
!excludedSubdirectories.contains(directory.getPackageFragment()),
"Fully excluded target pattern %s should have already been filtered out (%s)",
@@ -831,11 +844,12 @@
private static final List<String> SUFFIXES;
static {
- SUFFIXES = ImmutableList.<String>builder()
- .addAll(ALL_RULES_IN_SUFFIXES)
- .addAll(ALL_TARGETS_IN_SUFFIXES)
- .add("/...")
- .build();
+ SUFFIXES =
+ ImmutableList.<String>builder()
+ .addAll(ALL_RULES_IN_SUFFIXES)
+ .addAll(ALL_TARGETS_IN_SUFFIXES)
+ .add("/...")
+ .build();
}
/**
@@ -939,7 +953,7 @@
String targetPart = colonIndex < 0 ? "" : pattern.substring(colonIndex + 1);
if (packagePart.equals("...")) {
- packagePart = "/..."; // special case this for easier parsing
+ packagePart = "/..."; // special case this for easier parsing
}
if (packagePart.endsWith("/")) {
@@ -952,8 +966,8 @@
String realPackagePart = packagePart.substring(0, packagePart.length() - "/...".length());
PackageIdentifier packageIdentifier;
try {
- packageIdentifier = PackageIdentifier.parse(
- repository.getName() + "//" + realPackagePart);
+ packageIdentifier =
+ PackageIdentifier.parse(repository.getName() + "//" + realPackagePart);
} catch (LabelSyntaxException e) {
throw new TargetParsingException(
"Invalid package name '" + realPackagePart + "': " + e.getMessage(),
@@ -1078,9 +1092,7 @@
}
}
- /**
- * The target pattern type (targets below package, in package, explicit target, etc.)
- */
+ /** The target pattern type (targets below package, in package, explicit target, etc.) */
public enum Type {
/** A path interpreted as a target, eg "foo/bar/baz" */
PATH_AS_TARGET,
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
index f1c81df..03fdb98 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
@@ -21,32 +21,31 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
/**
- * A callback that is used during the process of converting target patterns (such as
- * <code>//foo:all</code>) into one or more lists of targets (such as <code>//foo:foo,
+ * A callback that is used during the process of converting target patterns (such as <code>//foo:all
+ * </code>) into one or more lists of targets (such as <code>//foo:foo,
* //foo:bar</code>). During a call to {@link TargetPattern#eval}, the {@link TargetPattern} makes
* calls to this interface to implement the target pattern semantics. The generic type {@code T} is
* only for compile-time type safety; there are no requirements to the actual type.
*/
public abstract class TargetPatternResolver<T> {
- /**
- * Reports the given warning.
- */
+ /** Reports the given warning. */
public abstract void warn(String msg);
/**
* Returns a single target corresponding to the given label, or null. This method may only throw
* an exception if the current thread was interrupted.
*/
- public abstract T getTargetOrNull(Label label) throws InterruptedException;
+ public abstract T getTargetOrNull(Label label)
+ throws InterruptedException, InconsistentFilesystemException;
- /**
- * Returns a single target corresponding to the given label, or an empty or failed result.
- */
+ /** Returns a single target corresponding to the given label, or an empty or failed result. */
public abstract ResolvedTargets<T> getExplicitTarget(Label label)
throws TargetParsingException, InterruptedException;
@@ -89,6 +88,8 @@
* @param callback the callback to receive the result, possibly in multiple batches.
* @param exceptionClass The class type of the parameterized exception.
* @throws TargetParsingException under implementation-specific failure conditions
+ * @throws ProcessPackageDirectoryException only when called from within Skyframe and an
+ * inconsistent filesystem state is observed
*/
public abstract <E extends Exception & QueryExceptionMarkerInterface>
void findTargetsBeneathDirectory(
@@ -100,10 +101,11 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<T, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException;
+ throws TargetParsingException, E, InterruptedException, ProcessPackageDirectoryException;
/**
- * Async version of {@link #findTargetsBeneathDirectory}
+ * Async version of {@link #findTargetsBeneathDirectory}. Never call this from within Skyframe
+ * evaluation.
*
* <p>Default implementation is synchronous.
*/
@@ -118,7 +120,7 @@
BatchCallback<T, E> callback,
Class<E> exceptionClass,
ListeningExecutorService executor) {
- try {
+ try {
findTargetsBeneathDirectory(
repository,
originalPattern,
@@ -129,16 +131,23 @@
callback,
exceptionClass);
return immediateVoidFuture();
- } catch (TargetParsingException e) {
+ } catch (TargetParsingException e) {
return immediateFailedFuture(e);
- } catch (InterruptedException e) {
+ } catch (InterruptedException e) {
return immediateCancelledFuture();
- } catch (Exception e) {
- if (exceptionClass.isInstance(e)) {
+ } catch (ProcessPackageDirectoryException e) {
+ throw new IllegalStateException(
+ "Async find targets beneath directory isn't called from within Skyframe: traversing "
+ + directory
+ + " for "
+ + originalPattern,
+ e);
+ } catch (Exception e) {
+ if (exceptionClass.isInstance(e)) {
return immediateFailedFuture(e);
- }
- throw new IllegalStateException(e);
}
+ throw new IllegalStateException(e);
+ }
}
/**
@@ -146,10 +155,8 @@
* file with the name {@code packageName/BUILD} exists in the appropriate repository.
*/
public abstract boolean isPackage(PackageIdentifier packageIdentifier)
- throws InterruptedException;
+ throws InterruptedException, InconsistentFilesystemException;
- /**
- * Returns the target kind of the given target, for example {@code cc_library rule}.
- */
+ /** Returns the target kind of the given target, for example {@code cc_library rule}. */
public abstract String getTargetKind(T target);
}
diff --git a/src/main/java/com/google/devtools/build/lib/io/BUILD b/src/main/java/com/google/devtools/build/lib/io/BUILD
index fdd02fe..540cd48 100644
--- a/src/main/java/com/google/devtools/build/lib/io/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/io/BUILD
@@ -16,6 +16,18 @@
)
java_library(
+ name = "process_package_directory_exception",
+ srcs = ["ProcessPackageDirectoryException.java"],
+ deps = [
+ ":inconsistent_filesystem_exception",
+ "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
+ "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
+ "//src/main/java/com/google/devtools/build/lib/vfs",
+ "//src/main/protobuf:failure_details_java_proto",
+ ],
+)
+
+java_library(
name = "file_symlink_exception",
srcs = ["FileSymlinkException.java"],
)
diff --git a/src/main/java/com/google/devtools/build/lib/io/ProcessPackageDirectoryException.java b/src/main/java/com/google/devtools/build/lib/io/ProcessPackageDirectoryException.java
new file mode 100644
index 0000000..4cc6ea5
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/io/ProcessPackageDirectoryException.java
@@ -0,0 +1,53 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.io;
+
+import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.skyframe.DetailedException;
+import com.google.devtools.build.lib.util.DetailedExitCode;
+import com.google.devtools.build.lib.vfs.RootedPath;
+
+/**
+ * Exception indicating that {@link com.google.devtools.build.lib.skyframe.ProcessPackageDirectory}
+ * failed due to an {@link InconsistentFilesystemException}. Wraps {@link
+ * InconsistentFilesystemException} so that we have a {@link DetailedException} for top-level
+ * reporting.
+ */
+public final class ProcessPackageDirectoryException extends Exception implements DetailedException {
+ private final DetailedExitCode detailedExitCode;
+
+ public ProcessPackageDirectoryException(RootedPath directory, InconsistentFilesystemException e) {
+ super(
+ "Directory '"
+ + directory.asPath().getPathString()
+ + "' could not be processed: "
+ + e.getMessage(),
+ e);
+ this.detailedExitCode =
+ DetailedExitCode.of(
+ FailureDetails.FailureDetail.newBuilder()
+ .setMessage(getMessage())
+ .setPackageLoading(
+ FailureDetails.PackageLoading.newBuilder()
+ .setCode(
+ FailureDetails.PackageLoading.Code
+ .TRANSIENT_INCONSISTENT_FILESYSTEM_ERROR))
+ .build());
+ }
+
+ @Override
+ public DetailedExitCode getDetailedExitCode() {
+ return detailedExitCode;
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD b/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD
index 66e91bd..df8ed13 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/BUILD
@@ -31,6 +31,8 @@
"//src/main/java/com/google/devtools/build/lib/collect/compacthashset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
+ "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
+ "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/query2/engine",
"//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java
index e04a7c6..923ad48 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java
@@ -16,6 +16,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
@@ -61,5 +62,5 @@
* @param packageName the name of the package.
*/
boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
- throws InterruptedException;
+ throws InconsistentFilesystemException, InterruptedException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
index 94de4cd..a513f39 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
@@ -28,9 +29,7 @@
import com.google.devtools.build.lib.vfs.RootedPath;
import java.util.Map;
-/**
- * Support for resolving {@code package/...} target patterns.
- */
+/** Support for resolving {@code package/...} target patterns. */
public interface RecursivePackageProvider extends PackageProvider {
/**
@@ -60,7 +59,8 @@
PathFragment directory,
ImmutableSet<PathFragment> ignoredSubdirectories,
ImmutableSet<PathFragment> excludedSubdirectories)
- throws InterruptedException, QueryException;
+ throws InterruptedException, QueryException, NoSuchPackageException,
+ ProcessPackageDirectoryException;
/**
* Returns the {@link Package} corresponding to each Package in "pkgIds". If any of the packages
diff --git a/src/main/java/com/google/devtools/build/lib/query2/BUILD b/src/main/java/com/google/devtools/build/lib/query2/BUILD
index 888d53c..202dd35 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/query2/BUILD
@@ -51,6 +51,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:target_and_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection",
"//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context",
+ "//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/buildeventstream",
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/causes",
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 b706e2a..265c4b3 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
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.query2;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import static com.google.devtools.build.lib.pkgcache.FilteringPolicies.NO_FILTER;
@@ -32,12 +33,12 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.common.flogger.GoogleLogger;
-import com.google.common.util.concurrent.AsyncFunction;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.ParallelVisitor.VisitTaskStatusCallback;
@@ -108,6 +109,7 @@
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationContext;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -138,9 +140,9 @@
* particular order. As well, this class eagerly loads the full transitive closure of targets, even
* if the full closure isn't needed.
*
- * <p>This class has concurrent implementations of the
- * {@link QueryTaskFuture}/{@link QueryTaskCallable} helper methods. The combination of this and the
- * asynchronous evaluation model yields parallel query evaluation.
+ * <p>This class has concurrent implementations of the {@link QueryTaskFuture}/{@link
+ * QueryTaskCallable} helper methods. The combination of this and the asynchronous evaluation model
+ * yields parallel query evaluation.
*/
public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
implements StreamableQueryEnvironment<Target> {
@@ -266,7 +268,7 @@
if (graph == null || graph != result.getWalkableGraph()) {
checkEvaluationResult(universeScopeListToUse, roots, universeKey, result);
packageSemaphore = makeFreshPackageMultisetSemaphore();
- graph = result.getWalkableGraph();
+ graph = Preconditions.checkNotNull(result.getWalkableGraph(), result);
ignoredPatternsSupplier =
MemoizingInterruptibleSupplier.of(new IgnoredPatternSupplier(graph));
graphBackedRecursivePackageProvider =
@@ -278,14 +280,15 @@
}
if (executor == null) {
- executor = MoreExecutors.listeningDecorator(
- new ThreadPoolExecutor(
- /*corePoolSize=*/ queryEvaluationParallelismLevel,
- /*maximumPoolSize=*/ queryEvaluationParallelismLevel,
- /*keepAliveTime=*/ 1,
- /*units=*/ TimeUnit.SECONDS,
- /*workQueue=*/ new BlockingStack<Runnable>(),
- new ThreadFactoryBuilder().setNameFormat("QueryEnvironment %d").build()));
+ executor =
+ MoreExecutors.listeningDecorator(
+ new ThreadPoolExecutor(
+ /*corePoolSize=*/ queryEvaluationParallelismLevel,
+ /*maximumPoolSize=*/ queryEvaluationParallelismLevel,
+ /*keepAliveTime=*/ 1,
+ /*unit=*/ TimeUnit.SECONDS,
+ /*workQueue=*/ new BlockingStack<Runnable>(),
+ new ThreadFactoryBuilder().setNameFormat("QueryEnvironment %d").build()));
}
resolver = makeNewTargetPatternResolver();
}
@@ -329,33 +332,82 @@
return dependencyFilter != DependencyFilter.ALL_DEPS;
}
- protected void checkEvaluationResult(
+ private void checkEvaluationResult(
ImmutableList<String> universeScopeList,
Set<SkyKey> roots,
SkyKey universeKey,
EvaluationResult<SkyValue> result)
throws QueryException {
// If the only root is the universe key, we expect to see either a single successfully evaluated
- // value or a cycle in the result.
- if (roots.size() == 1 && Iterables.getOnlyElement(roots).equals(universeKey)) {
- Collection<SkyValue> values = result.values();
- if (!values.isEmpty()) {
- Preconditions.checkState(
- values.size() == 1,
- "Universe query \"%s\" returned multiple values unexpectedly (%s values in result)",
- universeScopeList,
- values.size());
- Preconditions.checkNotNull(result.get(universeKey), result);
+ // value or a cycle in the result or a catastrophic error.
+ Collection<SkyValue> values = result.values();
+ if (!values.isEmpty()) {
+ if (roots.size() != 1 || !Iterables.getOnlyElement(roots).equals(universeKey)) {
+ return;
+ }
+ Preconditions.checkState(
+ values.size() == 1,
+ "Universe query \"%s\" returned multiple values unexpectedly (%s values in result)",
+ universeScopeList,
+ values.size());
+ Preconditions.checkNotNull(result.get(universeKey), result);
+ return;
+ }
+ Preconditions.checkState(
+ result.hasError(),
+ "Universe query \"%s\" failed but had no error: %s",
+ universeScopeList,
+ result);
+ Exception exception = result.getCatastrophe();
+ if (exception != null) {
+ throw throwException(exception, result);
+ }
+
+ // Catastrophe not present: look at top-level keys now.
+ boolean foundCycle = false;
+ for (ErrorInfo errorInfo : result.errorMap().values()) {
+ if (errorInfo == null) {
+ continue;
+ }
+ if (!errorInfo.getCycleInfo().isEmpty()) {
+ foundCycle = true;
} else {
- // No values in the result, so there must be an error. We expect the error to be a cycle.
- boolean foundCycle = !result.getError().getCycleInfo().isEmpty();
- Preconditions.checkState(
- foundCycle,
- "Universe query \"%s\" failed with non-cycle error: %s",
- universeScopeList,
- result.getError());
+ exception = errorInfo.getException();
+ if (exception instanceof DetailedException) {
+ break;
+ }
}
}
+
+ if (exception != null) {
+ throw throwException(exception, result);
+ }
+ Preconditions.checkState(
+ foundCycle,
+ "No cycle or exception found in result with error: %s %s %s %s",
+ result,
+ roots,
+ universeKey,
+ universeScopeList);
+ }
+
+ private static QueryException throwException(
+ Exception exception, EvaluationResult<SkyValue> resultForDebugging) throws QueryException {
+ FailureDetail failureDetail;
+ if (!(exception instanceof DetailedException)) {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ "Non-detailed exception found during universe scope building: " + resultForDebugging,
+ exception));
+ failureDetail =
+ FailureDetail.newBuilder()
+ .setQuery(Query.newBuilder().setCode(Code.NON_DETAILED_ERROR))
+ .build();
+ } else {
+ failureDetail = ((DetailedException) exception).getDetailedExitCode().getFailureDetail();
+ }
+ throw new QueryException(
+ "Building universe scope failed: " + exception.getMessage(), exception, failureDetail);
}
private static final Duration MIN_LOGGING = Duration.ofMillis(50);
@@ -403,7 +455,7 @@
@Override
protected void evalTopLevelInternal(
QueryExpression expr, OutputFormatterCallback<Target> callback)
- throws QueryException, InterruptedException {
+ throws QueryException, InterruptedException {
Throwable throwableToThrow = null;
try {
super.evalTopLevelInternal(expr, callback);
@@ -455,7 +507,7 @@
@Override
public QueryEvalResult evaluateQuery(
QueryExpression expr, ThreadSafeOutputFormatterCallback<Target> callback)
- throws QueryException, InterruptedException, IOException {
+ throws QueryException, InterruptedException, IOException {
beforeEvaluateQuery(expr);
// SkyQueryEnvironment batches callback invocations using a BatchStreamedCallback, created here
@@ -465,23 +517,22 @@
//
// This flushes the batched callback prior to constructing the QueryEvalResult in the unlikely
// case of a race between the original callback and the eventHandler.
- BatchStreamedCallback batchCallback = new BatchStreamedCallback(
- callback,
- BATCH_CALLBACK_SIZE,
- createUniquifierForOuterBatchStreamedCallback(expr));
+ BatchStreamedCallback batchCallback =
+ new BatchStreamedCallback(
+ callback, BATCH_CALLBACK_SIZE, createUniquifierForOuterBatchStreamedCallback(expr));
return super.evaluateQuery(expr, batchCallback);
}
Map<SkyKey, Collection<Target>> targetifyValues(Map<SkyKey, ? extends Iterable<SkyKey>> input)
throws InterruptedException {
return targetifyValues(
- input,
- makePackageKeyToTargetKeyMap(ImmutableSet.copyOf(Iterables.concat(input.values()))));
+ input, makePackageKeyToTargetKeyMap(ImmutableSet.copyOf(Iterables.concat(input.values()))));
}
private Map<SkyKey, Collection<Target>> targetifyValues(
Map<SkyKey, ? extends Iterable<SkyKey>> input,
- Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap) throws InterruptedException {
+ Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap)
+ throws InterruptedException {
ImmutableMap.Builder<SkyKey, Collection<Target>> result = ImmutableMap.builder();
Map<SkyKey, Target> allTargets =
@@ -644,7 +695,8 @@
/** Targetify SkyKeys of reverse deps and filter out targets whose deps are not allowed. */
Collection<Target> filterRawReverseDepsOfTransitiveTraversalKeys(
Map<SkyKey, ? extends Iterable<SkyKey>> rawReverseDeps,
- Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap) throws InterruptedException {
+ Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap)
+ throws InterruptedException {
return processRawReverseDeps(targetifyValues(rawReverseDeps, packageKeyToTargetKeyMap));
}
@@ -654,8 +706,8 @@
CompactHashSet<Target> visited =
CompactHashSet.createWithExpectedSize(totalSizeOfCollections(rawReverseDeps.values()));
- Set<Label> keys = CompactHashSet.create(Collections2.transform(rawReverseDeps.keySet(),
- SKYKEY_TO_LABEL));
+ Set<Label> keys =
+ CompactHashSet.create(Collections2.transform(rawReverseDeps.keySet(), SKYKEY_TO_LABEL));
for (Collection<Target> parentCollection : rawReverseDeps.values()) {
for (Target parent : parentCollection) {
if (visited.add(parent)) {
@@ -825,11 +877,6 @@
public QueryTaskFuture<Void> evalTargetPatternKey(
QueryExpression owner, TargetPatternKey targetPatternKey, Callback<Target> callback) {
TargetPattern patternToEval = targetPatternKey.getParsedPattern();
- AsyncFunction<TargetParsingException, Void> reportBuildFileErrorAsyncFunction =
- exn -> {
- handleError(owner, exn.getMessage(), exn.getDetailedExitCode());
- return Futures.immediateFuture(null);
- };
Callback<Target> filteredCallback = callback;
if (!targetPatternKey.getPolicy().equals(NO_FILTER)) {
filteredCallback =
@@ -852,7 +899,10 @@
Futures.catchingAsync(
evalFuture,
TargetParsingException.class,
- reportBuildFileErrorAsyncFunction,
+ exn -> {
+ handleError(owner, exn.getMessage(), exn.getDetailedExitCode());
+ return immediateVoidFuture();
+ },
directExecutor()));
}
@@ -1028,9 +1078,8 @@
@Override
public void buildTransitiveClosure(
- QueryExpression caller,
- ThreadSafeMutableSet<Target> targets,
- int maxDepth) throws QueryException, InterruptedException {
+ QueryExpression caller, ThreadSafeMutableSet<Target> targets, int maxDepth)
+ throws QueryException, InterruptedException {
// Everything has already been loaded, so here we just check for errors so that we can
// pre-emptively throw/report if needed.
reportUnsuccessfulOrMissingTargetsInternal(targets, ImmutableSet.of(), caller);
@@ -1188,9 +1237,7 @@
public static Set<PackageIdentifier> getPkgIdsNeededForTargetification(
Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap) {
- return packageKeyToTargetKeyMap
- .keySet()
- .stream()
+ return packageKeyToTargetKeyMap.keySet().stream()
.map(SkyQueryEnvironment.PACKAGE_SKYKEY_TO_PACKAGE_IDENTIFIER)
.collect(toImmutableSet());
}
@@ -1208,14 +1255,14 @@
Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap) throws InterruptedException {
Multimap<PackageIdentifier, Target> result = ArrayListMultimap.create();
getTargetsForPackageKeyToTargetKeyMapHelper(
- packageKeyToTargetKeyMap,
- (k, t) -> result.put(t.getLabel().getPackageIdentifier(), t));
+ packageKeyToTargetKeyMap, (k, t) -> result.put(t.getLabel().getPackageIdentifier(), t));
return result;
}
private void getTargetsForPackageKeyToTargetKeyMapHelper(
Multimap<SkyKey, SkyKey> packageKeyToTargetKeyMap,
- BiConsumer<SkyKey, Target> targetKeyAndTargetConsumer) throws InterruptedException {
+ BiConsumer<SkyKey, Target> targetKeyAndTargetConsumer)
+ throws InterruptedException {
Set<SkyKey> processedTargets = new HashSet<>();
Map<SkyKey, SkyValue> packageMap = graph.getSuccessfulValues(packageKeyToTargetKeyMap.keySet());
for (Map.Entry<SkyKey, SkyValue> entry : packageMap.entrySet()) {
@@ -1309,8 +1356,7 @@
private static class SkyKeyKeyExtractor implements KeyExtractor<SkyKey, SkyKey> {
private static final SkyKeyKeyExtractor INSTANCE = new SkyKeyKeyExtractor();
- private SkyKeyKeyExtractor() {
- }
+ private SkyKeyKeyExtractor() {}
@Override
public SkyKey extractKey(SkyKey element) {
@@ -1423,11 +1469,7 @@
protected QueryTaskFuture<Predicate<SkyKey>> getUnfilteredUniverseDTCSkyKeyPredicateFuture(
QueryExpression universe, QueryExpressionContext<Target> context) {
return ParallelSkyQueryUtils.getDTCSkyKeyPredicateFuture(
- this,
- universe,
- context,
- BATCH_CALLBACK_SIZE,
- queryEvaluationParallelismLevel);
+ this, universe, context, BATCH_CALLBACK_SIZE, queryEvaluationParallelismLevel);
}
@ThreadSafe
diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java
index 84c67b6..83e7efa 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java
@@ -220,8 +220,7 @@
}
@Override
- public void processOutput(Iterable<T> partialResult)
- throws IOException, InterruptedException {
+ public void processOutput(Iterable<T> partialResult) throws IOException, InterruptedException {
empty.compareAndSet(true, Iterables.isEmpty(partialResult));
callback.processOutput(partialResult);
}
@@ -242,15 +241,10 @@
@Override
public void handleError(
- QueryExpression expression, String message, @Nullable DetailedExitCode detailedExitCode)
+ QueryExpression expression, String message, DetailedExitCode detailedExitCode)
throws QueryException {
if (!keepGoing) {
- if (detailedExitCode != null) {
- throw new QueryException(expression, message, detailedExitCode.getFailureDetail());
- }
- logger.atWarning().atMostEvery(1, MINUTES).log(
- "Null detailed exit code for %s %s", message, expression);
- throw new QueryException(expression, message, Query.Code.BUILD_FILE_ERROR);
+ throw new QueryException(expression, message, detailedExitCode.getFailureDetail());
}
eventHandler.handle(createErrorEvent(expression, message, detailedExitCode));
}
@@ -327,8 +321,7 @@
protected static class TargetKeyExtractor implements KeyExtractor<Target, Label> {
public static final TargetKeyExtractor INSTANCE = new TargetKeyExtractor();
- private TargetKeyExtractor() {
- }
+ private TargetKeyExtractor() {}
@Override
public Label extractKey(Target element) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index e289087..8057aa5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -297,6 +297,7 @@
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_exception",
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function",
"//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
+ "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages:exec_group",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
@@ -1512,7 +1513,6 @@
"//src/main/java/com/google/devtools/build/lib/cmdline:batch_callback",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
- "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/query2/engine",
@@ -1932,7 +1932,6 @@
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
- "//third_party:jsr305",
],
)
@@ -1968,7 +1967,6 @@
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
- "//third_party:jsr305",
],
)
@@ -2006,6 +2004,7 @@
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_exception",
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function",
"//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
+ "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/vfs",
@@ -2062,7 +2061,7 @@
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
- "//third_party:flogger",
+ "//third_party:error_prone_annotations",
"//third_party:guava",
"//third_party:jsr305",
],
@@ -2115,6 +2114,8 @@
"//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
+ "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
+ "//src/main/java/com/google/devtools/build/lib/io:process_package_directory_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/query2/engine",
@@ -2134,12 +2135,12 @@
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
+ "//src/main/java/com/google/devtools/build/lib/skyframe:process_package_directory",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
- "//third_party:jsr305",
],
)
@@ -2384,9 +2385,9 @@
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
+ "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
- "//src/main/protobuf:failure_details_java_proto",
"//third_party:guava",
"//third_party:jsr305",
],
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java
index 35f7572..c71df9b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryFunction.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.skyframe.ProcessPackageDirectory.ProcessPackageDirectorySkyFunctionException;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
@@ -41,7 +42,8 @@
}
@Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
return new MyTraversalFunction(directories)
.visitDirectory((RecursivePkgKey) skyKey.argument(), env);
}
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 cd3dcd3..acb616e 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
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
@@ -35,10 +36,13 @@
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.ValueOrException2;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
/**
* A {@link com.google.devtools.build.lib.pkgcache.RecursivePackageProvider} backed by an {@link
@@ -47,12 +51,23 @@
*
* <p>This implementation never emits events through the {@link ExtendedEventHandler}s passed to its
* methods. Instead, it emits events through its environment's {@link Environment#getListener()}.
+ *
+ * <p>This implementation suppresses most {@link NoSuchPackageException}s discovered during package
+ * loading, since target pattern expansion may tolerate point failures in packages. The first one
+ * found is stored so that inside a nokeep-going build it can be retrieved, wrapped, and rethrown.
+ * The exception(!) to the rule is errors loading a package via {@link #getPackage}, since the
+ * corresponding target pattern does throw eagerly if the package cannot be loaded.
+ *
+ * <p>On the other hand, exceptions indicating a bad filesystem are propagated eagerly, since they
+ * are catastrophic failures that should terminate the evaluation.
*/
public final class EnvironmentBackedRecursivePackageProvider
extends AbstractRecursivePackageProvider {
private final Environment env;
private final AtomicBoolean encounteredPackageErrors = new AtomicBoolean(false);
+ private final AtomicReference<NoSuchPackageException> noSuchPackageException =
+ new AtomicReference<>();
EnvironmentBackedRecursivePackageProvider(Environment env) {
this.env = env;
@@ -71,6 +86,11 @@
return encounteredPackageErrors.get();
}
+ @Nullable
+ NoSuchPackageException maybeGetNoSuchPackageException() {
+ return noSuchPackageException.get();
+ }
+
@Override
public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
throws NoSuchPackageException, MissingDepException, InterruptedException {
@@ -102,6 +122,7 @@
// package returned by this method, or else determine whether any errors have been seen via
// the "encounteredPackageErrors" method.
encounteredPackageErrors.set(true);
+ noSuchPackageException.compareAndSet(null, e);
}
}
return pkgValue.getPackage();
@@ -117,9 +138,10 @@
return builder.build();
}
+ @SuppressWarnings("ThrowsUncheckedException") // Good for callers to know about MissingDep.
@Override
public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageId)
- throws MissingDepException, InterruptedException {
+ throws MissingDepException, InconsistentFilesystemException, InterruptedException {
SkyKey packageLookupKey = PackageLookupValue.key(packageId);
try {
PackageLookupValue packageLookupValue =
@@ -132,8 +154,8 @@
throw new MissingDepException();
}
return packageLookupValue.packageExists();
- } catch (NoSuchPackageException | InconsistentFilesystemException e) {
- env.getListener().handle(Event.error(e.getMessage()));
+ } catch (NoSuchPackageException e) {
+ noSuchPackageException.compareAndSet(null, e);
encounteredPackageErrors.set(true);
return false;
}
@@ -147,7 +169,7 @@
PathFragment directory,
ImmutableSet<PathFragment> ignoredSubdirectories,
ImmutableSet<PathFragment> excludedSubdirectories)
- throws MissingDepException, InterruptedException {
+ throws InterruptedException, NoSuchPackageException, ProcessPackageDirectoryException {
PathPackageLocator packageLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
if (packageLocator == null) {
throw new MissingDepException();
@@ -177,28 +199,44 @@
ImmutableSet.copyOf(
Iterables.filter(ignoredSubdirectories, path -> path.startsWith(directory)));
- for (Root root : roots) {
- RecursivePkgValue lookup =
- (RecursivePkgValue)
- env.getValue(
- RecursivePkgValue.key(
- repository,
- RootedPath.toRootedPath(root, directory),
- filteredIgnoredSubdirectories));
+ List<ValueOrException2<NoSuchPackageException, ProcessPackageDirectoryException>>
+ recursivePackageValues =
+ env.getOrderedValuesOrThrow(
+ Iterables.transform(
+ roots,
+ r ->
+ RecursivePkgValue.key(
+ repository,
+ RootedPath.toRootedPath(r, directory),
+ filteredIgnoredSubdirectories)),
+ NoSuchPackageException.class,
+ ProcessPackageDirectoryException.class);
+ NoSuchPackageException firstNspe = null;
+ for (ValueOrException2<NoSuchPackageException, ProcessPackageDirectoryException> lookupWrapper :
+ recursivePackageValues) {
+ RecursivePkgValue lookup;
+ try {
+ lookup = (RecursivePkgValue) lookupWrapper.get();
+ } catch (NoSuchPackageException e) {
+ // NoSuchPackageException can happen during error bubbling in a no-keep-going build.
+ if (firstNspe == null) {
+ firstNspe = e;
+ }
+ encounteredPackageErrors.set(true);
+ noSuchPackageException.compareAndSet(null, e);
+ continue;
+ }
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 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();
+ continue;
}
if (lookup.hasErrors()) {
encounteredPackageErrors.set(true);
}
+ if (env.valuesMissing()) {
+ // If values are missing, we're only checking for errors, not constructing a result.
+ continue;
+ }
for (String packageName : lookup.getPackages().toList()) {
// TODO(bazel-team): Make RecursivePkgValue return NestedSet<PathFragment> so this transform
// is unnecessary.
@@ -209,5 +247,11 @@
}
}
}
+ if (firstNspe != null) {
+ throw firstNspe;
+ }
+ if (env.valuesMissing()) {
+ throw new MissingDepException();
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
index ea9db34..de7f6d4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.skyframe;
import com.google.devtools.build.lib.actions.FileStateValue;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.FileType;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -45,6 +46,10 @@
this.externalFilesHelper = externalFilesHelper;
}
+ // InconsistentFilesystemException catch block needs to be separate from IOException catch block
+ // below because Java does "single dispatch": the runtime type of e is all that is considered when
+ // deciding which overload of FileStateFunctionException() to call.
+ @SuppressWarnings("UseMultiCatch")
@Override
public FileStateValue compute(SkyKey skyKey, Environment env)
throws FileStateFunctionException, InterruptedException {
@@ -63,6 +68,8 @@
return FileStateValue.create(rootedPath, syscallCache.get(), tsgm.get());
} catch (ExternalFilesHelper.NonexistentImmutableExternalFileException e) {
return FileStateValue.NONEXISTENT_FILE_STATE_NODE;
+ } catch (InconsistentFilesystemException e) {
+ throw new FileStateFunctionException(e);
} catch (IOException e) {
throw new FileStateFunctionException(e);
}
@@ -73,8 +80,21 @@
* FileStateFunction#compute}.
*/
public static final class FileStateFunctionException extends SkyFunctionException {
+ private final boolean isCatastrophic;
+
+ private FileStateFunctionException(InconsistentFilesystemException e) {
+ super(e, Transience.TRANSIENT);
+ this.isCatastrophic = true;
+ }
+
private FileStateFunctionException(IOException e) {
super(e, Transience.TRANSIENT);
+ this.isCatastrophic = false;
+ }
+
+ @Override
+ public boolean isCatastrophic() {
+ return isCatastrophic;
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index 0f9fdc1..f61cecf 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -30,7 +30,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
@@ -184,11 +183,16 @@
// package, because the SkyQuery environment has already loaded the universe.
return false;
} else {
- if (exception instanceof NoSuchPackageException
- || exception instanceof InconsistentFilesystemException) {
+ if (exception instanceof NoSuchPackageException) {
eventHandler.handle(Event.error(exception.getMessage()));
return false;
} else {
+ // InconsistentFilesystemException can theoretically be thrown by PackageLookupFunction.
+ // However, such exceptions are catastrophic. If we evaluated this PackageLookupFunction
+ // immediately prior to doing the current graph traversal, we should have already failed
+ // catastrophically. On the other hand, if PackageLookupFunction was evaluated on a
+ // previous evaluation, it would not have been committed to the graph, since a
+ // catastrophe triggers error bubbling, which does not commit nodes to the graph.
throw new IllegalStateException(
"During package lookup for '" + packageName + "', got unexpected exception type",
exception);
@@ -246,12 +250,10 @@
ImmutableSet<PathFragment> ignoredSubdirectories,
ImmutableSet<PathFragment> excludedSubdirectories)
throws InterruptedException, QueryException {
- ImmutableList<Root> roots = checkValidDirectoryAndGetRoots(repository, directory);
-
rootPackageExtractor.streamPackagesFromRoots(
results,
graph,
- roots,
+ checkValidDirectoryAndGetRoots(repository, directory),
eventHandler,
repository,
directory,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index 0ab4fda..c1d4e4b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -48,9 +48,7 @@
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.StarlarkSemantics;
-/**
- * SkyFunction for {@link PackageLookupValue}s.
- */
+/** SkyFunction for {@link PackageLookupValue}s. */
public class PackageLookupFunction implements SkyFunction {
/** Lists possible ways to handle a package label which crosses into a new repository. */
public enum CrossRepositoryLabelViolationStrategy {
@@ -105,11 +103,11 @@
PackageIdentifier packageKey = (PackageIdentifier) skyKey.argument();
- String packageNameErrorMsg = LabelValidator.validatePackageName(
- packageKey.getPackageFragment().getPathString());
+ String packageNameErrorMsg =
+ LabelValidator.validatePackageName(packageKey.getPackageFragment().getPathString());
if (packageNameErrorMsg != null) {
- return PackageLookupValue.invalidPackageName("Invalid package name '" + packageKey + "': "
- + packageNameErrorMsg);
+ return PackageLookupValue.invalidPackageName(
+ "Invalid package name '" + packageKey + "': " + packageNameErrorMsg);
}
if (deletedPackages.get().contains(packageKey)) {
@@ -384,8 +382,8 @@
return null;
}
} catch (NoSuchPackageException e) {
- throw new PackageLookupFunctionException(new BuildFileNotFoundException(id, e.getMessage()),
- Transience.PERSISTENT);
+ throw new PackageLookupFunctionException(
+ new BuildFileNotFoundException(id, e.getMessage()), Transience.PERSISTENT);
} catch (IOException | EvalException | AlreadyReportedException e) {
throw new PackageLookupFunctionException(
new RepositoryFetchException(id, e.getMessage()), Transience.PERSISTENT);
@@ -428,8 +426,14 @@
}
/**
- * Used to declare all the exception types that can be wrapped in the exception thrown by
- * {@link PackageLookupFunction#compute}.
+ * Used to declare all the exception types that can be wrapped in the exception thrown by {@link
+ * PackageLookupFunction#compute}. Note that {@link InconsistentFilesystemException} can only be
+ * thrown during target pattern parsing because of Bazel's end-to-end behavior: {@link
+ * com.google.devtools.build.lib.actions.FileStateValue} throws {@link
+ * InconsistentFilesystemException} only if a cached-on-this-evaluation directory listing said
+ * that an entry was a file but the stat had no result. However, the only time Bazel lists a
+ * directory without first accessing its BUILD/BUILD.bazel file is during evaluation of a
+ * recursive target pattern (like foo/...).
*/
private static final class PackageLookupFunctionException extends SkyFunctionException {
public PackageLookupFunctionException(BuildFileNotFoundException e, Transience transience) {
@@ -440,8 +444,8 @@
super(e, transience);
}
- public PackageLookupFunctionException(InconsistentFilesystemException e,
- Transience transience) {
+ public PackageLookupFunctionException(
+ InconsistentFilesystemException e, Transience transience) {
super(e, transience);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index 9f1e87a..eab41e0 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -27,6 +27,8 @@
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.cmdline.TargetPatternResolver;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
@@ -38,6 +40,8 @@
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.pkgcache.TargetPatternResolverUtil;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
+import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -68,7 +72,7 @@
@Nullable
@Override
public SkyValue compute(SkyKey key, Environment env)
- throws SkyFunctionException, InterruptedException {
+ throws PrepareDepsOfPatternFunctionException, InterruptedException {
TargetPatternValue.TargetPatternKey patternKey =
((TargetPatternValue.TargetPatternKey) key.argument());
@@ -78,8 +82,8 @@
// with SkyframeTargetPatternEvaluator, which can create TargetPatternKeys with other
// filtering policies like FILTER_TESTS or FILTER_MANUAL.) This check makes sure that the
// key's filtering policy is NO_FILTER as expected.
- Preconditions.checkState(patternKey.getPolicy().equals(FilteringPolicies.NO_FILTER),
- patternKey.getPolicy());
+ Preconditions.checkState(
+ patternKey.getPolicy().equals(FilteringPolicies.NO_FILTER), patternKey.getPolicy());
TargetPattern parsedPattern = patternKey.getParsedPattern();
@@ -124,6 +128,10 @@
// The DepsOfPatternPreparer constructed above might throw MissingDepException to signal
// when it has a dependency on a missing Environment value.
return null;
+ } catch (ProcessPackageDirectoryException e) {
+ throw new PrepareDepsOfPatternFunctionException(parsedPattern, e);
+ } catch (InconsistentFilesystemException e) {
+ throw new PrepareDepsOfPatternFunctionException(parsedPattern, e);
}
return PrepareDepsOfPatternValue.INSTANCE;
}
@@ -133,10 +141,58 @@
* PrepareDepsOfPatternFunction#compute}.
*/
private static final class PrepareDepsOfPatternFunctionException extends SkyFunctionException {
-
PrepareDepsOfPatternFunctionException(TargetParsingException e) {
super(e, Transience.PERSISTENT);
}
+
+ PrepareDepsOfPatternFunctionException(
+ TargetPattern pattern, ProcessPackageDirectoryException e) {
+ super(new PrepareDepsOfPatternException(pattern, e), Transience.PERSISTENT);
+ }
+
+ PrepareDepsOfPatternFunctionException(
+ TargetPattern pattern, InconsistentFilesystemException e) {
+ super(new PrepareDepsOfPatternException(pattern, e), Transience.PERSISTENT);
+ }
+ }
+
+ private static final class PrepareDepsOfPatternException extends Exception
+ implements DetailedException {
+ private final DetailedExitCode detailedExitCode;
+
+ PrepareDepsOfPatternException(TargetPattern pattern, ProcessPackageDirectoryException e) {
+ super(
+ "Preparing deps of pattern '"
+ + pattern.getOriginalPattern()
+ + "' failed: "
+ + e.getMessage(),
+ e);
+ detailedExitCode = e.getDetailedExitCode();
+ }
+
+ public PrepareDepsOfPatternException(TargetPattern pattern, InconsistentFilesystemException e) {
+ super(
+ "Preparing deps of pattern '"
+ + pattern.getOriginalPattern()
+ + "' failed: "
+ + e.getMessage(),
+ e);
+ detailedExitCode =
+ DetailedExitCode.of(
+ FailureDetails.FailureDetail.newBuilder()
+ .setMessage(getMessage())
+ .setPackageLoading(
+ FailureDetails.PackageLoading.newBuilder()
+ .setCode(
+ FailureDetails.PackageLoading.Code
+ .TRANSIENT_INCONSISTENT_FILESYSTEM_ERROR))
+ .build());
+ }
+
+ @Override
+ public DetailedExitCode getDetailedExitCode() {
+ return detailedExitCode;
+ }
}
/**
@@ -169,7 +225,7 @@
}
@Override
- public Void getTargetOrNull(Label label) throws InterruptedException {
+ public Void getTargetOrNull(Label label) {
// Note:
// This method is used in just one place, TargetPattern.TargetsInPackage#getWildcardConflict.
// Returning null tells #getWildcardConflict that there is not a target with a name like
@@ -223,14 +279,16 @@
}
return ImmutableSet.of();
} catch (NoSuchThingException e) {
- String message = TargetPatternResolverUtil.getParsingErrorMessage(
- "package contains errors", originalPattern);
+ String message =
+ TargetPatternResolverUtil.getParsingErrorMessage(
+ "package contains errors", originalPattern);
throw new TargetParsingException(message, e, e.getDetailedExitCode());
}
}
@Override
- public boolean isPackage(PackageIdentifier packageIdentifier) throws InterruptedException {
+ public boolean isPackage(PackageIdentifier packageIdentifier)
+ throws InterruptedException, InconsistentFilesystemException {
return packageProvider.isPackage(env.getListener(), packageIdentifier);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java
index 844e966..2dfad7d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryFunction.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.pkgcache.FilteringPolicy;
import com.google.devtools.build.lib.skyframe.PrepareDepsOfTargetsUnderDirectoryValue.PrepareDepsOfTargetsUnderDirectoryKey;
+import com.google.devtools.build.lib.skyframe.ProcessPackageDirectory.ProcessPackageDirectorySkyFunctionException;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
@@ -37,7 +38,8 @@
}
@Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
PrepareDepsOfTargetsUnderDirectoryKey argument =
(PrepareDepsOfTargetsUnderDirectoryKey) skyKey.argument();
final FilteringPolicy filteringPolicy = argument.getFilteringPolicy();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareTestSuitesUnderDirectoryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareTestSuitesUnderDirectoryFunction.java
index a8819d5..77ddd2d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareTestSuitesUnderDirectoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareTestSuitesUnderDirectoryFunction.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.skyframe.PrepareTestSuitesUnderDirectoryValue.Key;
+import com.google.devtools.build.lib.skyframe.ProcessPackageDirectory.ProcessPackageDirectorySkyFunctionException;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
@@ -35,7 +36,8 @@
}
@Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
Key argument = (Key) skyKey.argument();
ProcessPackageDirectory processPackageDirectory =
new ProcessPackageDirectory(directories, PrepareTestSuitesUnderDirectoryValue::key);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
index 57294aa..23712d1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionException;
import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction;
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.vfs.Dirent;
@@ -37,6 +38,7 @@
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.ValueOrException2;
import java.io.IOException;
@@ -81,13 +83,15 @@
RepositoryName repositoryName,
ImmutableSet<PathFragment> excludedPaths,
SkyFunction.Environment env)
- throws InterruptedException {
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
PathFragment rootRelativePath = rootedPath.getRootRelativePath();
SkyKey fileKey = FileValue.key(rootedPath);
FileValue fileValue;
try {
fileValue = (FileValue) env.getValueOrThrow(fileKey, IOException.class);
+ } catch (InconsistentFilesystemException e) {
+ throw new ProcessPackageDirectorySkyFunctionException(rootedPath, e);
} catch (IOException e) {
return reportErrorAndReturn(
"Failed to get information about path", e, rootRelativePath, env.getListener());
@@ -139,21 +143,13 @@
ImmutableList.of(pkgLookupKey, dirListingKey),
NoSuchPackageException.class,
IOException.class);
- if (env.valuesMissing()) {
- return null;
- }
PackageLookupValue pkgLookupValue;
try {
- pkgLookupValue =
- (PackageLookupValue)
- Preconditions.checkNotNull(
- pkgLookupAndDirectoryListingDeps.get(0).get(),
- "%s %s %s",
- rootedPath,
- repositoryName,
- pkgLookupKey);
- } catch (NoSuchPackageException | InconsistentFilesystemException e) {
+ pkgLookupValue = (PackageLookupValue) pkgLookupAndDirectoryListingDeps.get(0).get();
+ } catch (NoSuchPackageException e) {
return reportErrorAndReturn("Failed to load package", e, rootRelativePath, env.getListener());
+ } catch (InconsistentFilesystemException e) {
+ throw new ProcessPackageDirectorySkyFunctionException(rootedPath, e);
} catch (IOException e) {
throw new IllegalStateException(e);
}
@@ -161,12 +157,7 @@
try {
dirListingValue =
(DirectoryListingValue)
- Preconditions.checkNotNull(
- pkgLookupAndDirectoryListingDeps.get(1).getOrThrow(IOException.class),
- "%s %s %s",
- rootedPath,
- repositoryName,
- dirListingKey);
+ pkgLookupAndDirectoryListingDeps.get(1).getOrThrow(IOException.class);
} catch (FileSymlinkException e) {
// DirectoryListingFunction only throws FileSymlinkCycleException when FileFunction throws it,
// but FileFunction was evaluated for rootedPath above, and didn't throw there. It shouldn't
@@ -181,6 +172,10 @@
if (env.valuesMissing()) {
return null;
}
+ Preconditions.checkNotNull(
+ pkgLookupValue, "%s %s %s", rootedPath, repositoryName, pkgLookupKey);
+ Preconditions.checkNotNull(
+ dirListingValue, "%s %s %s", rootedPath, repositoryName, dirListingKey);
return new ProcessPackageDirectoryResult(
pkgLookupValue.packageExists() && pkgLookupValue.getRoot().equals(rootedPath.getRoot()),
getSubdirDeps(
@@ -277,4 +272,18 @@
}
return true;
}
+
+ /** Wraps {@link InconsistentFilesystemException} in {@link ProcessPackageDirectoryException}. */
+ public static final class ProcessPackageDirectorySkyFunctionException
+ extends SkyFunctionException {
+ private ProcessPackageDirectorySkyFunctionException(
+ RootedPath directory, InconsistentFilesystemException e) {
+ super(new ProcessPackageDirectoryException(directory, e), Transience.PERSISTENT);
+ }
+
+ @Override
+ public boolean isCatastrophic() {
+ return true;
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java
index 4d2fdc4..da3be2a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProgressEventSuppressingEnvironment.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.util.GroupedList;
@@ -283,9 +282,8 @@
}
@Override
- @VisibleForTesting
- public boolean inErrorBubblingForTesting() {
- return delegate.inErrorBubblingForTesting();
+ public boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors() {
+ return delegate.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index 2fcf29b..130e78b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -23,11 +23,13 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.skyframe.ProcessPackageDirectory.ProcessPackageDirectorySkyFunctionException;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.errorprone.annotations.ForOverride;
import java.util.Map;
import javax.annotation.Nullable;
@@ -35,6 +37,15 @@
* RecursiveDirectoryTraversalFunction allows for a custom recursive traversal of the subdirectories
* of a directory, building up a value based on package existence and results of the recursive
* traversal.
+ *
+ * <p>It attempts to ignore package-definition-related errors, and even file symlink cycles, which
+ * means that in keep-going mode, it will produce a result even if the traversed directory contains
+ * such errors. In no-keep-going mode, such exceptions will shut down the build, so callers must be
+ * prepared to handle {@link com.google.devtools.build.lib.packages.NoSuchPackageException} and
+ * {@link com.google.devtools.build.lib.io.FileSymlinkException}.
+ *
+ * <p>It will always eagerly fail on exceptions indicating filesystem inconsistencies, since they
+ * indicate bad disk that may make results unreliable.
*/
public abstract class RecursiveDirectoryTraversalFunction<
ConsumerT extends RecursiveDirectoryTraversalFunction.PackageDirectoryConsumer, ReturnT> {
@@ -45,9 +56,11 @@
}
/** Called by {@link #visitDirectory}, which will then recursive traverse the directory. */
+ @ForOverride
@Nullable
protected ProcessPackageDirectoryResult getProcessPackageDirectoryResult(
- RecursivePkgKey recursivePkgKey, Environment env) throws InterruptedException {
+ RecursivePkgKey recursivePkgKey, Environment env)
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
return new ProcessPackageDirectory(directories, this::getSkyKeyForSubdirectory)
.getPackageExistenceAndSubdirDeps(
recursivePkgKey.getRootedPath(),
@@ -117,10 +130,13 @@
*
* <p>Returns null if {@code env.valuesMissing()} is true, checked after each call to one of
* {@link RecursiveDirectoryTraversalFunction}'s abstract methods that were given {@code env}.
+ *
+ * <p>Will propagate {@link com.google.devtools.build.lib.packages.NoSuchPackageException} during
+ * a no-keep-going evaluation
*/
@Nullable
public final ReturnT visitDirectory(RecursivePkgKey recursivePkgKey, Environment env)
- throws InterruptedException {
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
ProcessPackageDirectoryResult processPackageDirectoryResult =
getProcessPackageDirectoryResult(recursivePkgKey, env);
if (env.valuesMissing()) {
@@ -136,6 +152,9 @@
SkyKey packageErrorMessageKey =
PackageErrorMessageValue.key(
PackageIdentifier.create(recursivePkgKey.getRepositoryName(), rootRelativePath));
+ // In a no-keep-going build during error bubbling, PackageErrorMessageFunction may throw a
+ // NoSuchPackageException. Since we don't catch such an exception here, this SkyFunction will
+ // return immediately with a missing value, and the NoSuchPackageException will propagate up.
Map<SkyKey, SkyValue> dependentSkyValues =
env.getValues(Iterables.concat(childDeps, ImmutableList.of(packageErrorMessageKey)));
if (env.valuesMissing()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index e9abf1b..56a5a59 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.util.concurrent.Futures.immediateCancelledFuture;
+import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
import com.google.common.base.Throwables;
@@ -24,6 +26,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
+import com.google.common.util.concurrent.Uninterruptibles;
import com.google.devtools.build.lib.cmdline.BatchCallback;
import com.google.devtools.build.lib.cmdline.BatchCallback.SafeBatchCallback;
import com.google.devtools.build.lib.cmdline.Label;
@@ -37,6 +40,8 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
@@ -54,12 +59,11 @@
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
-/**
- * A {@link TargetPatternResolver} backed by a {@link RecursivePackageProvider}.
- */
+/** A {@link TargetPatternResolver} backed by a {@link RecursivePackageProvider}. */
@ThreadCompatible
-public class RecursivePackageProviderBackedTargetPatternResolver
+public final class RecursivePackageProviderBackedTargetPatternResolver
extends TargetPatternResolver<Target> {
// TODO(janakr): Move this to a more generic place and unify with SkyQueryEnvironment's value?
@@ -99,12 +103,13 @@
}
private Map<PackageIdentifier, Package> bulkGetPackages(Iterable<PackageIdentifier> pkgIds)
- throws NoSuchPackageException, InterruptedException {
+ throws NoSuchPackageException, InterruptedException {
return recursivePackageProvider.bulkGetPackages(pkgIds);
}
@Override
- public Target getTargetOrNull(Label label) throws InterruptedException {
+ public Target getTargetOrNull(Label label)
+ throws InterruptedException, InconsistentFilesystemException {
try {
if (!isPackage(label.getPackageIdentifier())) {
return null;
@@ -132,15 +137,14 @@
public Collection<Target> getTargetsInPackage(
String originalPattern, PackageIdentifier packageIdentifier, boolean rulesOnly)
throws TargetParsingException, InterruptedException {
- FilteringPolicy actualPolicy = rulesOnly
- ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy)
- : policy;
+ FilteringPolicy actualPolicy =
+ rulesOnly ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy) : policy;
try {
Package pkg = getPackage(packageIdentifier);
return TargetPatternResolverUtil.resolvePackageTargets(pkg, actualPolicy);
} catch (NoSuchThingException e) {
- String message = TargetPatternResolverUtil.getParsingErrorMessage(
- e.getMessage(), originalPattern);
+ String message =
+ TargetPatternResolverUtil.getParsingErrorMessage(e.getMessage(), originalPattern);
throw new TargetParsingException(message, e, e.getDetailedExitCode());
}
}
@@ -151,25 +155,27 @@
try {
Map<PackageIdentifier, Package> pkgs = bulkGetPackages(pkgIds);
if (pkgs.size() != Iterables.size(pkgIds)) {
- throw new IllegalStateException("Bulk package retrieval missing results: "
- + Sets.difference(ImmutableSet.copyOf(pkgIds), pkgs.keySet()));
+ throw new IllegalStateException(
+ "Bulk package retrieval missing results: "
+ + Sets.difference(ImmutableSet.copyOf(pkgIds), pkgs.keySet()));
}
ImmutableMap.Builder<PackageIdentifier, Collection<Target>> result = ImmutableMap.builder();
for (PackageIdentifier pkgId : pkgIds) {
Package pkg = pkgs.get(pkgId);
- result.put(pkgId, TargetPatternResolverUtil.resolvePackageTargets(pkg, policy));
+ result.put(pkgId, TargetPatternResolverUtil.resolvePackageTargets(pkg, policy));
}
return result.build();
} catch (NoSuchThingException e) {
- String message = TargetPatternResolverUtil.getParsingErrorMessage(
- e.getMessage(), originalPattern);
+ String message =
+ TargetPatternResolverUtil.getParsingErrorMessage(e.getMessage(), originalPattern);
throw new IllegalStateException(
"Mismatch: Expected given pkgIds to correspond to valid Packages. " + message, e);
}
}
@Override
- public boolean isPackage(PackageIdentifier packageIdentifier) throws InterruptedException {
+ public boolean isPackage(PackageIdentifier packageIdentifier)
+ throws InterruptedException, InconsistentFilesystemException {
return recursivePackageProvider.isPackage(eventHandler, packageIdentifier);
}
@@ -188,9 +194,11 @@
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<Target, E> callback,
Class<E> exceptionClass)
- throws TargetParsingException, E, InterruptedException {
+ throws TargetParsingException, E, InterruptedException, ProcessPackageDirectoryException {
+ ListenableFuture<Void> future;
try {
- findTargetsBeneathDirectoryAsyncImpl(
+ future =
+ findTargetsBeneathDirectoryAsyncImpl(
repository,
originalPattern,
directory,
@@ -198,11 +206,27 @@
forbiddenSubdirectories,
excludedSubdirectories,
callback,
- MoreExecutors.newDirectExecutorService())
- .get();
- } catch (ExecutionException e) {
- Throwables.propagateIfPossible(e.getCause(), TargetParsingException.class, exceptionClass);
- throw new IllegalStateException(e.getCause());
+ MoreExecutors.newDirectExecutorService());
+ } catch (QueryException e) {
+ Throwables.propagateIfPossible(e, exceptionClass);
+ throw new IllegalStateException(e);
+ } catch (NoSuchPackageException e) {
+ // Can happen during a Skyframe no-keep-going evaluation.
+ throw new TargetParsingException(
+ "error loading package under directory '" + directory + "': " + e.getMessage(),
+ e,
+ e.getDetailedExitCode());
+ }
+ if (!isSuccessful(future)) {
+ // Don't get the future if it finished successfully: all that will do is throw an
+ // interrupted exception if this thread was interrupted, but that's not helpful for a done
+ // future.
+ try {
+ future.get();
+ } catch (ExecutionException e) {
+ Throwables.propagateIfPossible(e.getCause(), InterruptedException.class, exceptionClass);
+ throw new IllegalStateException(e.getCause());
+ }
}
}
@@ -218,17 +242,39 @@
BatchCallback<Target, E> callback,
Class<E> exceptionClass,
ListeningExecutorService executor) {
- return findTargetsBeneathDirectoryAsyncImpl(
- repository,
- originalPattern,
- directory,
- rulesOnly,
- forbiddenSubdirectories,
- excludedSubdirectories,
- callback,
- executor);
+ try {
+ return findTargetsBeneathDirectoryAsyncImpl(
+ repository,
+ originalPattern,
+ directory,
+ rulesOnly,
+ forbiddenSubdirectories,
+ excludedSubdirectories,
+ callback,
+ executor);
+ } catch (TargetParsingException e) {
+ return immediateFailedFuture(e);
+ } catch (InterruptedException e) {
+ return immediateCancelledFuture();
+ } catch (ProcessPackageDirectoryException | NoSuchPackageException e) {
+ throw new IllegalStateException(
+ "Async find targets beneath directory isn't called from within Skyframe: traversing "
+ + directory
+ + " for "
+ + originalPattern,
+ e);
+ } catch (QueryException e) {
+ if (exceptionClass.isInstance(e)) {
+ return immediateFailedFuture(e);
+ }
+ throw new IllegalStateException(e);
+ }
}
+ /**
+ * The returned future may throw {@link QueryException} (if {@code E} is {@link QueryException})
+ * or {@link InterruptedException} on retrieval, but no other exceptions.
+ */
private <E extends Exception & QueryExceptionMarkerInterface>
ListenableFuture<Void> findTargetsBeneathDirectoryAsyncImpl(
RepositoryName repository,
@@ -238,7 +284,9 @@
ImmutableSet<PathFragment> forbiddenSubdirectories,
ImmutableSet<PathFragment> excludedSubdirectories,
BatchCallback<Target, E> callback,
- ListeningExecutorService executor) {
+ ListeningExecutorService executor)
+ throws TargetParsingException, QueryException, InterruptedException,
+ ProcessPackageDirectoryException, NoSuchPackageException {
FilteringPolicy actualPolicy =
rulesOnly ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy) : policy;
@@ -249,11 +297,10 @@
executor.submit(
new GetTargetsInPackagesTask<>(pkgIdBatch, pattern, actualPolicy, callback)));
- PathFragment pathFragment;
+ PathFragment pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
try (PackageIdentifierBatchingCallback batchingCallback =
packageIdentifierBatchingCallbackFactory.create(
getPackageTargetsCallback, MAX_PACKAGES_BULK_GET)) {
- pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
recursivePackageProvider.streamPackagesUnderDirectory(
batchingCallback,
eventHandler,
@@ -261,19 +308,11 @@
pathFragment,
forbiddenSubdirectories,
excludedSubdirectories);
- } catch (TargetParsingException | QueryException e) {
- return Futures.immediateFailedFuture(e);
- } catch (InterruptedException e) {
- return Futures.immediateCancelledFuture();
}
-
if (futures.isEmpty()) {
- return Futures.immediateFailedFuture(
- new TargetParsingException(
- "no targets found beneath '" + pathFragment + "'",
- TargetPatterns.Code.TARGETS_MISSING));
+ throw new TargetParsingException(
+ "no targets found beneath '" + pathFragment + "'", TargetPatterns.Code.TARGETS_MISSING);
}
-
return Futures.whenAllSucceed(futures).call(() -> null, directExecutor());
}
@@ -301,7 +340,7 @@
}
@Override
- public Void call() throws Exception {
+ public Void call() throws E, InterruptedException {
ImmutableSet<PackageIdentifier> pkgIdBatchSet = ImmutableSet.copyOf(packageIdentifiers);
packageSemaphore.acquireAll(pkgIdBatchSet);
try {
@@ -336,5 +375,17 @@
}
return size;
}
-}
+ /** Inspired by not-yet-open-source futures code. */
+ private static boolean isSuccessful(Future<?> future) {
+ if (future.isDone() && !future.isCancelled()) {
+ try {
+ Uninterruptibles.getUninterruptibly(future);
+ return true;
+ } catch (ExecutionException | RuntimeException e) {
+ // Fall through.
+ }
+ }
+ return false;
+ }
+}
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 e7881f1..c569c59 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
@@ -19,6 +19,7 @@
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.skyframe.ProcessPackageDirectory.ProcessPackageDirectorySkyFunctionException;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
@@ -27,8 +28,7 @@
import java.util.Map;
/**
- * RecursivePkgFunction builds up the set of packages underneath a given directory
- * transitively.
+ * RecursivePkgFunction builds up the set of packages underneath a given directory transitively.
*
* <p>Example: foo/BUILD, foo/sub/x, foo/subpkg/BUILD would yield transitive packages "foo" and
* "foo/subpkg".
@@ -45,7 +45,8 @@
* in nokeep_going mode!
*/
@Override
- public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+ public SkyValue compute(SkyKey skyKey, Environment env)
+ throws InterruptedException, ProcessPackageDirectorySkyFunctionException {
return new MyTraversalFunction().visitDirectory((RecursivePkgKey) skyKey.argument(), env);
}
@@ -62,7 +63,9 @@
}
@Override
- protected SkyKey getSkyKeyForSubdirectory(RepositoryName repository, RootedPath subdirectory,
+ protected SkyKey getSkyKeyForSubdirectory(
+ RepositoryName repository,
+ RootedPath subdirectory,
ImmutableSet<PathFragment> excludedSubdirectoriesBeneathSubdirectory) {
return RecursivePkgValue.key(
repository, subdirectory, excludedSubdirectoriesBeneathSubdirectory);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java
index 912c132..ad536cd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctionEnvironmentForTesting.java
@@ -74,7 +74,7 @@
}
@Override
- public boolean inErrorBubblingForTesting() {
+ public boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors() {
return false;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index dcf4ccc..ce43277 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -21,6 +21,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.CachingPackageLocator;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -77,7 +78,8 @@
}
@Override
- public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName) {
+ public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
+ throws InconsistentFilesystemException, InterruptedException {
return getBuildFileForPackage(packageName) != null;
}
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 e6437fd..b95b4be 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
@@ -14,8 +14,10 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Preconditions;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.QueryExceptionMarkerInterface;
import com.google.devtools.build.lib.cmdline.ResolvedTargets;
@@ -24,6 +26,9 @@
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.FilteringPolicies;
@@ -79,6 +84,14 @@
EvaluationResult<SkyValue> result =
skyframeExecutor.targetPatterns(
allKeys, SkyframeExecutor.DEFAULT_THREAD_COUNT, keepGoing, eventHandler);
+ Exception catastrophe = result.getCatastrophe();
+ Throwables.propagateIfPossible(catastrophe, TargetParsingException.class);
+ if (catastrophe != null) {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ "Non-TargetParsingException catastrophe for " + result + " and " + patterns,
+ catastrophe));
+ }
WalkableGraph walkableGraph = Preconditions.checkNotNull(result.getWalkableGraph(), result);
for (PatternLookup patternLookup : patternLookups) {
SkyKey key = patternLookup.skyKey;
@@ -106,17 +119,31 @@
String errorMessage;
TargetParsingException targetParsingException;
if (error.getException() != null) {
- // This exception could be a TargetParsingException, a NoSuchPackageException, or
- // potentially a lower-level exception. If the exception is the former two, then the
- // DetailedExitCode can be extracted from the exception.
+ // This exception could be a TargetParsingException for a target pattern, a
+ // NoSuchPackageException for a label (or package wildcard), or potentially a lower-level
+ // exception if there is a bug in error handling.
Exception exception = error.getException();
errorMessage = exception.getMessage();
- targetParsingException =
- DetailedException.getDetailedExitCode(exception) != null
- ? new TargetParsingException(
- errorMessage, exception, DetailedException.getDetailedExitCode(exception))
- : new TargetParsingException(
- errorMessage, TargetPatterns.Code.CANNOT_PRELOAD_TARGET);
+ if (exception instanceof TargetParsingException) {
+ targetParsingException = (TargetParsingException) exception;
+ } else if (key instanceof PackageValue.Key
+ && exception instanceof NoSuchPackageException) {
+ targetParsingException =
+ new TargetParsingException(
+ errorMessage,
+ exception,
+ ((NoSuchPackageException) exception).getDetailedExitCode());
+ } else {
+ BugReport.sendBugReport(
+ new IllegalStateException(
+ "Unexpected exception when processing " + key, exception));
+ targetParsingException =
+ DetailedException.getDetailedExitCode(exception) != null
+ ? new TargetParsingException(
+ errorMessage, exception, DetailedException.getDetailedExitCode(exception))
+ : new TargetParsingException(
+ errorMessage, TargetPatterns.Code.CANNOT_PRELOAD_TARGET);
+ }
} else if (!error.getCycleInfo().isEmpty()) {
errorMessage = "cycles detected during target parsing";
targetParsingException =
@@ -219,7 +246,7 @@
SkyValue value,
WalkableGraph walkableGraph,
boolean keepGoing)
- throws InterruptedException {
+ throws InterruptedException, TargetParsingException {
TargetPatternValue resultValue = (TargetPatternValue) value;
ResolvedTargets<Label> results = resultValue.getTargets();
resultBuilder.addLabelsOfPositivePattern(results);
@@ -257,16 +284,21 @@
/* packageSemaphore= */ null,
SimplePackageIdentifierBatchingCallback::new);
AtomicReference<Collection<Target>> result = new AtomicReference<>();
- targetPattern.eval(
- resolver,
- /*ignoredSubdirectories=*/ ImmutableSet::of,
- /*excludedSubdirectories=*/ ImmutableSet.of(),
- partialResult ->
- result.set(
- partialResult instanceof Collection
- ? (Collection<Target>) partialResult
- : ImmutableSet.copyOf(partialResult)),
- QueryExceptionMarkerInterface.MarkerRuntimeException.class);
+ try {
+ targetPattern.eval(
+ resolver,
+ /*ignoredSubdirectories=*/ ImmutableSet::of,
+ /*excludedSubdirectories=*/ ImmutableSet.of(),
+ partialResult ->
+ result.set(
+ partialResult instanceof Collection
+ ? (Collection<Target>) partialResult
+ : ImmutableSet.copyOf(partialResult)),
+ QueryExceptionMarkerInterface.MarkerRuntimeException.class);
+ } catch (ProcessPackageDirectoryException | InconsistentFilesystemException e) {
+ throw new IllegalStateException(
+ "PackageBackedRecursivePackageProvider doesn't throw for " + targetPattern, e);
+ }
return result.get();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
index 0cbb8ed..84315fc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StateInformingSkyFunctionEnvironment.java
@@ -149,7 +149,7 @@
throws InterruptedException {
preFetch.inform();
try {
- return delegate.getValues(depKeys);
+ return delegate.getValues(depKeys);
} finally {
postFetch.inform();
}
@@ -352,8 +352,8 @@
}
@Override
- public boolean inErrorBubblingForTesting() {
- return delegate.inErrorBubblingForTesting();
+ public boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors() {
+ return delegate.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors();
}
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
index 23d554f..dadf241 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternErrorFunction.java
@@ -16,9 +16,8 @@
import com.google.common.collect.Interner;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
-import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.skyframe.AbstractSkyKey;
+import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
@@ -29,53 +28,67 @@
/**
* SkyFunction that throws a {@link TargetParsingException} for target pattern that could not be
- * parsed. Must only be requested when a SkyFunction wishes to ignore the errors
- * in a target pattern in keep_going mode, but to shut down the build in nokeep_going mode.
+ * parsed. Must only be requested when a SkyFunction wishes to ignore the errors in a target pattern
+ * in keep_going mode, but to shut down the build in nokeep_going mode.
*
* <p>This SkyFunction never returns a value, only throws a {@link TargetParsingException}, and
* should never return null, since all of its dependencies should already be present.
*/
public class TargetPatternErrorFunction implements SkyFunction {
- // We pass in the error message, which isn't ideal. We could consider reparsing the original
- // pattern instead, but that requires more information.
- public static Key key(String errorMessage) {
- return Key.create(errorMessage);
+ public static Key key(TargetParsingException e) {
+ return Key.create(e.getMessage(), e.getDetailedExitCode());
}
@AutoCodec.VisibleForSerialization
@AutoCodec
- static class Key extends AbstractSkyKey<String> {
+ static class Key implements SkyKey {
private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
+ private final String message;
+ private final DetailedExitCode detailedExitCode;
- private Key(String arg) {
- super(arg);
+ private Key(String message, DetailedExitCode detailedExitCode) {
+ this.message = message;
+ this.detailedExitCode = detailedExitCode;
}
@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
- static Key create(String arg) {
- return interner.intern(new Key(arg));
+ static Key create(String message, DetailedExitCode detailedExitCode) {
+ return interner.intern(new Key(message, detailedExitCode));
}
@Override
public SkyFunctionName functionName() {
return SkyFunctions.TARGET_PATTERN_ERROR;
}
+
+ @Override
+ public int hashCode() {
+ return 43 * message.hashCode() + detailedExitCode.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (!(obj instanceof Key)) {
+ return false;
+ }
+ Key that = (Key) obj;
+ return this.message.equals(that.message)
+ && this.detailedExitCode.equals(that.detailedExitCode);
+ }
}
@Nullable
@Override
public SkyValue compute(SkyKey skyKey, Environment env)
throws TargetErrorFunctionException, InterruptedException {
- String errorMessage = (String) skyKey.argument();
throw new TargetErrorFunctionException(
- new TargetParsingException(errorMessage, TargetPatterns.Code.TARGET_PATTERN_PARSE_FAILURE),
+ new TargetParsingException(((Key) skyKey).message, ((Key) skyKey).detailedExitCode),
Transience.PERSISTENT);
}
private static class TargetErrorFunctionException extends SkyFunctionException {
- public TargetErrorFunctionException(
- TargetParsingException cause, Transience transience) {
+ public TargetErrorFunctionException(TargetParsingException cause, Transience transience) {
super(cause, transience);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
index 53aa19f..5284f56 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java
@@ -22,10 +22,14 @@
import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.cmdline.TargetPattern;
import com.google.devtools.build.lib.concurrent.MultisetSemaphore;
+import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.AbstractRecursivePackageProvider.MissingDepException;
import com.google.devtools.build.lib.pkgcache.ParsingFailedEvent;
+import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -33,17 +37,15 @@
import com.google.devtools.build.skyframe.SkyValue;
/**
- * TargetPatternFunction translates a target pattern (eg, "foo/...") into a set of resolved
- * Targets.
+ * TargetPatternFunction translates a target pattern (eg, "foo/...") into a set of resolved Targets.
*/
public class TargetPatternFunction implements SkyFunction {
- public TargetPatternFunction() {
- }
+ public TargetPatternFunction() {}
@Override
- public SkyValue compute(SkyKey key, Environment env) throws TargetPatternFunctionException,
- InterruptedException {
+ public SkyValue compute(SkyKey key, Environment env)
+ throws TargetPatternFunctionException, InterruptedException {
TargetPatternValue.TargetPatternKey patternKey =
((TargetPatternValue.TargetPatternKey) key.argument());
TargetPattern parsedPattern = patternKey.getParsedPattern();
@@ -57,9 +59,9 @@
ImmutableSet<PathFragment> ignoredPatterns = ignoredPackagePrefixes.getPatterns();
ResolvedTargets<Target> resolvedTargets;
+ EnvironmentBackedRecursivePackageProvider provider =
+ new EnvironmentBackedRecursivePackageProvider(env);
try {
- EnvironmentBackedRecursivePackageProvider provider =
- new EnvironmentBackedRecursivePackageProvider(env);
RecursivePackageProviderBackedTargetPatternResolver resolver =
new RecursivePackageProviderBackedTargetPatternResolver(
provider,
@@ -87,20 +89,29 @@
resolvedTargetsBuilder.add(target);
}
};
- parsedPattern.eval(
- resolver,
- () -> ignoredPatterns,
- excludedSubdirectories,
- callback,
- QueryExceptionMarkerInterface.MarkerRuntimeException.class);
+ try {
+ parsedPattern.eval(
+ resolver,
+ () -> ignoredPatterns,
+ excludedSubdirectories,
+ callback,
+ QueryExceptionMarkerInterface.MarkerRuntimeException.class);
+ } catch (ProcessPackageDirectoryException e) {
+ throw new TargetPatternFunctionException(e);
+ } catch (InconsistentFilesystemException e) {
+ throw new TargetPatternFunctionException(e);
+ }
if (provider.encounteredPackageErrors()) {
resolvedTargetsBuilder.setError();
}
resolvedTargets = resolvedTargetsBuilder.build();
} catch (TargetParsingException e) {
- env.getListener().post(new ParsingFailedEvent(patternKey.getPattern(), e.getMessage()));
+ env.getListener().post(new ParsingFailedEvent(patternKey.getPattern(), e.getMessage()));
throw new TargetPatternFunctionException(e);
} catch (MissingDepException e) {
+ // If there is at least one missing dependency, we should eagerly throw any exception we might
+ // have encountered: if we are in error bubbling, this could be our last chance.
+ maybeThrowEncounteredException(parsedPattern, provider);
// The EnvironmentBackedRecursivePackageProvider constructed above might throw
// MissingDepException to signal when it has a dependency on a missing Environment value.
// Note that MissingDepException extends RuntimeException because the methods called
@@ -108,6 +119,9 @@
// implementations that are unconcerned with MissingDepExceptions.
return null;
}
+ if (env.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors()) {
+ maybeThrowEncounteredException(parsedPattern, provider);
+ }
Preconditions.checkNotNull(resolvedTargets, key);
ResolvedTargets.Builder<Label> resolvedLabelsBuilder = ResolvedTargets.builder();
if (resolvedTargets.hasError()) {
@@ -122,13 +136,42 @@
return new TargetPatternValue(resolvedLabelsBuilder.build());
}
- /**
- * Used to declare all the exception types that can be wrapped in the exception thrown by
- * {@link TargetPatternFunction#compute}.
- */
+ private static void maybeThrowEncounteredException(
+ TargetPattern pattern, EnvironmentBackedRecursivePackageProvider provider)
+ throws TargetPatternFunctionException {
+ NoSuchPackageException e = provider.maybeGetNoSuchPackageException();
+ if (e != null) {
+ throw new TargetPatternFunctionException(
+ new TargetParsingException(
+ "Error evaluating '" + pattern.getOriginalPattern() + "': " + e.getMessage(),
+ e,
+ FailureDetails.TargetPatterns.Code.PACKAGE_NOT_FOUND));
+ }
+ }
+
private static final class TargetPatternFunctionException extends SkyFunctionException {
- public TargetPatternFunctionException(TargetParsingException e) {
+ private final boolean isCatastrophic;
+
+ TargetPatternFunctionException(TargetParsingException e) {
super(e, Transience.PERSISTENT);
+ this.isCatastrophic = false;
+ }
+
+ TargetPatternFunctionException(InconsistentFilesystemException e) {
+ super(new TargetParsingException(e), Transience.PERSISTENT);
+ this.isCatastrophic = true;
+ }
+
+ TargetPatternFunctionException(ProcessPackageDirectoryException e) {
+ super(
+ new TargetParsingException(e.getMessage(), e, e.getDetailedExitCode()),
+ Transience.PERSISTENT);
+ this.isCatastrophic = true;
+ }
+
+ @Override
+ public boolean isCatastrophic() {
+ return isCatastrophic;
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index b1168ec..df7bc71 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -92,7 +92,7 @@
}
// Determine targets to build:
- List<String> failedPatterns = new ArrayList<String>();
+ List<String> failedPatterns = new ArrayList<>();
List<ExpandedPattern> expandedPatterns =
getTargetsToBuild(
env, options, repositoryMappingValue.getRepositoryMapping(), failedPatterns);
@@ -155,10 +155,11 @@
testFilteredTargets = ImmutableSet.copyOf(allFilteredTargets);
filteredTargets = ImmutableSet.of();
- targets = ResolvedTargets.<Target>builder()
- .merge(testTargets)
- .mergeError(targets.hasError())
- .build();
+ targets =
+ ResolvedTargets.<Target>builder()
+ .merge(testTargets)
+ .mergeError(targets.hasError())
+ .build();
if (options.getDetermineTests()) {
testsToRun = testTargets.getTargets();
}
@@ -179,10 +180,11 @@
if (testsToRun != null) {
// Note that testsToRun can still be null here, if buildTestsOnly && !shouldRunTests.
if (!targets.getTargets().containsAll(testsToRun)) {
- throw new IllegalStateException(String.format(
- "Internal consistency check failed; some targets are scheduled for test execution "
- + "but not for building (%s)",
- Sets.difference(testsToRun, targets.getTargets())));
+ throw new IllegalStateException(
+ String.format(
+ "Internal consistency check failed; some targets are scheduled for test execution"
+ + " but not for building (%s)",
+ Sets.difference(testsToRun, targets.getTargets())));
}
}
}
@@ -258,9 +260,13 @@
ExtendedEventHandler eventHandler, Collection<Target> targets) {
for (Rule rule : Iterables.filter(targets, Rule.class)) {
if (rule.isAttributeValueExplicitlySpecified("deprecation")) {
- eventHandler.handle(Event.warn(rule.getLocation(), String.format(
- "target '%s' is deprecated: %s", rule.getLabel(),
- NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING))));
+ eventHandler.handle(
+ Event.warn(
+ rule.getLocation(),
+ String.format(
+ "target '%s' is deprecated: %s",
+ rule.getLabel(),
+ NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING))));
}
}
}
@@ -301,8 +307,7 @@
// pattern could be parsed successfully).
env.getListener().post(new ParsingFailedEvent(pattern, e.getMessage()));
try {
- env.getValueOrThrow(
- TargetPatternErrorFunction.key(e.getMessage()), TargetParsingException.class);
+ env.getValueOrThrow(TargetPatternErrorFunction.key(e), TargetParsingException.class);
} catch (TargetParsingException ignore) {
// We ignore this. Keep going is active.
}
@@ -360,9 +365,8 @@
}
}
- ResolvedTargets<Target> result = builder
- .filter(TargetUtils.tagFilter(options.getBuildTargetFilter()))
- .build();
+ ResolvedTargets<Target> result =
+ builder.filter(TargetUtils.tagFilter(options.getBuildTargetFilter())).build();
if (options.getCompileOneDependency()) {
EnvironmentBackedRecursivePackageProvider environmentBackedRecursivePackageProvider =
new EnvironmentBackedRecursivePackageProvider(env);
@@ -374,8 +378,7 @@
return null;
} catch (TargetParsingException e) {
try {
- env.getValueOrThrow(
- TargetPatternErrorFunction.key(e.getMessage()), TargetParsingException.class);
+ env.getValueOrThrow(TargetPatternErrorFunction.key(e), TargetParsingException.class);
} catch (TargetParsingException ignore) {
// We ignore this. Keep going is active.
}
@@ -476,17 +479,13 @@
private static ImmutableSetMultimap<String, Label> mapOriginalPatternsToLabels(
List<ExpandedPattern> expandedPatterns, Set<Target> includedTargets) {
- return expandedPatterns
- .stream()
+ return expandedPatterns.stream()
.filter(expansion -> !expansion.pattern().isNegative())
.collect(
flatteningToImmutableSetMultimap(
expansion -> expansion.pattern().getPattern(),
expansion ->
- expansion
- .resolvedTargets()
- .getTargets()
- .stream()
+ expansion.resolvedTargets().getTargets().stream()
.filter(includedTargets::contains)
.map(Target::getLabel)));
}
@@ -500,6 +499,7 @@
}
abstract TargetPatternKey pattern();
+
abstract ResolvedTargets<Target> resolvedTargets();
}
}
