Improve error handling for builtins injection
BuiltinsFailedException gets wrapped as a BzlLoadFailedException in BzlLoadFunction, and vice versa in StarlarkBuiltinsFunction. This makes for more verbose errors, but generally users who are not Bazel developers shouldn't see these errors.
The tests assert against the long form of the error rather than a substring to demonstrate the message in context.
Also slightly refactored the way errors in loaded bzls are reported, for uniformity between inlining and non-inlining code paths.
Work toward #11437.
RELNOTES: None
PiperOrigin-RevId: 315303094
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index d8f9acf..9692879 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -42,10 +42,10 @@
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.StarlarkExportable;
import com.google.devtools.build.lib.packages.WorkspaceFileValue;
+import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.LoadStatement;
-import com.google.devtools.build.lib.syntax.Location;
import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.StarlarkFile;
@@ -345,7 +345,7 @@
@Nullable
private StarlarkBuiltinsValue getStarlarkBuiltinsValue(
BzlLoadValue.Key key, Environment env, StarlarkSemantics starlarkSemantics)
- throws InterruptedException {
+ throws BuiltinsFailedException, InterruptedException {
// Don't request @builtins if we're in workspace evaluation, or already in @builtins evaluation.
if (!(key instanceof BzlLoadValue.KeyForBuild)
// TODO(#11437): Remove ability to disable injection by setting flag to empty string.
@@ -353,7 +353,8 @@
return uninjectedStarlarkBuiltins;
}
// May be null.
- return (StarlarkBuiltinsValue) env.getValue(StarlarkBuiltinsValue.key());
+ return (StarlarkBuiltinsValue)
+ env.getValueOrThrow(StarlarkBuiltinsValue.key(), BuiltinsFailedException.class);
}
@Nullable
@@ -462,8 +463,12 @@
return null;
}
- StarlarkBuiltinsValue starlarkBuiltinsValue =
- getStarlarkBuiltinsValue(key, env, starlarkSemantics);
+ StarlarkBuiltinsValue starlarkBuiltinsValue;
+ try {
+ starlarkBuiltinsValue = getStarlarkBuiltinsValue(key, env, starlarkSemantics);
+ } catch (BuiltinsFailedException e) {
+ throw BzlLoadFailedException.builtinsFailed(label, e);
+ }
if (starlarkBuiltinsValue == null) {
return null;
}
@@ -546,10 +551,15 @@
}
// Load .bzl modules in parallel.
+ // TODO(bazel-team): In case of a failed load(), we should report the location of the load()
+ // statement in the requesting file, e.g. using
+ // file.getLoadStatements().get(...).getStartLocation(). We should also probably catch and
+ // rethrow InconsistentFilesystemException with location info in the non-inlining code path so
+ // the error message is the same in both code paths.
List<BzlLoadValue> bzlLoads =
inliningState == null
- ? computeBzlLoadsWithSkyframe(env, loadKeys, file.getStartLocation())
- : computeBzlLoadsWithInlining(env, loadKeys, label, inliningState);
+ ? computeBzlLoadsWithSkyframe(env, loadKeys, file)
+ : computeBzlLoadsWithInlining(env, loadKeys, file, inliningState);
if (bzlLoads == null) {
return null; // Skyframe deps unavailable
}
@@ -694,7 +704,7 @@
*/
@Nullable
private static List<BzlLoadValue> computeBzlLoadsWithSkyframe(
- Environment env, List<BzlLoadValue.Key> keys, Location locationForErrors)
+ Environment env, List<BzlLoadValue.Key> keys, StarlarkFile requestingFile)
throws BzlLoadFailedException, InterruptedException {
List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
Map<SkyKey, ValueOrException<BzlLoadFailedException>> values =
@@ -704,8 +714,7 @@
try {
bzlLoads.add((BzlLoadValue) values.get(key).get());
} catch (BzlLoadFailedException exn) {
- throw new BzlLoadFailedException(
- "in " + locationForErrors.file() + ": " + exn.getMessage());
+ throw BzlLoadFailedException.whileLoadingDep(requestingFile.getStartLocation().file(), exn);
}
}
return env.valuesMissing() ? null : bzlLoads;
@@ -718,12 +727,16 @@
*/
@Nullable
private List<BzlLoadValue> computeBzlLoadsWithInlining(
- Environment env, List<BzlLoadValue.Key> keys, Label fileLabel, InliningState inliningState)
+ Environment env,
+ List<BzlLoadValue.Key> keys,
+ StarlarkFile requestingFile,
+ InliningState inliningState)
throws InterruptedException, BzlLoadFailedException, InconsistentFilesystemException {
+ String filePathForErrors = requestingFile.getStartLocation().file();
Preconditions.checkState(
env instanceof RecordingSkyFunctionEnvironment,
"Expected to be recording dep requests when inlining BzlLoadFunction: %s",
- fileLabel);
+ filePathForErrors);
Environment strippedEnv = ((RecordingSkyFunctionEnvironment) env).getDelegate();
List<BzlLoadValue> bzlLoads = Lists.newArrayListWithExpectedSize(keys.size());
@@ -739,7 +752,11 @@
CachedBzlLoadData cachedData;
try {
cachedData = computeInlineWithState(key, strippedEnv, inliningState);
- } catch (BzlLoadFailedException | InconsistentFilesystemException e) {
+ } catch (BzlLoadFailedException e) {
+ e = BzlLoadFailedException.whileLoadingDep(filePathForErrors, e);
+ deferredException = MoreObjects.firstNonNull(deferredException, e);
+ continue;
+ } catch (InconsistentFilesystemException e) {
deferredException = MoreObjects.firstNonNull(deferredException, e);
continue;
}
@@ -856,6 +873,18 @@
this.transience = transience;
}
+ Transience getTransience() {
+ return transience;
+ }
+
+ // TODO(bazel-team): This exception should hold a Location of the requesting file's load
+ // statement, and code that catches it should use the location in the Event they create.
+ static BzlLoadFailedException whileLoadingDep(
+ String requestingFile, BzlLoadFailedException cause) {
+ // Don't chain exception cause, just incorporate the message with a prefix.
+ return new BzlLoadFailedException("in " + requestingFile + ": " + cause.getMessage());
+ }
+
static BzlLoadFailedException errors(PathFragment file) {
return new BzlLoadFailedException(String.format("Extension file '%s' has errors", file));
}
@@ -899,7 +928,14 @@
return new BzlLoadFailedException(String.format("Extension '%s' has errors", file));
}
- // TODO(#11437): Add builtinsFailed() factory method here.
+ static BzlLoadFailedException builtinsFailed(Label file, BuiltinsFailedException cause) {
+ return new BzlLoadFailedException(
+ String.format(
+ "Internal error while loading Starlark builtins for %s: %s",
+ file, cause.getMessage()),
+ cause,
+ cause.getTransience());
+ }
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
index 1b773f5..1e0892d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunction.java
@@ -19,12 +19,14 @@
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.StructProvider;
+import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Location;
import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.LinkedHashMap;
@@ -111,7 +113,14 @@
throws StarlarkBuiltinsFunctionException, InterruptedException {
// skyKey is a singleton, unused.
- BzlLoadValue exportsValue = (BzlLoadValue) env.getValue(EXPORTS_ENTRYPOINT_KEY);
+ BzlLoadValue exportsValue;
+ try {
+ exportsValue =
+ (BzlLoadValue) env.getValueOrThrow(EXPORTS_ENTRYPOINT_KEY, BzlLoadFailedException.class);
+ } catch (BzlLoadFailedException ex) {
+ throw new StarlarkBuiltinsFunctionException(
+ BuiltinsFailedException.errorEvaluatingBuiltinsBzls(ex));
+ }
if (exportsValue == null) {
return null;
}
@@ -128,7 +137,7 @@
return new StarlarkBuiltinsValue(predeclared, exportedToJava, transitiveDigest);
} catch (EvalException ex) {
ex.ensureLocation(EXPORTS_ENTRYPOINT_LOC);
- throw new StarlarkBuiltinsFunctionException(ex);
+ throw new StarlarkBuiltinsFunctionException(BuiltinsFailedException.errorApplyingExports(ex));
}
}
@@ -245,11 +254,40 @@
return null;
}
+ /** An exception that occurs while trying to determine the injected builtins. */
+ static final class BuiltinsFailedException extends Exception implements SaneAnalysisException {
+
+ private final Transience transience;
+
+ private BuiltinsFailedException(String errorMessage, Exception cause, Transience transience) {
+ super(errorMessage, cause);
+ this.transience = transience;
+ }
+
+ Transience getTransience() {
+ return transience;
+ }
+
+ static BuiltinsFailedException errorEvaluatingBuiltinsBzls(BzlLoadFailedException cause) {
+ return new BuiltinsFailedException(
+ String.format("Failed to load builtins sources: %s", cause.getMessage()),
+ cause,
+ cause.getTransience());
+ }
+
+ static BuiltinsFailedException errorApplyingExports(EvalException cause) {
+ return new BuiltinsFailedException(
+ String.format("Failed to apply declared builtins: %s", cause.getMessage()),
+ cause,
+ Transience.PERSISTENT);
+ }
+ }
+
/** The exception type thrown by {@link StarlarkBuiltinsFunction}. */
static final class StarlarkBuiltinsFunctionException extends SkyFunctionException {
- private StarlarkBuiltinsFunctionException(Exception cause) {
- super(cause, Transience.PERSISTENT);
+ private StarlarkBuiltinsFunctionException(BuiltinsFailedException cause) {
+ super(cause, cause.transience);
}
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
index 660090d..57c9939 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BuiltinsInjectionTest.java
@@ -116,6 +116,51 @@
assertContainsEvent("overridable_rule :: new_rule");
}
+ @Test
+ public void evalWithInjection_errorInEvaluatingBuiltins() throws Exception {
+ // Test case with a Starlark error in the @builtins pseudo-repo itself.
+ // TODO(#11437): Use @builtins//:... syntax for load, once supported.
+ scratch.file(
+ "tools/builtins_staging/helper.bzl", //
+ "toplevels = {'overridable_symbol': 1//0} # <-- dynamic error");
+ writeExportsBzl(
+ "load('//tools/builtins_staging:helper.bzl', 'toplevels')",
+ "exported_toplevels = toplevels",
+ "exported_rules = {}",
+ "exported_to_java = {}");
+ writePkgBzl("print('evaluation completed')");
+ reporter.removeHandler(failFastHandler);
+
+ buildDummyWithoutAssertingSuccess();
+ assertContainsEvent(
+ "/workspace/tools/builtins_staging/helper.bzl:1:36: integer division by zero");
+ assertContainsEvent(
+ "error loading package 'pkg': Internal error while loading Starlark builtins for "
+ + "//pkg:dummy.bzl: Failed to load builtins sources: in "
+ + "/workspace/tools/builtins_staging/exports.bzl: Extension file "
+ + "'tools/builtins_staging/helper.bzl' has errors");
+ assertDoesNotContainEvent("evaluation completed");
+ }
+
+ @Test
+ public void evalWithInjection_errorInProcessingExports() throws Exception {
+ // Test case with an error in the symbols exported by exports.bzl, but no actual Starlark errors
+ // in the @builtins files themselves.
+ writeExportsBzl(
+ "exported_toplevels = None", // should be dict
+ "exported_rules = {}",
+ "exported_to_java = {}");
+ writePkgBzl("print('evaluation completed')");
+ reporter.removeHandler(failFastHandler);
+
+ buildDummyWithoutAssertingSuccess();
+ assertContainsEvent(
+ "error loading package 'pkg': Internal error while loading Starlark builtins for "
+ + "//pkg:dummy.bzl: Failed to apply declared builtins: got NoneType for "
+ + "'exported_toplevels dict', want dict");
+ assertDoesNotContainEvent("evaluation completed");
+ }
+
// TODO(#11437): Remove once disabling is not allowed.
@Test
public void injectionDisabledByFlag() throws Exception {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java
index 74689b9..e086677 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkBuiltinsFunctionTest.java
@@ -21,9 +21,9 @@
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.analysis.util.MockRule;
+import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException;
import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
import com.google.devtools.build.lib.syntax.ClassObject;
-import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.SkyKey;
@@ -146,10 +146,12 @@
"exported_toplevels = {}", //
"# exported_rules missing",
"exported_to_java = {}");
- assertThat(ex).isInstanceOf(EvalException.class);
+ assertThat(ex).isInstanceOf(BuiltinsFailedException.class);
assertThat(ex)
.hasMessageThat()
- .contains("expected a 'exported_rules' dictionary to be defined");
+ .contains(
+ "Failed to apply declared builtins: expected a 'exported_rules' dictionary to be "
+ + "defined");
}
@Test
@@ -159,8 +161,11 @@
"exported_toplevels = {}", //
"exported_rules = None",
"exported_to_java = {}");
- assertThat(ex).isInstanceOf(EvalException.class);
- assertThat(ex).hasMessageThat().contains("got NoneType for 'exported_rules dict', want dict");
+ assertThat(ex).isInstanceOf(BuiltinsFailedException.class);
+ assertThat(ex)
+ .hasMessageThat()
+ .contains(
+ "Failed to apply declared builtins: got NoneType for 'exported_rules dict', want dict");
}
@Test
@@ -170,10 +175,12 @@
"exported_toplevels = {}", //
"exported_rules = {1: 'a'}",
"exported_to_java = {}");
- assertThat(ex).isInstanceOf(EvalException.class);
+ assertThat(ex).isInstanceOf(BuiltinsFailedException.class);
assertThat(ex)
.hasMessageThat()
- .contains("got dict<int, string> for 'exported_rules dict', want dict<string, unknown>");
+ .contains(
+ "Failed to apply declared builtins: got dict<int, string> for 'exported_rules dict', "
+ + "want dict<string, unknown>");
}
@Test
@@ -185,9 +192,12 @@
"exported_rules = {}",
"exported_to_java = {}",
"asdf asdf # <-- parse error");
+ assertThat(ex).isInstanceOf(BuiltinsFailedException.class);
assertThat(ex)
.hasMessageThat()
- .contains("Extension 'tools/builtins_staging/exports.bzl' has errors");
+ .contains(
+ "Failed to load builtins sources: Extension 'tools/builtins_staging/exports.bzl' has "
+ + "errors");
}
@Test
@@ -199,9 +209,12 @@
"exported_rules = {}",
"exported_to_java = {}",
"1 // 0 # <-- dynamic error");
+ assertThat(ex).isInstanceOf(BuiltinsFailedException.class);
assertThat(ex)
.hasMessageThat()
- .contains("Extension file 'tools/builtins_staging/exports.bzl' has errors");
+ .contains(
+ "Failed to load builtins sources: Extension file 'tools/builtins_staging/exports.bzl' "
+ + "has errors");
}
@Test
@@ -218,8 +231,11 @@
"exported_toplevels = {}",
"exported_rules = {}",
"exported_to_java = {}");
+ assertThat(ex).isInstanceOf(BuiltinsFailedException.class);
assertThat(ex)
.hasMessageThat()
- .contains("Extension file 'builtins_helper/dummy.bzl' has errors");
+ .contains(
+ "Failed to load builtins sources: in /workspace/tools/builtins_staging/exports.bzl: "
+ + "Extension file 'builtins_helper/dummy.bzl' has errors");
}
}
