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 8edaa2957afcc6..435274306788b6 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 @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java index bd2a41da6fc8e9..7c9d20d7347226 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java +++ b/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 fixups = ImmutableList.builder(); + ImmutableList.Builder 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()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java index b5dd58f6d1569a..11ba16507b858f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyValue.java +++ b/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 moduleFilePaths(); + /** Errors encountered while evaluating prerequisites for {@code bazel mod tidy}. */ + public abstract ImmutableList errors(); + static BazelModTidyValue create( List fixups, Path buildozer, - ImmutableSet moduleFilePaths) { + ImmutableSet moduleFilePaths, + ImmutableList errors) { return new AutoValue_BazelModTidyValue( - ImmutableList.copyOf(fixups), buildozer, moduleFilePaths); + ImmutableList.copyOf(fixups), buildozer, moduleFilePaths, errors); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java index 26d06ea70eed1c..b94a07663c0eec 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java +++ b/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 depGraph = computeAugmentedGraph(unprunedDepGraph, resolvedDepGraph.keySet(), overrides); - ImmutableSetMultimap 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 computeAugmentedGraph( @@ -180,9 +181,13 @@ public static ImmutableMap computeAugmentedGraph( .collect(toImmutableMap(Entry::getKey, e -> e.getValue().build())); } + private record ExtensionRepos( + ImmutableSetMultimap extensionToRepoInternalNames, + ImmutableList errors) {} + @Nullable - private ImmutableSetMultimap computeExtensionToRepoInternalNames( - BazelDepGraphValue depGraphValue, Environment env) throws InterruptedException { + private ExtensionRepos computExtensionRepos(BazelDepGraphValue depGraphValue, Environment env) + throws InterruptedException { ImmutableSet extensionEvalKeys = depGraphValue.getExtensionUsagesTable().rowKeySet(); ImmutableList singleExtensionKeys = @@ -191,15 +196,26 @@ private ImmutableSetMultimap computeExtensionToRepoIn ImmutableSetMultimap.Builder extensionToRepoInternalNames = ImmutableSetMultimap.builder(); + ImmutableList.Builder 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()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorValue.java index 6df76aa8913ce1..16f1c3ebe4fc54 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorValue.java +++ b/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 depGraph, ImmutableMap> modulesIndex, ImmutableSetMultimap extensionToRepoInternalNames, - ImmutableMap moduleKeyToCanonicalNames) { + ImmutableMap moduleKeyToCanonicalNames, + ImmutableList 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 getModuleKeyToCanonicalNames(); + /** A list of exceptions that occurred during module graph inspection. */ + public abstract ImmutableList getErrors(); + /** * A wrapper for {@link Module}, augmented with references to dependants (and also those who are * not used in the final dep graph). 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 index af3acd8d9aeb8c..86242df0ca454f 100644 --- 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 @@ -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 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); } diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 7df8ca4ea13e73..979f8cb717137f 100644 --- a/src/main/protobuf/failure_details.proto +++ b/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; diff --git a/src/test/py/bazel/bzlmod/mod_command_test.py b/src/test/py/bazel/bzlmod/mod_command_test.py index 32a1dd4179813c..0aac89227245ba 100644 --- a/src/test/py/bazel/bzlmod/mod_command_test.py +++ b/src/test/py/bazel/bzlmod/mod_command_test.py @@ -120,6 +120,8 @@ def setUp(self): ')', 'def _ext_impl(ctx):', ' deps = {dep.name: 1 for mod in ctx.modules for dep in mod.tags.dep}', + ' if "fail" in deps:', + ' fail("ext failed")', ' for dep in deps:', ' data_repo(name=dep, data="requested repo")', 'ext=module_extension(_ext_impl,', @@ -239,6 +241,57 @@ def testGraphWithExtensionFilter(self): 'wrong output in graph query with extension filter specified', ) + def testGraphWithFailingExtensions(self): + # Force ext2 to fail. + with open(self.Path('MODULE.bazel'), 'a+') as f: + f.write("ext2.dep(name = 'repo2')\n") + f.write("ext2.dep(name = 'fail')\n") + + exit_code, stdout, stderr = self.RunBazel( + ['mod', 'graph', '--extension_info=all'], + rstrip=True, + allow_failure=True, + ) + self.AssertNotExitCode(exit_code, 0, stderr) + self.assertIn( + 'ERROR: Results may be incomplete as 1 extension failed.', stderr + ) + self.assertIn('\t\tfail("ext failed")', stderr) + self.assertIn('Error in fail: ext failed', stderr) + self.assertListEqual( + stdout, + [ + ' (my_project@1.0)', + '|___$@@ext+//:ext.bzl%ext', + '| |___repo1', + '| |...repo2', + '| |...repo5', + '|___$@@ext2+//:ext.bzl%ext', + '| |___repo1', + '|___ext@1.0', + '|___ext2@1.0', + '|___foo@1.0', + '| |___$@@ext+//:ext.bzl%ext ...', + '| | |___repo1', + '| |___ext@1.0 (*)', + '| |___bar@2.0', + '| |___$@@ext+//:ext.bzl%ext ...', + '| | |___repo3', + '| |___$@@ext2+//:ext.bzl%ext ...', + '| | |___repo3', + '| |___ext@1.0 (*)', + '| |___ext2@1.0 (*)', + '|___foo@2.0', + ' |___$@@ext+//:ext.bzl%ext ...', + ' | |___repo3', + ' | |___repo4', + ' |___bar@2.0 (*)', + ' |___ext@1.0 (*)', + '', + ], + 'wrong output in graph with extensions query', + ) + def testShowExtensionAllUsages(self): _, stdout, _ = self.RunBazel( ['mod', 'show_extension', '@ext//:ext.bzl%ext'], rstrip=True @@ -935,7 +988,7 @@ def testModTidyFailsOnExtensionFailure(self): # Verify that bazel mod tidy fails if an extension fails to execute. exit_code, _, stderr = self.RunBazel(['mod', 'tidy'], allow_failure=True) - self.assertNotEqual(0, exit_code) + self.AssertNotExitCode(exit_code, 0, stderr) stderr = '\n'.join(stderr) self.assertIn('//:extension.bzl', stderr) self.assertIn('Error: index out of range', stderr) @@ -951,6 +1004,147 @@ def testModTidyFailsOnExtensionFailure(self): module_file.read().split('\n'), ) + def testModTidyKeepGoing(self): + self.ScratchFile( + 'MODULE.bazel', + [ + 'ext1 = use_extension("//:extension.bzl", "ext1")', + 'use_repo(ext1, "dep", "indirect_dep")', + 'ext2 = use_extension("//:extension.bzl", "ext2")', + 'use_repo(ext2, "other_dep", "other_indirect_dep")', + ], + ) + self.ScratchFile('BUILD.bazel') + self.ScratchFile( + 'extension.bzl', + [ + 'def _repo_rule_impl(ctx):', + ' ctx.file("WORKSPACE")', + ' ctx.file("BUILD", "filegroup(name=\'lala\')")', + '', + 'repo_rule = repository_rule(implementation=_repo_rule_impl)', + '', + 'def _ext1_impl(ctx):', + ' print("ext1 is being evaluated")', + ' repo_rule(name="dep")', + ' repo_rule(name="missing_dep")', + ' repo_rule(name="indirect_dep")', + ' return ctx.extension_metadata(', + ' root_module_direct_deps=["dep", "missing_dep"],', + ' root_module_direct_dev_deps=[],', + ' )', + '', + 'ext1 = module_extension(implementation=_ext1_impl)', + '', + 'def _ext2_impl(ctx):', + ' print("ext2 is being evaluated")', + ' fail("ext2 failed")', + '', + 'ext2 = module_extension(implementation=_ext2_impl)', + ], + ) + + # Create a lockfile and let the extension evaluations emit fixup warnings. + exit_code, _, stderr = self.RunBazel( + [ + 'mod', + 'deps', + '--lockfile_mode=update', + ], + allow_failure=True, + ) + self.AssertNotExitCode(exit_code, 0, stderr) + stderr = '\n'.join(stderr) + self.assertIn('ext1 is being evaluated', stderr) + self.assertIn('ext2 is being evaluated', stderr) + self.assertIn('Error in fail: ext2 failed', stderr) + self.assertIn( + 'Not imported, but reported as direct dependencies by the extension' + ' (may cause the build to fail):\nmissing_dep', + stderr, + ) + self.assertIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + + # Run bazel mod tidy to fix the imports. + exit_code, stdout, stderr = self.RunBazel( + [ + 'mod', + 'tidy', + '--lockfile_mode=update', + ], + allow_failure=True, + ) + self.AssertNotExitCode(exit_code, 0, stderr) + self.assertEqual([], stdout) + self.assertIn('ERROR: Failed to process 1 extension due to errors.', stderr) + stderr = '\n'.join(stderr) + # 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( + 'Not imported, but reported as direct dependencies by the extension' + ' (may cause the build to fail):\nmissing_dep', + stderr, + ) + self.assertIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + # Fixes are reported. + self.assertIn( + 'INFO: Updated use_repo calls for @//:extension.bzl%ext1', stderr + ) + self.assertNotIn( + 'INFO: Updated use_repo calls for @//:extension.bzl%ext2', stderr + ) + + # Rerun bazel mod deps to check that the fixup warnings are gone + # and the lockfile is up-to-date. + exit_code, _, stderr = self.RunBazel( + [ + 'mod', + 'deps', + '--lockfile_mode=error', + ], + allow_failure=True, + ) + self.AssertNotExitCode(exit_code, 0, stderr) + # The exit code if the build fails due to a lockfile that is not up-to-date. + self.AssertNotExitCode(exit_code, 48, stderr) + stderr = '\n'.join(stderr) + self.assertNotIn('ext1 is being evaluated', stderr) + self.assertIn('ext2 is being evaluated', stderr) + self.assertNotIn( + 'Not imported, but reported as direct dependencies by the extension' + ' (may cause the build to fail):\nmissing_dep', + stderr, + ) + self.assertNotIn( + 'Imported, but reported as indirect dependencies by the' + ' extension:\nindirect_dep', + stderr, + ) + + # Verify that use_repo statements have been updated. + with open('MODULE.bazel', 'r') as module_file: + self.assertEqual( + [ + 'ext1 = use_extension("//:extension.bzl", "ext1")', + 'use_repo(ext1, "dep", "missing_dep")', + '', + 'ext2 = use_extension("//:extension.bzl", "ext2")', + 'use_repo(ext2, "other_dep", "other_indirect_dep")', + '', + ], + module_file.read().split('\n'), + ) + def testModTidyFixesInvalidImport(self): self.ScratchFile( 'MODULE.bazel',