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());
+ }
}
/**
