Fix long-standing bug with Blaze's exit code in --keep_going mode when encountering a package loading error during target pattern parsing of patterns like "//foo:all" and "//foo/...".
The bug is that we were completely ignoring package loading errors in the SkyFunction work that went into TargetPatternPhaseValue computation, and so therefore the control flow all the way from SkyframeExecutor#loadTargetPatterns to BuildView#createErrorMessage could never hope to take such a package loading error into account.
Fixes #7115
RELNOTES: In --keep_going mode, Bazel now correctly returns a non-zero exit code when encountering a package loading error during target pattern parsing of patterns like "//foo:all" and "//foo/...".
PiperOrigin-RevId: 229839139
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 8f3f4e8..d420573 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
@@ -39,6 +39,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
/**
* A {@link RecursivePackageProvider} backed by an {@link Environment}. Its methods may throw {@link
@@ -52,11 +53,18 @@
extends AbstractRecursivePackageProvider {
private final Environment env;
+ private final AtomicBoolean encounteredPackageErrors = new AtomicBoolean(false);
EnvironmentBackedRecursivePackageProvider(Environment env) {
this.env = env;
}
+ // TODO(nharmata): Audit the rest of the codebase to determine if we should be calling this method
+ // in more places.
+ boolean encounteredPackageErrors() {
+ return encounteredPackageErrors.get();
+ }
+
@Override
public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
throws NoSuchPackageException, MissingDepException, InterruptedException {
@@ -77,7 +85,11 @@
Preconditions.checkState(env.valuesMissing(), "Should have thrown for %s", packageName);
throw new MissingDepException();
} catch (BuildFileContainsErrorsException e) {
- // Expected.
+ // If this is a keep_going build, then the user of this RecursivePackageProvider has two
+ // options for handling the "package in error" case. The user must either inspect the
+ // package returned by this method, or else determine whether any errors have been seen via
+ // the "encounteredPackageErrors" method.
+ encounteredPackageErrors.set(true);
}
}
return pkgValue.getPackage();
@@ -107,6 +119,7 @@
return packageLookupValue.packageExists();
} catch (NoSuchPackageException | InconsistentFilesystemException e) {
env.getListener().handle(Event.error(e.getMessage()));
+ encounteredPackageErrors.set(true);
return false;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
index acef91d..3ede380 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageErrorFunction.java
@@ -33,10 +33,11 @@
/**
* SkyFunction that throws a {@link BuildFileContainsErrorsException} for {@link Package} that
- * loaded, but was in error. Must only be requested when a SkyFunction wishes to ignore the errors
- * in a {@link Package} in keep_going mode, but to shut down the build in nokeep_going mode. Thus,
- * this SkyFunction should only be requested when the corresponding {@link PackageFunction} has
- * already been successfully called and the resulting Package contains an error.
+ * loaded, but was in error. Must only be requested when a SkyFunction wishes to ignore the Skyframe
+ * error from a {@link PackageValue} in keep_going mode, but to shut down the build in nokeep_going
+ * mode. Thus, this SkyFunction should only be requested when the corresponding {@link
+ * PackageFunction} has already been successfully called and the resulting Package contains an
+ * error.
*
* <p>This SkyFunction never returns a value, only throws a {@link BuildFileNotFoundException}, and
* should never return null, since all of its dependencies should already be present.
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 52308fd..b195308 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
@@ -82,7 +82,12 @@
// The exception type here has to match the one on the BatchCallback. Since the callback
// defined above never throws, the exact type here is not really relevant.
RuntimeException.class);
- resolvedTargets = ResolvedTargets.<Target>builder().addAll(results).build();
+ ResolvedTargets.Builder<Target> resolvedTargetsBuilder =
+ ResolvedTargets.<Target>builder().addAll(results);
+ if (provider.encounteredPackageErrors()) {
+ resolvedTargetsBuilder.setError();
+ }
+ resolvedTargets = resolvedTargetsBuilder.build();
} catch (TargetParsingException e) {
env.getListener().post(new ParsingFailedEvent(patternKey.getPattern(), e.getMessage()));
throw new TargetPatternFunctionException(e);
@@ -96,6 +101,9 @@
}
Preconditions.checkNotNull(resolvedTargets, key);
ResolvedTargets.Builder<Label> resolvedLabelsBuilder = ResolvedTargets.builder();
+ if (resolvedTargets.hasError()) {
+ resolvedLabelsBuilder.setError();
+ }
for (Target target : resolvedTargets.getTargets()) {
resolvedLabelsBuilder.add(target.getLabel());
}
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 090988b..93e8ff2 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
@@ -43,6 +43,7 @@
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions;
@@ -1003,6 +1004,51 @@
"Bad target pattern '../foo': package name component contains only '.' characters");
}
+ private void runTestPackageLoadingError(boolean keepGoing, String... patterns) throws Exception {
+ tester.addFile("bad/BUILD", "nope");
+ 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("Package 'bad' contains errors");
+ }
+ tester.assertContainsError("/workspace/bad/BUILD:1:1: name 'nope' is not defined");
+ }
+
+ @Test
+ public void testPackageLoadingError_KeepGoing_ExplicitTarget() throws Exception {
+ runTestPackageLoadingError(/*keepGoing=*/ true, "//bad:BUILD");
+ }
+
+ @Test
+ public void testPackageLoadingError_NoKeepGoing_ExplicitTarget() throws Exception {
+ runTestPackageLoadingError(/*keepGoing=*/ false, "//bad:BUILD");
+ }
+
+ @Test
+ public void testPackageLoadingError_KeepGoing_TargetsInPackage() throws Exception {
+ runTestPackageLoadingError(/*keepGoing=*/ true, "//bad:all");
+ }
+
+ @Test
+ public void testPackageLoadingError_NoKeepGoing_TargetsInPackage() throws Exception {
+ runTestPackageLoadingError(/*keepGoing=*/ false, "//bad:all");
+ }
+
+ @Test
+ public void testPackageLoadingError_KeepGoing_TargetsBeneathDirectory() throws Exception {
+ runTestPackageLoadingError(/*keepGoing=*/ true, "//bad/...");
+ }
+
+ @Test
+ public void testPackageLoadingError_NoKeepGoing_TargetsBeneathDirectory() throws Exception {
+ runTestPackageLoadingError(/*keepGoing=*/ false, "//bad/...");
+ }
+
private static class LoadingPhaseTester {
private final ManualClock clock = new ManualClock();
private final Path workspace;
diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh
index 8c7344d..2fc74af 100755
--- a/src/test/shell/integration/loading_phase_test.sh
+++ b/src/test/shell/integration/loading_phase_test.sh
@@ -377,4 +377,19 @@
expect_log "//$pkg/foo:BUILD"
}
+function test_package_loading_errors_in_target_parsing() {
+ mkdir bad || fail "mkdir failed"
+ echo "nope" > bad/BUILD || fail "echo failed"
+
+ for keep_going in "--keep_going" "--nokeep_going"
+ do
+ for target_pattern in "//bad:BUILD" "//bad:all" "//bad/..."
+ do
+ bazel build --nobuild "$target_pattern" >& "$TEST_log" \
+ && fail "Expected failure"
+ expect_log "Build did NOT complete successfully"
+ done
+ done
+}
+
run_suite "Integration tests of ${PRODUCT_NAME} using loading/analysis phases."
