Skip to content

Commit

Permalink
Don't crash when loading a non-existent exec transition .bzl file.
Browse files Browse the repository at this point in the history
i.e.

`$ bazel build //foo --experimental_exec_config=//bar/does_not_exist.bzl%my_transition`

This is a small change but has lots of files because I had to move
`BzlLoadFailedException` into its own class. So a lot of simple reference updates.

Details:

 - `StarlarkExecTransitionLoader` is responsible for loading the exec transition.
 - It delegates to `BzlLoadFunction` to load the `bzl` file.
 - I need `StarlarkExecTransitionLoader` to anticipate `BzlLoadFailedException`.
 - I moved `BzlLoadFailedException` out of `BzlLoadFunction` to avoid a build
   dependency cycle: `BzlLoadFunction` is part of the catch-call
   `:skyframe_cluster` library, which itself depends on
   `StarlarkExecTransitionLoader`.
 - Kept a bunch of static `BzlLoadFailedException` instantiators in
  `BzlLoadFunction`. Many of these involve higher order types with the same
  dependency issues.
 - Updated the `BzlFileLoader` interface (which wraps `BzlLoadFunction` calls)
   to throw `BzlLoadFailedException`.
 - Updated `BzlFileLoader` implementations to check for that exception when Skyframe-evaluating.
 - Uncommented the test checking this functionality.

PiperOrigin-RevId: 585800246
Change-Id: I71921380a658eb6c441b2600f0758049162f85c0
  • Loading branch information
