Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for --keep_going to bazel mod #23828

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ java_library(
"BazelModTidyValue.java",
],
deps = [
":exception",
":root_module_file_fixup",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
Expand All @@ -377,6 +378,7 @@ java_library(
],
deps = [
":common",
":exception",
":resolution",
":root_module_file_fixup",
":tidy",
Expand All @@ -398,6 +400,7 @@ java_library(
],
deps = [
":common",
":exception",
":module_extension",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
Expand All @@ -415,9 +418,11 @@ java_library(
],
deps = [
":common",
":exception",
":inspection",
":module_extension",
":resolution",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
"//third_party:jsr305",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,16 @@ public SkyValue compute(SkyKey skyKey, Environment env)
return null;
}
ImmutableList.Builder<RootModuleFileFixup> fixups = ImmutableList.builder();
ImmutableList.Builder<ExternalDepsException> errors = ImmutableList.builder();
for (SkyKey extension : extensionsUsedByRootModule) {
SkyValue value = result.get(extension);
SkyValue value;
try {
value = result.getOrThrow(extension, ExternalDepsException.class);
} catch (ExternalDepsException e) {
// This extension failed, but we can still tidy up other extensions in keep going mode.
errors.add(e);
continue;
}
if (value == null) {
return null;
}
Expand All @@ -102,6 +110,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
}

return BazelModTidyValue.create(
fixups.build(), buildozer.asPath(), rootModuleFileValue.getModuleFilePaths());
fixups.build(),
buildozer.asPath(),
rootModuleFileValue.getModuleFilePaths(),
errors.build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,15 @@ public abstract class BazelModTidyValue implements SkyValue {
/** The set of paths to the root MODULE.bazel file and all its includes. */
public abstract ImmutableSet<PathFragment> moduleFilePaths();

/** Errors encountered while evaluating prerequisites for {@code bazel mod tidy}. */
public abstract ImmutableList<ExternalDepsException> errors();

static BazelModTidyValue create(
List<RootModuleFileFixup> fixups,
Path buildozer,
ImmutableSet<PathFragment> moduleFilePaths) {
ImmutableSet<PathFragment> moduleFilePaths,
ImmutableList<ExternalDepsException> errors) {
return new AutoValue_BazelModTidyValue(
ImmutableList.copyOf(fixups), buildozer, moduleFilePaths);
ImmutableList.copyOf(fixups), buildozer, moduleFilePaths, errors);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
ImmutableMap<ModuleKey, AugmentedModule> depGraph =
computeAugmentedGraph(unprunedDepGraph, resolvedDepGraph.keySet(), overrides);

ImmutableSetMultimap<ModuleExtensionId, String> extensionToRepoInternalNames =
computeExtensionToRepoInternalNames(depGraphValue, env);
if (extensionToRepoInternalNames == null) {
ExtensionRepos extensionRepos = computExtensionRepos(depGraphValue, env);
if (extensionRepos == null) {
return null;
}

Expand All @@ -85,8 +84,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
return BazelModuleInspectorValue.create(
depGraph,
modulesIndex,
extensionToRepoInternalNames,
depGraphValue.getCanonicalRepoNameLookup().inverse());
extensionRepos.extensionToRepoInternalNames(),
depGraphValue.getCanonicalRepoNameLookup().inverse(),
extensionRepos.errors());
}

public static ImmutableMap<ModuleKey, AugmentedModule> computeAugmentedGraph(
Expand Down Expand Up @@ -180,9 +180,13 @@ public static ImmutableMap<ModuleKey, AugmentedModule> computeAugmentedGraph(
.collect(toImmutableMap(Entry::getKey, e -> e.getValue().build()));
}

private record ExtensionRepos(
ImmutableSetMultimap<ModuleExtensionId, String> extensionToRepoInternalNames,
ImmutableList<ExternalDepsException> errors) {}

@Nullable
private ImmutableSetMultimap<ModuleExtensionId, String> computeExtensionToRepoInternalNames(
BazelDepGraphValue depGraphValue, Environment env) throws InterruptedException {
private ExtensionRepos computExtensionRepos(BazelDepGraphValue depGraphValue, Environment env)
throws InterruptedException {
ImmutableSet<ModuleExtensionId> extensionEvalKeys =
depGraphValue.getExtensionUsagesTable().rowKeySet();
ImmutableList<SingleExtensionValue.Key> singleExtensionKeys =
Expand All @@ -191,15 +195,25 @@ private ImmutableSetMultimap<ModuleExtensionId, String> computeExtensionToRepoIn

ImmutableSetMultimap.Builder<ModuleExtensionId, String> extensionToRepoInternalNames =
ImmutableSetMultimap.builder();
ImmutableList.Builder<ExternalDepsException> errors = ImmutableList.builder();
for (SingleExtensionValue.Key singleExtensionKey : singleExtensionKeys) {
SingleExtensionValue singleExtensionValue =
(SingleExtensionValue) singleExtensionValues.get(singleExtensionKey);
SingleExtensionValue singleExtensionValue;
try {
singleExtensionValue =
(SingleExtensionValue)
singleExtensionValues.getOrThrow(singleExtensionKey, ExternalDepsException.class);
} catch (ExternalDepsException e) {
// The extension failed, so we can't report its generated repos. We can still report the
// imported repos in keep going mode, so don't fail and just skip this extension.
errors.add(e);
continue;
}
if (singleExtensionValue == null) {
return null;
}
extensionToRepoInternalNames.putAll(
singleExtensionKey.argument(), singleExtensionValue.getGeneratedRepoSpecs().keySet());
}
return extensionToRepoInternalNames.build();
return new ExtensionRepos(extensionToRepoInternalNames.build(), errors.build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
Expand Down Expand Up @@ -44,9 +45,10 @@ public static BazelModuleInspectorValue create(
ImmutableMap<ModuleKey, AugmentedModule> depGraph,
ImmutableMap<String, ImmutableSet<ModuleKey>> modulesIndex,
ImmutableSetMultimap<ModuleExtensionId, String> extensionToRepoInternalNames,
ImmutableMap<ModuleKey, RepositoryName> moduleKeyToCanonicalNames) {
ImmutableMap<ModuleKey, RepositoryName> moduleKeyToCanonicalNames,
ImmutableList<ExternalDepsException> errors) {
return new AutoValue_BazelModuleInspectorValue(
depGraph, modulesIndex, extensionToRepoInternalNames, moduleKeyToCanonicalNames);
depGraph, modulesIndex, extensionToRepoInternalNames, moduleKeyToCanonicalNames, errors);
}

/**
Expand Down Expand Up @@ -74,6 +76,9 @@ public static BazelModuleInspectorValue create(
/** A mapping from a module key to the canonical repository name of the module repository. */
public abstract ImmutableMap<ModuleKey, RepositoryName> getModuleKeyToCanonicalNames();

/** A list of exceptions that occurred during module graph inspection. */
public abstract ImmutableList<ExternalDepsException> getErrors();

/**
* A wrapper for {@link Module}, augmented with references to dependants (and also those who are
* not used in the final dep graph).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@
@Command(
name = ModCommand.NAME,
buildPhase = LOADS,
// TODO(andreisolo): figure out which extra options are really needed
options = {
ModOptions.class,
PackageOptions.class,
Expand Down Expand Up @@ -194,6 +193,7 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
EvaluationContext.newBuilder()
.setParallelism(threadsOption.threads)
.setEventHandler(env.getReporter())
.setKeepGoing(options.getOptions(KeepGoingOption.class).keepGoing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just set this to true unconditionally. After all, it's been the case so far, and IMO for an informational subcommand like bazel mod, most people would want to set --keep_going as a default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's technically been enabled so far, I don't think that it had any effect in practice since the relevant SkyFunctions are not catching errors. But I still agree that bazel mod commands should probably keep going by default. I've also implemented this for bazel mod tidy.

.build();

try {
Expand All @@ -208,11 +208,11 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
keys.add(BazelDepGraphValue.KEY, BazelModuleInspectorValue.KEY);
}
EvaluationResult<SkyValue> evaluationResult =
skyframeExecutor.prepareAndGet(keys.build(), evaluationContext);
skyframeExecutor.getEvaluator().evaluate(keys.build(), evaluationContext);

if (evaluationResult.hasError()) {
Exception e = evaluationResult.getError().getException();
String message = "Unexpected error during repository rule evaluation.";
String message = "Unexpected error during module graph evaluation.";
if (e != null) {
message = e.getMessage();
}
Expand Down Expand Up @@ -541,7 +541,14 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
return reportAndCreateFailureResult(env, e.getMessage(), Code.INVALID_ARGUMENTS);
}

return BlazeCommandResult.success();
if (moduleInspector.getErrors().isEmpty()) {
return BlazeCommandResult.success();
} else {
return reportAndCreateFailureResult(
env,
"Not all extensions have been processed due to errors",
Code.ERROR_DURING_GRAPH_INSPECTION);
}
}

private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue modTidyValue) {
Expand Down Expand Up @@ -587,7 +594,14 @@ private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue mod
env.getReporter().handle(Event.info(fixupEvent.getSuccessMessage()));
}

return BlazeCommandResult.success();
if (modTidyValue.errors().isEmpty()) {
return BlazeCommandResult.success();
} else {
return reportAndCreateFailureResult(
env,
"Not all extensions have been processed due to errors",
Code.ERROR_DURING_GRAPH_INSPECTION);
}
}

/** Collects a list of {@link ModuleArg} into a set of {@link ModuleKey}s. */
Expand Down
1 change: 1 addition & 0 deletions src/main/protobuf/failure_details.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1372,6 +1372,7 @@ message ModCommand {
TOO_MANY_ARGUMENTS = 2 [(metadata) = { exit_code: 2 }];
INVALID_ARGUMENTS = 3 [(metadata) = { exit_code: 2 }];
BUILDOZER_FAILED = 4 [(metadata) = { exit_code: 2 }];
ERROR_DURING_GRAPH_INSPECTION = 5 [(metadata) = {exit_code: 2}];
}

Code code = 1;
Expand Down
Loading