From 455ddb74f79a5eb9b47ad212a8d439f56fd58a5f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 19 Dec 2024 13:18:01 -0800 Subject: [PATCH] Don't show fixup warnings during `bazel mod tidy` The warnings tell the user to run `bazel mod tidy`, which is very confusing. Fixes #24495 Closes #24729. PiperOrigin-RevId: 708007222 Change-Id: I60dc889281a776bbf08a7f9537272d7692cce1d8 --- .../devtools/build/lib/bazel/bzlmod/BUILD | 1 + .../bazel/bzlmod/ModuleExtensionMetadata.java | 20 ++++++++----------- .../lib/bazel/bzlmod/RootModuleFileFixup.java | 4 +++- .../bzlmod/SingleExtensionEvalFunction.java | 3 +-- .../bazel/bzlmod/SingleExtensionFunction.java | 4 ++++ src/test/py/bazel/bzlmod/mod_command_test.py | 16 ++++++--------- 6 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index 9127d464f26a9a..9f26f685f383c7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -458,6 +458,7 @@ java_library( ], deps = [ ":module_extension", + "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:guava", ], diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index 50a6644aca8ef6..3c267fff16e6ef 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -26,7 +26,6 @@ import com.google.devtools.build.lib.bazel.bzlmod.ModuleExtensionUsage.Proxy; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; -import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.vfs.PathFragment; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; import java.util.ArrayList; @@ -171,8 +170,7 @@ static ModuleExtensionMetadata create( } public Optional generateFixup( - ModuleExtensionUsage rootUsage, Set allRepos, EventHandler eventHandler) - throws EvalException { + ModuleExtensionUsage rootUsage, Set allRepos) throws EvalException { var rootModuleDirectDevDeps = getRootModuleDirectDevDeps(allRepos); var rootModuleDirectDeps = getRootModuleDirectDeps(allRepos); if (rootModuleDirectDevDeps.isEmpty() && rootModuleDirectDeps.isEmpty()) { @@ -193,19 +191,14 @@ public Optional generateFixup( } return generateFixup( - rootUsage, - allRepos, - rootModuleDirectDeps.get(), - rootModuleDirectDevDeps.get(), - eventHandler); + rootUsage, allRepos, rootModuleDirectDeps.get(), rootModuleDirectDevDeps.get()); } private static Optional generateFixup( ModuleExtensionUsage rootUsage, Set allRepos, Set expectedImports, - Set expectedDevImports, - EventHandler eventHandler) { + Set expectedDevImports) { var actualDevImports = rootUsage.getProxies().stream() .filter(p -> p.isDevDependency()) @@ -329,8 +322,11 @@ private static Optional generateFixup( } } - eventHandler.handle(Event.warn(rootUsage.getProxies().getFirst().getLocation(), message)); - return Optional.of(new RootModuleFileFixup(moduleFilePathToCommandsBuilder.build(), rootUsage)); + return Optional.of( + new RootModuleFileFixup( + moduleFilePathToCommandsBuilder.build(), + rootUsage, + Event.warn(rootUsage.getProxies().getFirst().getLocation(), message))); } private static String makeUseRepoCommand(String cmd, String proxyName, Collection repos) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java index b745bf0700259b..c55e6dd9e18310 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RootModuleFileFixup.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.bazel.bzlmod; import com.google.common.collect.ImmutableListMultimap; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.vfs.PathFragment; /** @@ -29,7 +30,8 @@ */ public record RootModuleFileFixup( ImmutableListMultimap moduleFilePathToBuildozerCommands, - ModuleExtensionUsage usage) { + ModuleExtensionUsage usage, + Event warning) { /** A human-readable message describing the fixup after it has been applied. */ public String getSuccessMessage() { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index bd9904922e6da5..68c45e3dd773f3 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -459,8 +459,7 @@ private SingleExtensionValue createSingleExtensionValue( .get() .generateFixup( usagesValue.getExtensionUsages().get(ModuleKey.ROOT), - generatedRepoSpecs.keySet(), - env.getListener()); + generatedRepoSpecs.keySet()); } catch (EvalException e) { env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack())); throw new SingleExtensionEvalFunctionException( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java index 91032158b18e5e..86c8eb79656769 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionFunction.java @@ -48,6 +48,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } + // SingleExtensionEvalFunction doesn't handle the fixup warning so that bazel mod tidy doesn't + // show it. + evalOnlyValue.fixup().ifPresent(fixup -> env.getListener().handle(fixup.warning())); + // Check that all imported repos have actually been generated. for (ModuleExtensionUsage usage : usagesValue.getExtensionUsages().values()) { for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) { diff --git a/src/test/py/bazel/bzlmod/mod_command_test.py b/src/test/py/bazel/bzlmod/mod_command_test.py index 0aac89227245ba..e62ae55debde1f 100644 --- a/src/test/py/bazel/bzlmod/mod_command_test.py +++ b/src/test/py/bazel/bzlmod/mod_command_test.py @@ -735,13 +735,13 @@ def testModTidy(self): # The extensions should not be reevaluated by the command. self.assertNotIn('ext1 is being evaluated', stderr) self.assertNotIn('ext2 is being evaluated', stderr) - # The fixup warnings should be shown again due to Skyframe replaying. - self.assertIn( + # bazel mod tidy doesn't show fixup warnings. + self.assertNotIn( 'Not imported, but reported as direct dependencies by the extension' ' (may cause the build to fail):\nmissing_dep', stderr, ) - self.assertIn( + self.assertNotIn( 'Imported, but reported as indirect dependencies by the' ' extension:\nindirect_dep', stderr, @@ -1085,13 +1085,13 @@ def testModTidyKeepGoing(self): # The passing extension should not be reevaluated by the command. self.assertNotIn('ext1 is being evaluated', stderr) self.assertIn('ext2 is being evaluated', stderr) - # The fixup warnings should be shown again due to Skyframe replaying. - self.assertIn( + # baze mod tidy doesn't show fixup warnings. + self.assertNotIn( 'Not imported, but reported as direct dependencies by the extension' ' (may cause the build to fail):\nmissing_dep', stderr, ) - self.assertIn( + self.assertNotIn( 'Imported, but reported as indirect dependencies by the' ' extension:\nindirect_dep', stderr, @@ -1178,10 +1178,6 @@ def testModTidyFixesInvalidImport(self): # extension fails after evaluation. _, _, stderr = self.RunBazel(['mod', 'tidy']) stderr = '\n'.join(stderr) - self.assertIn( - 'ext defined in @//:extension.bzl reported incorrect imports', stderr - ) - self.assertIn('invalid_dep', stderr) self.assertIn( 'INFO: Updated use_repo calls for @//:extension.bzl%ext', stderr )