Correctly handle failing finalizers; allow unexpanded finalizers in a package which contains errors
If the evaluation of a package threw an EvalException, it is possible and
legal for some finalizers to be left unexpanded. Moreover, we shouldn't try
to expand finalizers in a package that's already in error, just as we
already stop evaluating top-level code and non-finalizer macros after an
earlier step in package evaluation throws.
Fixes ISE introduced by https://github.com/bazelbuild/bazel/commit/11c38261667b9976b7bd08f9f05c8a735d65f8e2
PiperOrigin-RevId: 761997199
Change-Id: I2a1576b0f112ef20e1f71a0b6e3b774fba69b7c1
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index bbbc50f..0cd1ff8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1538,7 +1538,8 @@
}
/**
- * Ensures that all symbolic macros in the package have expanded.
+ * Ensures that all symbolic macros in an error-free package have expanded. No-op if the package
+ * already {@link #containsErrors}.
*
* <p>This does not run any macro that has already been evaluated. It *does* run macros that are
* newly discovered during the operation of this method.
@@ -1555,8 +1556,9 @@
// longer a concern, we will want to support delayed expansion of non-finalizer macros before
// the finalizer expansion step.
- // Finalizer expansion step.
- if (!unexpandedMacros.isEmpty()) {
+ // Finalizer expansion step. Requires that the package not be in error (no point in finalizing
+ // a package that already threw an EvalException).
+ if (!containsErrors() && !unexpandedMacros.isEmpty()) {
Preconditions.checkState(
unexpandedMacros.stream()
.allMatch(id -> recorder.getMacroMap().get(id).getMacroClass().isFinalizer()),
@@ -1584,16 +1586,16 @@
@Override
@CanIgnoreReturnValue
protected Builder beforeBuild() throws NoSuchPackageException {
- // For correct semantics, we refuse to build a package that has declared symbolic macros that
- // have not yet been expanded. (Currently finalizers are the only use case where this happens,
- // but the Package logic is agnostic to that detail.)
+ // For correct semantics, we refuse to build a package that hasn't thrown any EvalExceptions
+ // but has declared symbolic macros that have not yet been expanded. (Currently finalizers are
+ // the only use case where this happens, but the Package logic is agnostic to that detail.)
//
// Production code should be calling expandAllRemainingMacros() to guarantee that nothing is
// left unexpanded. Tests that do not declare any symbolic macros need not make the call.
// Package deserialization doesn't have to do it either, since we shouldn't be evaluating
// symbolic macros on the deserialized result of an already evaluated package.
Preconditions.checkState(
- unexpandedMacros.isEmpty(),
+ unexpandedMacros.isEmpty() || containsErrors(),
"Cannot build a package with unexpanded symbolic macros; call"
+ " expandAllRemainingMacros()");
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFinalizerTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFinalizerTest.java
index ec25016..df37e4b 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleFinalizerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFinalizerTest.java
@@ -262,4 +262,66 @@
assertContainsEventWithFrequency(
"native.existing_rules and native.existing_rule are as expected", 4);
}
+
+ @Test
+ public void packageInError_notFinalized() throws Exception {
+ scratch.file(
+ "pkg/finalizers.bzl",
+ """
+ def _impl(name, visibility):
+ print("in my_finalizer")
+ native.cc_library(name = name + "_lib")
+
+ my_finalizer = macro(implementation = _impl, finalizer = True)
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":finalizers.bzl", "my_finalizer")
+ my_finalizer(name = "finalize")
+ cc_library(name = 1 // 0) # causes EvalException
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("division by zero");
+ assertDoesNotContainEvent("in my_finalizer");
+ assertThat(pkg.getTargets().keySet()).doesNotContain("finalize_lib");
+ }
+
+ // Regression test for b/419523258.
+ @Test
+ public void finalizerFailure_handledCleanly() throws Exception {
+ scratch.file(
+ "pkg/finalizers.bzl",
+ """
+ def _fail_impl(name, visibility):
+ fail("fail fail fail")
+
+ def _good_impl(name, visibility):
+ native.cc_library(name = name + "_lib")
+
+ fail_finalizer = macro(implementation = _fail_impl, finalizer = True)
+ good_finalizer = macro(implementation = _good_impl, finalizer = True)
+ """);
+ scratch.file(
+ "pkg/BUILD",
+ """
+ load(":finalizers.bzl", "fail_finalizer", "good_finalizer")
+ good_finalizer(name = "good_finalizer")
+ fail_finalizer(name = "bad_finalizer")
+ good_finalizer(name = "should_not_be_expanded") # because it follows a failing one
+ cc_library(name = "unrelated_target") # evaluated before any finalizers
+ """);
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = getPackage("pkg");
+ assertThat(pkg).isNotNull();
+ assertThat(pkg.containsErrors()).isTrue();
+ assertContainsEvent("fail fail fail");
+ assertThat(pkg.getTargets().keySet()).containsAtLeast("unrelated_target", "good_finalizer_lib");
+ assertThat(pkg.getTargets().keySet()).doesNotContain("should_not_be_expanded_lib");
+ }
}
