[8.0.0] Add support for --keep_going to bazel mod (#24259)

RELNOTES: `bazel mod` now tries to evaluate all module extensions, even
when some have failed to evaluate.

Closes #23828.

PiperOrigin-RevId: 694448503
Change-Id: I1e4f7a36384c7dfdc16d13cdfcd0b4b3aeb41834

Commit
a0471c7

Co-authored-by: Fabian Meumertzheim <[email protected]>

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD

@@ -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",
@@ -377,10 +378,12 @@ java_library(
     ],
     deps = [
         ":common",
+        ":exception",
         ":resolution",
         ":root_module_file_fixup",
         ":tidy",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
         "//src/main/java/com/google/devtools/build/lib/vfs",
@@ -398,6 +401,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",
@@ -415,9 +419,12 @@ java_library(
     ],
     deps = [
         ":common",
+        ":exception",
         ":inspection",
         ":module_extension",
         ":resolution",
+        "//src/main/java/com/google/devtools/build/lib/events",
+        "//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",

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java

@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
@@ -91,8 +92,17 @@ 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);
+        env.getListener().handle(Event.error(e.getMessage()));
+        continue;
+      }
       if (value == null) {
         return null;
       }
@@ -102,6 +112,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());
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java

@@ -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);
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java

@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -67,9 +68,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;
     }
 
@@ -85,8 +85,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(
@@ -180,9 +181,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 =
@@ -191,15 +196,26 @@ 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);
+        env.getListener().handle(Event.error(e.getMessage()));
+        continue;
+      }
       if (singleExtensionValue == null) {
         return null;
       }
       extensionToRepoInternalNames.putAll(
           singleExtensionKey.argument(), singleExtensionValue.getGeneratedRepoSpecs().keySet());
     }
-    return extensionToRepoInternalNames.build();
+    return new ExtensionRepos(extensionToRepoInternalNames.build(), errors.build());
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorValue.java

@@ -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;
@@ -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);
   }
 
   /**
@@ -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).

src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java

@@ -61,7 +61,6 @@
 import com.google.devtools.build.lib.runtime.BlazeCommandResult;
 import com.google.devtools.build.lib.runtime.Command;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
-import com.google.devtools.build.lib.runtime.KeepGoingOption;
 import com.google.devtools.build.lib.runtime.LoadingPhaseThreadsOption;
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
@@ -100,11 +99,9 @@
 @Command(
     name = ModCommand.NAME,
     buildPhase = LOADS,
-    // TODO(andreisolo): figure out which extra options are really needed
     options = {
       ModOptions.class,
       PackageOptions.class,
-      KeepGoingOption.class,
       LoadingPhaseThreadsOption.class
     },
     help = "resource:mod.txt",
@@ -212,7 +209,7 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
 
       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();
         }
@@ -541,7 +538,17 @@ 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,
+          String.format(
+              "Results may be incomplete as %d extension%s failed.",
+              moduleInspector.getErrors().size(),
+              moduleInspector.getErrors().size() == 1 ? "" : "s"),
+          Code.ERROR_DURING_GRAPH_INSPECTION);
+    }
   }
 
   private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue modTidyValue) {
@@ -572,7 +579,7 @@ private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue mod
       if (e instanceof AbnormalTerminationException abnormalTerminationException) {
         if (abnormalTerminationException.getResult().getTerminationStatus().getRawExitCode() == 3) {
           // Buildozer exits with exit code 3 if it didn't make any changes.
-          return BlazeCommandResult.success();
+          return reportAndCreateTidyResult(env, modTidyValue);
         }
         suffix =
             ":\n" + new String(((AbnormalTerminationException) e).getResult().getStderr(), UTF_8);
@@ -587,7 +594,21 @@ private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue mod
       env.getReporter().handle(Event.info(fixupEvent.getSuccessMessage()));
     }
 
-    return BlazeCommandResult.success();
+    return reportAndCreateTidyResult(env, modTidyValue);
+  }
+
+  private static BlazeCommandResult reportAndCreateTidyResult(
+      CommandEnvironment env, BazelModTidyValue modTidyValue) {
+    if (modTidyValue.errors().isEmpty()) {
+      return BlazeCommandResult.success();
+    } else {
+      return reportAndCreateFailureResult(
+          env,
+          String.format(
+              "Failed to process %d extension%s due to errors.",
+              modTidyValue.errors().size(), modTidyValue.errors().size() == 1 ? "" : "s"),
+          Code.ERROR_DURING_GRAPH_INSPECTION);
+    }
   }
 
   /** Collects a list of {@link ModuleArg} into a set of {@link ModuleKey}s. */
@@ -640,9 +661,13 @@ private static ImmutableSortedSet<ModuleExtensionId> extensionArgListToIds(
   private static BlazeCommandResult reportAndCreateFailureResult(
       CommandEnvironment env, String message, Code detailedCode) {
     String fullMessage =
-        String.format(
-            "%s%s Type '%s help mod' for syntax and help.",
-            message, message.endsWith(".") ? "" : ".", env.getRuntime().getProductName());
+        switch (detailedCode) {
+          case MISSING_ARGUMENTS, TOO_MANY_ARGUMENTS, INVALID_ARGUMENTS ->
+              String.format(
+                  "%s%s Type '%s help mod' for syntax and help.",
+                  message, message.endsWith(".") ? "" : ".", env.getRuntime().getProductName());
+          default -> message;
+        };
     env.getReporter().handle(Event.error(fullMessage));
     return createFailureResult(fullMessage, detailedCode);
   }

src/main/protobuf/failure_details.proto

@@ -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;