From 4cbbc5409f1b9c15fa7a8282999ed388d189291d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 6 Nov 2024 12:42:51 +0100 Subject: [PATCH] --keep_going --- .../devtools/build/lib/bazel/bzlmod/BUILD | 5 + .../bazel/bzlmod/BazelModTidyFunction.java | 15 +- .../lib/bazel/bzlmod/BazelModTidyValue.java | 8 +- .../bzlmod/BazelModuleInspectorFunction.java | 34 ++- .../bzlmod/BazelModuleInspectorValue.java | 9 +- .../build/lib/bazel/commands/ModCommand.java | 26 ++- src/main/protobuf/failure_details.proto | 1 + src/test/py/bazel/bzlmod/mod_command_test.py | 200 ++++++++++++++++++ 8 files changed, 275 insertions(+), 23 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 8edaa2957afcc6..307ac69f606c49 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,6 +378,7 @@ java_library( ], deps = [ ":common", + ":exception", ":resolution", ":root_module_file_fixup", ":tidy", @@ -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", @@ -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", 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..900a6e72953e9e 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 @@ -91,8 +91,16 @@ 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); + continue; + } if (value == null) { return null; } @@ -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()); } } 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..21365aea2e187f 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 @@ -67,9 +67,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 +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 computeAugmentedGraph( @@ -180,9 +180,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 +195,25 @@ 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); + 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..17fd359f8407ee 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 @@ -56,7 +56,6 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; -import com.google.devtools.build.lib.pkgcache.PackageOptions; import com.google.devtools.build.lib.runtime.BlazeCommand; import com.google.devtools.build.lib.runtime.BlazeCommandResult; import com.google.devtools.build.lib.runtime.Command; @@ -100,10 +99,8 @@ @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 }, @@ -194,6 +191,7 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe EvaluationContext.newBuilder() .setParallelism(threadsOption.threads) .setEventHandler(env.getReporter()) + .setKeepGoing(options.getOptions(KeepGoingOption.class).keepGoing) .build(); try { @@ -208,11 +206,11 @@ private BlazeCommandResult execInternal(CommandEnvironment env, OptionsParsingRe keys.add(BazelDepGraphValue.KEY, BazelModuleInspectorValue.KEY); } EvaluationResult 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(); } @@ -541,7 +539,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) { @@ -587,7 +592,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. */ diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 7df8ca4ea13e73..123f0d4b23a140 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..335cf5a59539c5 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,69 @@ 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('\t\tfail("ext failed")', stderr) + self.assertIn('Error in fail: ext failed', stderr) + self.assertEmpty(stdout) + + def testGraphWithFailingExtensionsKeepGoing(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', '--keep_going'], + rstrip=True, allow_failure=True + ) + self.AssertNotExitCode(exit_code, 0, stderr) + self.assertIn('Not all extensions have been processed due to errors', '\n'.join(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 @@ -951,6 +1016,141 @@ 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', + '--keep_going', + ], 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', + '--keep_going', + ], allow_failure=True) + self.AssertNotExitCode(exit_code, 0, stderr) + self.assertEqual([], stdout) + stderr = '\n'.join(stderr) + self.assertIn('Not all extensions have been processed due to errors', 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', + '--keep_going', + ], 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',