Handle packages with errors in TestsInSuiteFunction.
Clients of PackageFunction have to be careful to handle partially valid package results. Packages in an error state at least need to be flagged, so higher levels don't think the whole build succeeded. If --nokeep_going is in effect, an exception should generally be raised to abort the build.
Fixes https://github.com/bazelbuild/bazel/issues/10027.
Closes #10080.
PiperOrigin-RevId: 277227257
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 3ede380..8c78787 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
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
-import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -39,8 +38,8 @@
* 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.
+ * <p>This SkyFunction always throws a {@link BuildFileContainsErrorsException}. It also should
+ * never request a skyframe restart, since all of its dependencies should already be present.
*/
public class PackageErrorFunction implements SkyFunction {
public static Key key(PackageIdentifier packageIdentifier) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java
index 27adb3b..16e2c43 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestsInSuiteFunction.java
@@ -18,6 +18,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.ResolvedTargets;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -171,6 +172,20 @@
if (pkg == null) {
continue;
}
+ if (pkg.containsErrors()) {
+ hasError = true;
+ // Abort the build if --nokeep_going.
+ try {
+ env.getValueOrThrow(
+ PackageErrorFunction.key(label.getPackageIdentifier()),
+ BuildFileContainsErrorsException.class);
+ return false;
+ } catch (BuildFileContainsErrorsException e) {
+ // PackageErrorFunction always throws this exception, and this fact is used by Skyframe to
+ // abort the build. If we get here, it's either because of error bubbling or because we're
+ // in --keep_going mode. In either case, we *should* ignore the exception.
+ }
+ }
try {
targets.add(pkg.getTarget(label.getName()));
} catch (NoSuchTargetException e) {
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 b73a6763..5c2d84a 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
@@ -506,6 +506,28 @@
}
@Test
+ public void failureWhileLoadingTestsForTestSuiteKeepGoing() throws Exception {
+ tester.addFile("ts/BUILD", "test_suite(name = 'tests', tests = ['//pkg:tests'])");
+ tester.addFile("pkg/BUILD", "test_suite(name = 'tests')", "test_suite()");
+ TargetPatternPhaseValue loadingResult = tester.loadKeepGoing("//ts:tests");
+ assertThat(loadingResult.hasError()).isFalse();
+ assertThat(loadingResult.hasPostExpansionError()).isTrue();
+ tester.assertContainsError("test_suite rule has no 'name' attribute");
+ }
+
+ @Test
+ public void failureWhileLoadingTestsForTestSuiteNoKeepGoing() throws Exception {
+ tester.addFile("ts/BUILD", "test_suite(name = 'tests', tests = ['//pkg:tests'])");
+ tester.addFile("pkg/BUILD", "test_suite(name = 'tests')", "test_suite()");
+ TargetParsingException e =
+ assertThrows(TargetParsingException.class, () -> tester.load("//ts:tests"));
+ assertThat(e)
+ .hasMessageThat()
+ .isEqualTo("error loading package 'pkg': Package 'pkg' contains errors");
+ tester.assertContainsError("test_suite rule has no 'name' attribute");
+ }
+
+ @Test
public void testTestSuiteExpansionFailsMissingTarget() throws Exception {
tester.addFile("other/BUILD", "");
tester.addFile("ts/BUILD",
