From 8c3b3fba3f68833bd97d3df2db9c48f0539efc3b Mon Sep 17 00:00:00 2001 From: adgar Date: Mon, 3 Jun 2019 13:35:28 -0700 Subject: [PATCH] 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 --- ...ronmentBackedRecursivePackageProvider.java | 17 +- .../lib/skyframe/RecursivePkgFunction.java | 13 +- .../build/lib/skyframe/RecursivePkgValue.java | 16 +- .../lib/pkgcache/LoadingPhaseRunnerTest.java | 174 +++++++++++++++++- 4 files changed, 206 insertions(+), 14 deletions(-) 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