Fix long-standing bug with Blaze's exit code in --keep_going mode whe…

…n 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 bazelbuild#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

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 @@ public final class EnvironmentBackedRecursivePackageProvider
     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 @@ public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier p
         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 @@ public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier pa
       return packageLookupValue.packageExists();
     } catch (NoSuchPackageException | InconsistentFilesystemException e) {
       env.getListener().handle(Event.error(e.getMessage()));
+      encounteredPackageErrors.set(true);
       return false;
     }
   }

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.

src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java

@@ -82,7 +82,12 @@ public void process(Iterable<Target> partialResult) {
           // 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 @@ public void process(Iterable<Target> partialResult) {
     }
     Preconditions.checkNotNull(resolvedTargets, key);
     ResolvedTargets.Builder<Label> resolvedLabelsBuilder = ResolvedTargets.builder();
+    if (resolvedTargets.hasError()) {
+      resolvedLabelsBuilder.setError();
+    }
     for (Target target : resolvedTargets.getTargets()) {
       resolvedLabelsBuilder.add(target.getLabel());
     }

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 @@ public void testPatternStartingWithDotDotSlash() throws Exception {
         "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;

src/test/shell/integration/loading_phase_test.sh

@@ -377,4 +377,19 @@ function test_incompatible_disallow_load_labels_to_cross_package_boundaries() {
   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."