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 8e10e715a6ebdd..d0c5f6277b2808 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 @@ boolean encounteredPackageErrors() { 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 @@ public Iterable getPackagesUnderDirectory( // 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 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 2de3bce9cb93e4..9476606cf104fd 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 @@ protected RecursivePkgValue aggregateWithSubdirectorySkyValues( // 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 @@ private static class MyPackageDirectoryConsumer implements RecursiveDirectoryTraversalFunction.PackageDirectoryConsumer { private final NestedSetBuilder packages = new NestedSetBuilder<>(Order.STABLE_ORDER); + private boolean hasErrors = false; @Override public void notePackage(PathFragment pkgPath) { @@ -90,16 +94,19 @@ public void notePackage(PathFragment pkgPath) { @Override public void notePackageError(String noSuchPackageExceptionErrorMessage) { - // Nothing to do because the RecursiveDirectoryTraversalFunction has already emitted an error - // event. + hasErrors = true; } void addTransitivePackages(NestedSet 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 d864c024d334ad..0fd722f6b1d89f 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.emptySet(Order.STABLE_ORDER)); + new RecursivePkgValue(NestedSetBuilder.emptySet(Order.STABLE_ORDER), false); private final NestedSet packages; + private final boolean hasErrors; - private RecursivePkgValue(NestedSet packages) { + private RecursivePkgValue(NestedSet packages, boolean hasErrors) { this.packages = packages; + this.hasErrors = hasErrors; } - static RecursivePkgValue create(NestedSetBuilder packages) { - if (packages.isEmpty()) { + static RecursivePkgValue create(NestedSetBuilder 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 @@ public NestedSet getPackages() { 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 ad872a660aec42..307426bfed9026 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 @@ public void testPackageLoadingError_NoKeepGoing_TargetsBeneathDirectory() throws 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 static class LoadingPhaseTester { 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 @@ public ImmutableSet