Address review comments

bazelbuild · Jan 30, 2024 · 1fdcced · 1fdcced
1 parent 674ba8a
commit 1fdcced
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 31 deletions.
diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java
@@ -160,7 +160,7 @@ public boolean rootModuleHasNonDevDependency() {
                     + " dependencies of the root module. If the root module imports additional"
                     + " repositories or does not import all of these repositories via <a"
                     + " href=\"../globals/module.html#use_repo\"><code>use_repo</code></a>, Bazel"
-                    + " will print a warning when the  extension is evaluated, instructing the user"
+                    + " will print a warning when the extension is evaluated, instructing the user"
                     + " to run <code>bazel mod tidy</code> to fix the <code>use_repo</code> calls"
                     + " automatically. <p>If one of <code>root_module_direct_deps</code> and"
                     + " will print a warning and a fixup command when the extension is"

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java b/src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java
@@ -119,9 +119,6 @@ public final class ModCommand implements BlazeCommand {
 
   public static final String NAME = "mod";
 
-  private final ArrayList<RootModuleFileFixupEvent> fixupEvents = new ArrayList<>();
-  @Nullable private BazelModuleResolutionEvent bazelModuleResolutionEvent = null;
-
   @Override
   public void editOptions(OptionsParser optionsParser) {
     try {
@@ -137,7 +134,6 @@ public void editOptions(OptionsParser optionsParser) {
 
   @Override
   public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) {
-    env.getEventBus().register(this);
     env.getEventBus()
         .post(
             new NoBuildEvent(
@@ -146,13 +142,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
                 /* separateFinishedEvent= */ true,
                 /* showProgress= */ true,
                 /* id= */ null));
-    BlazeCommandResult result;
-    try {
-      result = execInternal(env, options);
-    } finally {
-      fixupEvents.clear();
-      bazelModuleResolutionEvent = null;
-    }
+    BlazeCommandResult result = execInternal(env, options);
     env.getEventBus()
         .post(
             new NoBuildRequestFinishedEvent(
@@ -207,6 +197,7 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
     @Nullable BazelModuleInspectorValue moduleInspector;
     @Nullable BazelModTidyValue modTidyValue;
     ImmutableList<RepositoryMappingValue> repoMappingValues;
+    TidyEventRecorder tidyEventRecorder = new TidyEventRecorder();
 
     SkyframeExecutor skyframeExecutor = env.getSkyframeExecutor();
     LoadingPhaseThreadsOption threadsOption = options.getOptions(LoadingPhaseThreadsOption.class);
@@ -225,6 +216,7 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
         keys.addAll(repoMappingKeys);
       } else if (subcommand.equals(ModSubcommand.TIDY)) {
         keys.add(BazelModTidyValue.KEY);
+        env.getEventBus().register(tidyEventRecorder);
       } else {
         keys.add(BazelDepGraphValue.KEY, BazelModuleInspectorValue.KEY);
       }
@@ -289,9 +281,9 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
       // tidy doesn't take extra arguments.
       if (!args.isEmpty()) {
         return reportAndCreateFailureResult(
-            env, "the 'fix' command doesn't take extra arguments", Code.TOO_MANY_ARGUMENTS);
+            env, "the 'tidy' command doesn't take extra arguments", Code.TOO_MANY_ARGUMENTS);
       }
-      return runTidy(env, modTidyValue);
+      return runTidy(env, modTidyValue, tidyEventRecorder);
     }
 
     // Extract and check the --base_module argument first to use it when parsing the other args.
@@ -563,14 +555,30 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe
     return BlazeCommandResult.success();
   }
 
-  private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue modTidyValue) {
+  private static class TidyEventRecorder {
+    final List<RootModuleFileFixupEvent> fixupEvents = new ArrayList<>();
+    @Nullable BazelModuleResolutionEvent bazelModuleResolutionEvent;
+
+    @Subscribe
+    public void fixupGenerated(RootModuleFileFixupEvent event) {
+      fixupEvents.add(event);
+    }
+
+    @Subscribe
+    public void bazelModuleResolved(BazelModuleResolutionEvent event) {
+      bazelModuleResolutionEvent = event;
+    }
+  }
+
+  private BlazeCommandResult runTidy(
+      CommandEnvironment env, BazelModTidyValue modTidyValue, TidyEventRecorder eventRecorder) {
     CommandBuilder buildozerCommand =
         new CommandBuilder()
             .setWorkingDir(env.getWorkspace())
             .addArg(modTidyValue.buildozer().getPathString())
             .addArgs(
                 Stream.concat(
-                        fixupEvents.stream()
+                        eventRecorder.fixupEvents.stream()
                             .map(RootModuleFileFixupEvent::getBuildozerCommands)
                             .flatMap(Collection::stream),
                         Stream.of("format"))
@@ -590,7 +598,7 @@ private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue mod
           Code.BUILDOZER_FAILED);
     }
 
-    for (RootModuleFileFixupEvent fixupEvent : fixupEvents) {
+    for (RootModuleFileFixupEvent fixupEvent : eventRecorder.fixupEvents) {
       env.getReporter().handle(Event.info(fixupEvent.getSuccessMessage()));
     }
 
@@ -628,13 +636,15 @@ private BlazeCommandResult runTidy(CommandEnvironment env, BazelModTidyValue mod
             "Unexpected error parsing module file after running buildozer: " + e.getMessage(),
             Code.BUILDOZER_FAILED);
       }
+      // BazelModuleResolutionEvent is cached by Skyframe and thus always emitted.
       BazelModuleResolutionEvent updatedModuleResolutionEvent =
           BazelModuleResolutionEvent.create(
-              bazelModuleResolutionEvent.getOnDiskLockfileValue(),
-              bazelModuleResolutionEvent
+              eventRecorder.bazelModuleResolutionEvent.getOnDiskLockfileValue(),
+              eventRecorder
+                  .bazelModuleResolutionEvent
                   .getResolutionOnlyLockfileValue()
                   .withShallowlyReplacedRootModule(newRootModuleFileValue),
-              bazelModuleResolutionEvent.getExtensionUsagesById());
+              eventRecorder.bazelModuleResolutionEvent.getExtensionUsagesById());
       env.getReporter().post(updatedModuleResolutionEvent);
     }
 
@@ -710,14 +720,4 @@ public static void dumpRepoMappings(List<RepositoryMappingValue> repoMappings, W
     }
     writer.flush();
   }
-
-  @Subscribe
-  public void fixupGenerated(RootModuleFileFixupEvent event) {
-    fixupEvents.add(event);
-  }
-
-  @Subscribe
-  public void bazelModuleResolved(BazelModuleResolutionEvent event) {
-    bazelModuleResolutionEvent = event;
-  }
 }