Skyframe-based loading phase runner: report errors if a package is in error.

This is a bit odd - the legacy loading phase runner reports a loading error,
but no target pattern error in keep_going mode, even though it's clearly an
error in the referenced target itself, rather than in its transitive closure.

This happens because the target pattern eval swallows such errors in keep_going
mode, and doesn't even report the error. I tried changing that, but it's a
fairly large refactoring, and that code path is dead if we switch to the new
one.

In the Skyframe-based implementation, both keep_going and nokeep_going paths
now report a target pattern error. (Note that the new code can never report a
loading error, because it doesn't perform transitive loading.)

The corresponding test is moved from SkyframeLoadingAndAnalysisTest to
LoadingPhaseRunnerTest - we don't need any integration test setup for that.

--
MOS_MIGRATED_REVID=114236897

2 files changed, 39 insertions, 0 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
index ad2cff7a29..d5ecb25ae2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestSuiteExpansionFunction.java
@@ -107,6 +107,9 @@ final class TestSuiteExpansionFunction implements SkyFunction {
       }
       try {
         builder.add(pkg.getTarget(label.getName()));
+        if (pkg.containsErrors()) {
+          builder.setError();
+        }
       } catch (NoSuchTargetException e) {
         builder.setError();
       }
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 3696a10a66..d96507ae18 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
@@ -499,6 +499,37 @@ public class LoadingPhaseRunnerTest {
     assertThat(loadingResult.getTargets()).containsExactlyElementsIn(getTargets("//suite:c"));
   }
 
+  @Test
+  public void testTopLevelTargetErrorsPrintedExactlyOnce_NoKeepGoing() throws Exception {
+    // 42 is not a valid bash_version, and this is detected during package loading.
+    tester.addFile("bad/BUILD", "sh_binary(name = 'bad', srcs = ['bad.sh'], bash_version = '42')");
+    try {
+      tester.load("//bad");
+      fail();
+    } catch (TargetParsingException expected) {
+    }
+    tester.assertContainsEventWithFrequency("invalid value in 'bash_version' attribute", 1);
+  }
+
+  @Test
+  public void testTopLevelTargetErrorsPrintedExactlyOnce_KeepGoing() throws Exception {
+    // 42 is not a valid bash_version, and this is detected during package loading.
+    tester.addFile("bad/BUILD", "sh_binary(name = 'bad', srcs = ['bad.sh'], bash_version = '42')");
+    LoadingResult loadingResult = tester.loadKeepGoing("//bad");
+    if (runsLoadingPhase()) {
+      // The legacy loading phase runner reports a loading error, but no target pattern error in
+      // keep_going mode, even though it's clearly an error in the referenced target itself, rather
+      // than in its transitive closure. This happens because the target pattern eval swallows such
+      // errors in keep_going mode. We could fix that, but it's a fairly invasive change, and we're
+      // planning to migrate to the Skyframe-based implementation anyway.
+      assertThat(loadingResult.hasTargetPatternError()).isFalse();
+      assertThat(loadingResult.hasLoadingError()).isTrue();
+    } else {
+      assertThat(loadingResult.hasTargetPatternError()).isTrue();
+    }
+    tester.assertContainsEventWithFrequency("invalid value in 'bash_version' attribute", 1);
+  }
+
   private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception {
     try {
       tester.load(targetPattern);
@@ -698,6 +729,11 @@ public class LoadingPhaseRunnerTest {
     public Event assertContainsError(String expectedMessage) {
       return MoreAsserts.assertContainsEvent(filteredEvents(), expectedMessage, EventKind.ERRORS);
     }
+
+    public void assertContainsEventWithFrequency(String expectedMessage, int expectedFrequency) {
+      MoreAsserts.assertContainsEventWithFrequency(
+          filteredEvents(), expectedMessage, expectedFrequency);
+    }
   }
 
   public static class FilteredTargetListener {