gregestren authored and copybara-github committed Nov 28, 2023
1 parent 2454812 commit b2cca31
Show file tree
Hide file tree
Showing 21 changed files with 137 additions and 98 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1845,6 +1845,7 @@ java_library(
":config/core_options",
":config/starlark_defined_config_transition",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_failed_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value",
"//third_party:auto_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.devtools.build.lib.analysis.starlark.StarlarkAttributeTransitionProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.skyframe.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.BzlLoadValue;
import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsValue;
import java.util.List;
Expand Down Expand Up @@ -46,7 +47,7 @@ public interface BzlFileLoader {
* Loads the given {@link BzlLoadValue.Key}. Returns null if not all Skyframe deps are ready.
*/
@Nullable
BzlLoadValue getValue(BzlLoadValue.Key key) throws InterruptedException;
BzlLoadValue getValue(BzlLoadValue.Key key) throws BzlLoadFailedException, InterruptedException;
}

/**
Expand Down Expand Up @@ -75,14 +76,18 @@ public static Optional<StarlarkAttributeTransitionProvider> loadStarlarkExecTran
}
TransitionReference parsedRef =
TransitionReference.create(userRef, "--experimental_exec_config");
// TODO(b/301644122): catch and report BzlLoadFailedExceptions. This may required Bazel BUILD
// refactoring since BzlLoadFailedException is defined in BzlLoadFunction, which is part of
// the ":skyframe_cluster" target, which we can't depend on without dependency cycles.
BzlLoadValue bzlValue =
bzlFileLoader.getValue(
Objects.equals(parsedRef.bzlFile().getRepository(), StarlarkBuiltinsValue.BUILTINS_REPO)
? BzlLoadValue.keyForBuiltins(parsedRef.bzlFile())
: BzlLoadValue.keyForBuild(parsedRef.bzlFile()));
BzlLoadValue bzlValue;
try {
bzlValue =
bzlFileLoader.getValue(
Objects.equals(
parsedRef.bzlFile().getRepository(), StarlarkBuiltinsValue.BUILTINS_REPO)
? BzlLoadValue.keyForBuiltins(parsedRef.bzlFile())
: BzlLoadValue.keyForBuild(parsedRef.bzlFile()));
} catch (BzlLoadFailedException e) {
throw new StarlarkExecTransitionLoadingException(
"--experimental_exec_config", userRef, e.getMessage());
}
if (bzlValue == null) {
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_failed_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps;
import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
import com.google.devtools.build.lib.skyframe.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.BzlLoadValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.util.Fingerprint;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_failed_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
"//src/main/java/com/google/devtools/build/lib/util:filetype",
"//src/main/java/com/google/devtools/build/skydoc/rendering:rendering_util",
"//src/main/java/com/google/devtools/build/skydoc/rendering/proto:stardoc_output_java_proto",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SafeImplicitOutputsFunction;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.BzlLoadValue;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.profiler.memory.CurrentRuleTracker;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor.BuildViewProvider;
import com.google.devtools.build.lib.skyframe.config.BuildConfigurationKey;
import com.google.devtools.build.lib.skyframe.toolchains.ToolchainException;
Expand Down Expand Up @@ -318,7 +317,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
targetAndConfiguration.getConfiguration() == null
? null
: targetAndConfiguration.getConfiguration().getOptions(),
(bzlKey) -> (BzlLoadValue) env.getValue(bzlKey));
(bzlKey) ->
(BzlLoadValue) env.getValueOrThrow(bzlKey, BzlLoadFailedException.class));
if (starlarkExecTransition == null) {
return null; // Need Skyframe deps.
}
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ java_library(
":build_result_listener",
":bzl_compile",
":bzl_load_cycle_reporter",
":bzl_load_failed_exception",
":bzl_load_value",
":bzlmod_repo_cycle_reporter",
":cached_bzl_load_value_and_deps",
Expand Down Expand Up @@ -2404,6 +2405,17 @@ java_library(
],
)

java_library(
name = "bzl_load_failed_exception",
srcs = ["BzlLoadFailedException.java"],
deps = [
":sane_analysis_exception",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/protobuf:failure_details_java_proto",
],
)

java_library(
name = "state_informing_sky_function_environment",
srcs = ["StateInformingSkyFunctionEnvironment.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.skyframe.SkyFunction;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright 2023 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.devtools.build.lib.skyframe;

import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.StarlarkLoading;
import com.google.devtools.build.lib.server.FailureDetails.StarlarkLoading.Code;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;

/** Exceptions from {@link BzlLoadFunction}. */
public final class BzlLoadFailedException extends Exception implements SaneAnalysisException {
private final Transience transience;
private final DetailedExitCode detailedExitCode;

private BzlLoadFailedException(
String errorMessage, DetailedExitCode detailedExitCode, Transience transience) {
super(errorMessage);
this.transience = transience;
this.detailedExitCode = detailedExitCode;
}

BzlLoadFailedException(String errorMessage, DetailedExitCode detailedExitCode) {
this(errorMessage, detailedExitCode, Transience.PERSISTENT);
}

BzlLoadFailedException(
String errorMessage,
DetailedExitCode detailedExitCode,
Exception cause,
Transience transience) {
super(errorMessage, cause);
this.transience = transience;
this.detailedExitCode = detailedExitCode;
}

BzlLoadFailedException(String errorMessage, Code code) {
this(errorMessage, createDetailedExitCode(errorMessage, code), Transience.PERSISTENT);
}

BzlLoadFailedException(String errorMessage, Code code, Exception cause, Transience transience) {
this(errorMessage, createDetailedExitCode(errorMessage, code), cause, transience);
}

Transience getTransience() {
return transience;
}

@Override
public DetailedExitCode getDetailedExitCode() {
return detailedExitCode;
}

static DetailedExitCode createDetailedExitCode(String message, Code code) {
return DetailedExitCode.of(
FailureDetail.newBuilder()
.setMessage(message)
.setStarlarkLoading(StarlarkLoading.newBuilder().setCode(code))
.build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@
import com.google.devtools.build.lib.packages.SymbolGenerator;
import com.google.devtools.build.lib.packages.WorkspaceFileValue;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.StarlarkLoading;
import com.google.devtools.build.lib.server.FailureDetails.StarlarkLoading.Code;
import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException;
import com.google.devtools.build.lib.util.DetailedExitCode;
Expand Down Expand Up @@ -553,7 +551,7 @@ private BzlLoadValue computeInternal(
try {
compileValue = getter.getBzlCompileValue(compileKey, env);
} catch (BzlCompileFunction.FailedIOException e) {
throw BzlLoadFailedException.errorReadingBzl(filePath, e);
throw errorReadingBzl(filePath, e);
}
if (compileValue == null) {
return null;
Expand Down Expand Up @@ -609,7 +607,7 @@ private StarlarkBuiltinsValue getBuiltins(
/* bzlLoadFunction= */ this);
}
} catch (BuiltinsFailedException e) {
throw BzlLoadFailedException.builtinsFailed(key.getLabel(), e);
throw builtinsFailed(key.getLabel(), e);
}
}

Expand Down Expand Up @@ -649,7 +647,7 @@ private BzlCompileValue.Key validatePackageAndGetCompileKey(
// Bypass package lookup entirely if builtins.
if (key.isBuiltins()) {
if (!label.getPackageName().isEmpty()) {
throw BzlLoadFailedException.noBuildFile(label, "@_builtins cannot have subpackages");
throw noBuildFile(label, "@_builtins cannot have subpackages");
}
return key.getCompileKey(getBuiltinsRoot(builtinsBzlPath));
}
Expand All @@ -666,7 +664,7 @@ private BzlCompileValue.Key validatePackageAndGetCompileKey(
BuildFileNotFoundException.class,
InconsistentFilesystemException.class);
} catch (BuildFileNotFoundException | InconsistentFilesystemException e) {
throw BzlLoadFailedException.errorFindingContainingPackage(label.toPathFragment(), e);
throw errorFindingContainingPackage(label.toPathFragment(), e);
}
if (packageLookup == null) {
return null;
Expand All @@ -685,10 +683,9 @@ private BzlCompileValue.Key validatePackageAndGetCompileKey(
compileKey = key.getCompileKey(packageLookup.getContainingPackageRoot());
} else {
if (!packageLookup.hasContainingPackage()) {
throw BzlLoadFailedException.noBuildFile(
label, packageLookup.getReasonForNoContainingPackage());
throw noBuildFile(label, packageLookup.getReasonForNoContainingPackage());
} else {
throw BzlLoadFailedException.labelCrossesPackageBoundary(label, packageLookup);
throw labelCrossesPackageBoundary(label, packageLookup);
}
}
}
Expand Down Expand Up @@ -1153,7 +1150,7 @@ private static List<BzlLoadValue> computeBzlLoadsWithSkyframe(
try {
bzlLoads.add((BzlLoadValue) values.getOrThrow(keys.get(i), BzlLoadFailedException.class));
} catch (BzlLoadFailedException ex) {
throw BzlLoadFailedException.whileLoadingDep(programLoads.get(i).second, ex);
throw whileLoadingDep(programLoads.get(i).second, ex);
}
}
return env.valuesMissing() ? null : bzlLoads;
Expand Down Expand Up @@ -1197,7 +1194,7 @@ private List<BzlLoadValue> computeBzlLoadsWithInlining(
cachedData = computeInlineCachedData(keys.get(i), inliningState);
} catch (BzlLoadFailedException e) {
if (deferredException == null) {
deferredException = BzlLoadFailedException.whileLoadingDep(programLoads.get(i).second, e);
deferredException = whileLoadingDep(programLoads.get(i).second, e);
}
continue;
}
Expand Down Expand Up @@ -1273,7 +1270,7 @@ static void checkLoadVisibilities(
}
}
if (foundViolation && !demoteErrorsToWarnings) {
throw BzlLoadFailedException.visibilityViolation(requestingFileDescription);
throw visibilityViolation(requestingFileDescription);
}
}

Expand Down Expand Up @@ -1360,7 +1357,7 @@ private static void executeBzlFile(

execAndExport(prog, label, starlarkEventHandler, module, thread);
if (sawStarlarkError.get()) {
throw BzlLoadFailedException.executionFailed(label);
throw executionFailed(label);
}
}
}
Expand Down Expand Up @@ -1496,59 +1493,6 @@ private void reset() {
}
}

