Failures early in package loading will now fail all --keep_going builds.
When loading all packages under a directory (//foo/...) we use RecursivePkgFunction,
which in --keep_going mode was silently ignoring any NoSuchPackageExceptions thrown
by PackageFunction. Critically, any exceptions thrown by loading a Starlark .bzl file
were being ignored during loading.
RecursivePkgFunction now transitively collects presence of errors into the
RecursivePkgValue so callers (EnvironmentBackedRecursivePackageProvider)
can observe and record that errors happened during loading.
Fixes #7674.
RELNOTES:
None.
PiperOrigin-RevId: 251298005
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 8e10e71..d0c5f62 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
@@ -74,11 +74,17 @@
public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
throws NoSuchPackageException, MissingDepException, InterruptedException {
SkyKey pkgKey = PackageValue.key(packageName);
- PackageValue pkgValue =
- (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
- if (pkgValue == null) {
- throw new MissingDepException();
+ PackageValue pkgValue;
+ try {
+ pkgValue = (PackageValue) env.getValueOrThrow(pkgKey, NoSuchPackageException.class);
+ if (pkgValue == null) {
+ throw new MissingDepException();
+ }
+ } catch (NoSuchPackageException e) {
+ encounteredPackageErrors.set(true);
+ throw e;
}
+
Package pkg = pkgValue.getPackage();
if (pkg.containsErrors()) {
// If this is a nokeep_going build, we must shut the build down by throwing an exception. To
@@ -187,6 +193,9 @@
// the exception types it can accept.
throw new MissingDepException();
}
+ if (lookup.hasErrors()) {
+ encounteredPackageErrors.set(true);
+ }
for (String packageName : lookup.getPackages()) {
// TODO(bazel-team): Make RecursivePkgValue return NestedSet<PathFragment> so this transform
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 2de3bce..9476606 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
@@ -73,6 +73,9 @@
// Aggregate the transitive subpackages.
for (SkyValue childValue : subdirectorySkyValues.values()) {
consumer.addTransitivePackages(((RecursivePkgValue) childValue).getPackages());
+ if (((RecursivePkgValue) childValue).hasErrors()) {
+ consumer.addTransitiveErrors();
+ }
}
return consumer.createRecursivePkgValue();
}
@@ -82,6 +85,7 @@
implements RecursiveDirectoryTraversalFunction.PackageDirectoryConsumer {
private final NestedSetBuilder<String> packages = new NestedSetBuilder<>(Order.STABLE_ORDER);
+ private boolean hasErrors = false;
@Override
public void notePackage(PathFragment pkgPath) {
@@ -90,16 +94,19 @@
@Override
public void notePackageError(String noSuchPackageExceptionErrorMessage) {
- // Nothing to do because the RecursiveDirectoryTraversalFunction has already emitted an error
- // event.
+ hasErrors = true;
}
void addTransitivePackages(NestedSet<String> transitivePackages) {
packages.addTransitive(transitivePackages);
}
+ void addTransitiveErrors() {
+ hasErrors = true;
+ }
+
RecursivePkgValue createRecursivePkgValue() {
- return RecursivePkgValue.create(packages);
+ return RecursivePkgValue.create(packages, hasErrors);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
index d864c02..0fd722f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
@@ -37,19 +37,21 @@
public class RecursivePkgValue implements SkyValue {
@AutoCodec
static final RecursivePkgValue EMPTY =
- new RecursivePkgValue(NestedSetBuilder.<String>emptySet(Order.STABLE_ORDER));
+ new RecursivePkgValue(NestedSetBuilder.<String>emptySet(Order.STABLE_ORDER), false);
private final NestedSet<String> packages;
+ private final boolean hasErrors;
- private RecursivePkgValue(NestedSet<String> packages) {
+ private RecursivePkgValue(NestedSet<String> packages, boolean hasErrors) {
this.packages = packages;
+ this.hasErrors = hasErrors;
}
- static RecursivePkgValue create(NestedSetBuilder<String> packages) {
- if (packages.isEmpty()) {
+ static RecursivePkgValue create(NestedSetBuilder<String> packages, boolean hasErrors) {
+ if (packages.isEmpty() && !hasErrors) {
return EMPTY;
}
- return new RecursivePkgValue(packages.build());
+ return new RecursivePkgValue(packages.build(), hasErrors);
}
/** Create a transitive package lookup request. */
@@ -65,6 +67,10 @@
return packages;
}
+ public boolean hasErrors() {
+ return hasErrors;
+ }
+
@AutoCodec.VisibleForSerialization
@AutoCodec
static class Key extends RecursivePkgSkyKey {
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index ad872a6..307426b 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -27,6 +27,7 @@
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
+import com.google.common.collect.Maps;
import com.google.common.collect.MoreCollectors;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -58,7 +59,6 @@
import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
-import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
import com.google.devtools.build.lib.vfs.Path;
@@ -70,8 +70,10 @@
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import java.io.IOException;
+import java.io.InputStream;
import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
import java.util.UUID;
import java.util.logging.Level;
import java.util.logging.Logger;
@@ -1026,8 +1028,147 @@
runTestPackageLoadingError(/*keepGoing=*/ false, "//bad/...");
}
+ @Test
+ public void testPackageLoadingError_KeepGoing_SomeGoodTargetsBeneathDirectory() throws Exception {
+ tester.addFile("good/BUILD", "sh_library(name = 't')\n");
+ runTestPackageLoadingError(/*keepGoing=*/ true, "//...");
+ }
+
+ @Test
+ public void testPackageLoadingError_NoKeepGoing_SomeGoodTargetsBeneathDirectory()
+ throws Exception {
+ tester.addFile("good/BUILD", "sh_library(name = 't')\n");
+ runTestPackageLoadingError(/*keepGoing=*/ false, "//...");
+ }
+
+ private void runTestPackageFileInconsistencyError(boolean keepGoing, String... patterns)
+ throws Exception {
+ tester.addFile("bad/BUILD", "sh_library(name = 't')\n");
+ IOException ioExn = new IOException("nope");
+ tester.throwExceptionOnGetInputStream(tester.getWorkspace().getRelative("bad/BUILD"), ioExn);
+ if (keepGoing) {
+ TargetPatternPhaseValue value = tester.loadKeepGoing(patterns);
+ assertThat(value.hasError()).isTrue();
+ tester.assertContainsWarning("Target pattern parsing failed");
+ tester.assertContainsError("error loading package 'bad': nope");
+ } else {
+ TargetParsingException exn =
+ assertThrows(TargetParsingException.class, () -> tester.load(patterns));
+ assertThat(exn).hasCauseThat().isInstanceOf(BuildFileContainsErrorsException.class);
+ assertThat(exn).hasCauseThat().hasMessageThat().contains("error loading package 'bad': nope");
+ }
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_KeepGoing_ExplicitTarget() throws Exception {
+ runTestPackageFileInconsistencyError(true, "//bad:BUILD");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_NoKeepGoing_ExplicitTarget() throws Exception {
+ runTestPackageFileInconsistencyError(false, "//bad:BUILD");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_KeepGoing_TargetsInPackage() throws Exception {
+ runTestPackageFileInconsistencyError(true, "//bad:all");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_NoKeepGoing_TargetsInPackage() throws Exception {
+ runTestPackageFileInconsistencyError(false, "//bad:all");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_KeepGoing_argetsBeneathDirectory()
+ throws Exception {
+ runTestPackageFileInconsistencyError(true, "//bad/...");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_NoKeepGoing_TargetsBeneathDirectory()
+ throws Exception {
+ runTestPackageFileInconsistencyError(false, "//bad/...");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_KeepGoing_SomeGoodTargetsBeneathDirectory()
+ throws Exception {
+ tester.addFile("good/BUILD", "sh_library(name = 't')\n");
+ runTestPackageFileInconsistencyError(true, "//...");
+ }
+
+ @Test
+ public void testPackageFileInconsistencyError_NoKeepGoing_SomeGoodTargetsBeneathDirectory()
+ throws Exception {
+ tester.addFile("good/BUILD", "sh_library(name = 't')\n");
+ runTestPackageFileInconsistencyError(false, "//...");
+ }
+
+ private void runTestExtensionLoadingError(boolean keepGoing, String... patterns)
+ throws Exception {
+ tester.addFile("bad/f1.bzl", "nope");
+ tester.addFile("bad/BUILD", "load(\":f1.bzl\", \"not_a_symbol\")");
+ if (keepGoing) {
+ TargetPatternPhaseValue value = tester.loadKeepGoing(patterns);
+ assertThat(value.hasError()).isTrue();
+ tester.assertContainsWarning("Target pattern parsing failed");
+ } else {
+ TargetParsingException exn =
+ assertThrows(TargetParsingException.class, () -> tester.load(patterns));
+ assertThat(exn).hasCauseThat().isInstanceOf(BuildFileContainsErrorsException.class);
+ assertThat(exn).hasCauseThat().hasMessageThat().contains("Extension 'bad/f1.bzl' has errors");
+ }
+ tester.assertContainsError("/workspace/bad/f1.bzl:1:1: name 'nope' is not defined");
+ }
+
+ @Test
+ public void testExtensionLoadingError_KeepGoing_ExplicitTarget() throws Exception {
+ runTestExtensionLoadingError(/*keepGoing=*/ true, "//bad:BUILD");
+ }
+
+ @Test
+ public void testExtensionLoadingError_NoKeepGoing_ExplicitTarget() throws Exception {
+ runTestExtensionLoadingError(/*keepGoing=*/ false, "//bad:BUILD");
+ }
+
+ @Test
+ public void testExtensionLoadingError_KeepGoing_TargetsInPackage() throws Exception {
+ runTestExtensionLoadingError(/*keepGoing=*/ true, "//bad:all");
+ }
+
+ @Test
+ public void testExtensionLoadingError_NoKeepGoing_TargetsInPackage() throws Exception {
+ runTestExtensionLoadingError(/*keepGoing=*/ false, "//bad:all");
+ }
+
+ @Test
+ public void testExtensionLoadingError_KeepGoing_TargetsBeneathDirectory() throws Exception {
+ runTestExtensionLoadingError(/*keepGoing=*/ true, "//bad/...");
+ }
+
+ @Test
+ public void testExtensionLoadingError_NoKeepGoing_TargetsBeneathDirectory() throws Exception {
+ runTestExtensionLoadingError(/*keepGoing=*/ false, "//bad/...");
+ }
+
+ @Test
+ public void testExtensionLoadingError_KeepGoing_SomeGoodTargetsBeneathDirectory()
+ throws Exception {
+ tester.addFile("good/BUILD", "sh_library(name = 't')\n");
+ runTestExtensionLoadingError(/*keepGoing=*/ true, "//...");
+ }
+
+ @Test
+ public void testExtensionLoadingError_NoKeepGoing_SomeGoodTargetsBeneathDirectory()
+ throws Exception {
+ tester.addFile("good/BUILD", "sh_library(name = 't')\n");
+ runTestExtensionLoadingError(/*keepGoing=*/ false, "//...");
+ }
+
private static class LoadingPhaseTester {
private final ManualClock clock = new ManualClock();
+ private final CustomInMemoryFs fs = new CustomInMemoryFs(clock);
private final Path workspace;
private final AnalysisMock analysisMock;
@@ -1047,7 +1188,6 @@
private MockToolsConfig mockToolsConfig;
public LoadingPhaseTester() throws IOException {
- FileSystem fs = new InMemoryFileSystem(clock);
this.workspace = fs.getPath("/workspace");
workspace.createDirectory();
mockToolsConfig = new MockToolsConfig(workspace);
@@ -1234,6 +1374,10 @@
return loadingPhaseCompleteEvent.getFilteredLabels();
}
+ void throwExceptionOnGetInputStream(Path path, IOException exn) {
+ fs.throwExceptionOnGetInputStream(path, exn);
+ }
+
private Iterable<Event> filteredEvents() {
return Iterables.filter(storedErrors.getEvents(), new Predicate<Event>() {
@Override
@@ -1275,4 +1419,30 @@
.collect(MoreCollectors.onlyElement());
}
}
+
+ /**
+ * Custom {@link InMemoryFileSystem} that can be pre-configured per-file to throw a supplied
+ * IOException instead of the usual behavior.
+ */
+ private static class CustomInMemoryFs extends InMemoryFileSystem {
+
+ private final Map<Path, IOException> pathsToErrorOnGetInputStream = Maps.newHashMap();
+
+ CustomInMemoryFs(ManualClock manualClock) {
+ super(manualClock);
+ }
+
+ synchronized void throwExceptionOnGetInputStream(Path path, IOException exn) {
+ pathsToErrorOnGetInputStream.put(path, exn);
+ }
+
+ @Override
+ protected synchronized InputStream getInputStream(Path path) throws IOException {
+ IOException exnToThrow = pathsToErrorOnGetInputStream.get(path);
+ if (exnToThrow != null) {
+ throw exnToThrow;
+ }
+ return super.getInputStream(path);
+ }
+ }
}