/** Indicates a failure to load a .bzl file. */
public static final class BzlLoadFailedException extends Exception
implements SaneAnalysisException {
private final Transience transience;
private final DetailedExitCode detailedExitCode;

private BzlLoadFailedException(
String errorMessage, DetailedExitCode detailedExitCode, Transience transience) {
super(errorMessage);
this.transience = transience;
this.detailedExitCode = detailedExitCode;
}

private BzlLoadFailedException(String errorMessage, DetailedExitCode detailedExitCode) {
this(errorMessage, detailedExitCode, Transience.PERSISTENT);
}

private BzlLoadFailedException(
String errorMessage,
DetailedExitCode detailedExitCode,
Exception cause,
Transience transience) {
super(errorMessage, cause);
this.transience = transience;
this.detailedExitCode = detailedExitCode;
}

private BzlLoadFailedException(String errorMessage, Code code) {
this(errorMessage, createDetailedExitCode(errorMessage, code), Transience.PERSISTENT);
}

private BzlLoadFailedException(
String errorMessage, Code code, Exception cause, Transience transience) {
this(errorMessage, createDetailedExitCode(errorMessage, code), cause, transience);
}

Transience getTransience() {
return transience;
}

@Override
public DetailedExitCode getDetailedExitCode() {
return detailedExitCode;
}

private static DetailedExitCode createDetailedExitCode(String message, Code code) {
return DetailedExitCode.of(
FailureDetail.newBuilder()
.setMessage(message)
.setStarlarkLoading(StarlarkLoading.newBuilder().setCode(code))
.build());
}

private static BzlLoadFailedException whileLoadingDep(
Location loc, BzlLoadFailedException cause) {
// Don't chain exception cause, just incorporate the message with a prefix.
Expand All @@ -1573,10 +1517,11 @@ static BzlLoadFailedException errorFindingContainingPackage(
String errorMessage =
String.format(
"Encountered error while reading extension file '%s': %s", file, cause.getMessage());
DetailedExitCode detailedExitCode =
cause instanceof DetailedException
? ((DetailedException) cause).getDetailedExitCode()
: createDetailedExitCode(errorMessage, Code.CONTAINING_PACKAGE_NOT_FOUND);
DetailedExitCode detailedExitCode =
cause instanceof DetailedException
? ((DetailedException) cause).getDetailedExitCode()
: BzlLoadFailedException.createDetailedExitCode(
errorMessage, Code.CONTAINING_PACKAGE_NOT_FOUND);
return new BzlLoadFailedException(
errorMessage, detailedExitCode, cause, Transience.PERSISTENT);
}
Expand Down Expand Up @@ -1639,11 +1584,11 @@ static BzlLoadFailedException visibilityViolation(String fileDescription) {
String.format("%s contains .bzl load visibility violations", fileDescription),
Code.VISIBILITY_ERROR);
}
}


private static final class BzlLoadFunctionException extends SkyFunctionException {
private BzlLoadFunctionException(BzlLoadFailedException cause) {
super(cause, cause.transience);
super(cause, cause.getTransience());
}
}
}
Loading

0 comments on commit b2cca31

Please sign in to comment.