From 6cdb42defa28083e16c088a7cb5f896cb94db2eb Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 17 Apr 2023 16:19:13 +0200 Subject: [PATCH] Address review comments --- .../bazel/bzlmod/ModuleExtensionMetadata.java | 15 +- .../bazel/bzlmod/ModuleExtensionUsage.java | 15 +- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 1 + .../build/lib/bazel/bzlmod/ModuleKey.java | 13 - .../devtools/build/lib/bazel/bzlmod/BUILD | 1 + .../bzlmod/BazelDepGraphFunctionTest.java | 1 + .../bzlmod/ModuleExtensionMetadataTest.java | 133 ---------- .../bzlmod/ModuleExtensionResolutionTest.java | 246 +++++++++++++++++- .../bazel/bzlmod/ModuleFileFunctionTest.java | 11 +- .../bazel/bzlmod/StarlarkBazelModuleTest.java | 1 + 10 files changed, 264 insertions(+), 173 deletions(-) delete mode 100644 src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadataTest.java 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 1ddbb1b1cfd11b..23cb5a0d4c60a8 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 @@ -1,6 +1,9 @@ package com.google.devtools.build.lib.bazel.bzlmod; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; + import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -129,8 +132,8 @@ public void evaluate(Collection usages, Set allRep Optional generateFixupMessage(Collection usages, Set allRepos) throws EvalException { var rootUsages = usages.stream() - .filter(usage -> usage.getModuleKey().equals(ModuleKey.ROOT)) - .collect(ImmutableList.toImmutableList()); + .filter(usage -> usage.getUsingModule().equals(ModuleKey.ROOT)) + .collect(toImmutableList()); if (rootUsages.isEmpty()) { // The root module doesn't use the current extension. Do not suggest fixes as the user isn't // expected to modify any other module's MODULE.bazel file. @@ -153,11 +156,11 @@ private static Optional generateFixupMessage(List r Set allRepos, Set expectedImports, Set expectedDevImports) { var actualDevImports = rootUsages.stream() .flatMap(usage -> usage.getDevImports().stream()) - .collect(ImmutableSet.toImmutableSet()); + .collect(toImmutableSet()); var actualImports = rootUsages.stream() .flatMap(usage -> usage.getImports().keySet().stream()) .filter(repo -> !actualDevImports.contains(repo)) - .collect(ImmutableSet.toImmutableSet()); + .collect(toImmutableSet()); // All label strings that map to the same Label are equivalent for buildozer as it implements // the same normalization of label strings with no or empty repo name. @@ -197,7 +200,7 @@ private static Optional generateFixupMessage(List r Sets.difference(allExpectedImports, allActualImports)); if (!missingImports.isEmpty()) { message += String.format( - "Not imported, but declared as direct dependencies (may cause the build to fail):\n %s\n\n", + "Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\n %s\n\n", String.join(", ", missingImports)); } @@ -205,7 +208,7 @@ private static Optional generateFixupMessage(List r Sets.difference(Sets.intersection(allActualImports, allRepos), allExpectedImports)); if (!indirectDepImports.isEmpty()) { message += String.format( - "Imported, but not declared as direct dependencies:\n %s\n\n", + "Imported, but reported as indirect dependencies by the extension:\n %s\n\n", String.join(", ", indirectDepImports)); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index be94bcdb095e06..405a5f8a3d0357 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -36,6 +36,9 @@ public abstract class ModuleExtensionUsage { /** The name of the extension. */ public abstract String getExtensionName(); + /** The module that contains this particular extension usage. */ + public abstract ModuleKey getUsingModule(); + /** * The location where this proxy object was created (by the {@code use_extension} call). Note that * if there were multiple {@code use_extension} calls on same extension, then this only stores the @@ -61,16 +64,6 @@ public abstract class ModuleExtensionUsage { */ public abstract ImmutableList getTags(); - public ModuleKey getModuleKey() { - String file = getLocation().file(); - Preconditions.checkState(file.endsWith("/MODULE.bazel")); - try { - return ModuleKey.fromString(file.substring(0, file.length() - "/MODULE.bazel".length())); - } catch (Version.ParseException e) { - throw new IllegalStateException("Unexpected location for module extension usage: " + file, e); - } - } - public static Builder builder() { return new AutoValue_ModuleExtensionUsage.Builder(); } @@ -85,6 +78,8 @@ public abstract static class Builder { public abstract Builder setExtensionName(String value); + public abstract Builder setUsingModule(ModuleKey value); + public abstract Builder setLocation(Location value); public abstract Builder setImports(ImmutableBiMap value); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index f598f3f06c655d..28b849c891f6a5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -452,6 +452,7 @@ ModuleExtensionUsage buildUsage() { return ModuleExtensionUsage.builder() .setExtensionBzlFile(extensionBzlFile) .setExtensionName(extensionName) + .setUsingModule(module.getKey()) .setLocation(location) .setImports(ImmutableBiMap.copyOf(imports)) .setDevImports(devImports.build()) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java index 30445e53dfbff0..09029703782caa 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java @@ -65,19 +65,6 @@ public final String toString() { return getName() + "@" + (getVersion().isEmpty() ? "_" : getVersion().toString()); } - public static ModuleKey fromString(String s) throws ParseException { - if (s.equals("")) { - return ROOT; - } - int at = s.indexOf('@'); - if (at == -1) { - throw new ParseException("Failed to parse version from non-root module key string"); - } - String name = s.substring(0, at); - String version = s.substring(at + 1); - return create(name, version.equals("_") ? Version.EMPTY : Version.parse(version)); - } - /** Returns the canonical name of the repo backing this module. */ public RepositoryName getCanonicalRepoName() { if (WELL_KNOWN_MODULES.containsKey(getName())) { diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index ef2c7d901b27cd..0d299f598cd4a7 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -78,6 +78,7 @@ java_library( "//src/main/java/net/starlark/java/syntax", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/testutil", + "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils", "//third_party:auto_value", "//third_party:caffeine", "//third_party:gson", diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java index f960521efcc2aa..d99e7c2956b196 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java @@ -236,6 +236,7 @@ private static ModuleExtensionUsage createModuleExtensionUsage( .setExtensionName(name) .setImports(importsBuilder.buildOrThrow()) .setDevImports(ImmutableSet.of()) + .setUsingModule(ModuleKey.ROOT) .setLocation(Location.BUILTIN) .build(); } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadataTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadataTest.java deleted file mode 100644 index c3740efe0170da..00000000000000 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadataTest.java +++ /dev/null @@ -1,133 +0,0 @@ -package com.google.devtools.build.lib.bazel.bzlmod; - -import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; - -import com.google.common.collect.ImmutableBiMap; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.events.EventKind; -import net.starlark.java.eval.EvalException; -import net.starlark.java.eval.Starlark; -import net.starlark.java.eval.StarlarkList; -import net.starlark.java.syntax.Location; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class ModuleExtensionMetadataTest { - @Test - public void testCreate_exactlyOneArgIsNone() { - Exception e = assertThrows( - EvalException.class, - () -> ModuleExtensionMetadata.create(Starlark.NONE, StarlarkList.immutableOf())); - assertThat(e).hasMessageThat().isEqualTo( - "root_module_direct_deps and root_module_direct_dev_deps must both be specified or both be unspecified"); - - e = assertThrows( - EvalException.class, - () -> ModuleExtensionMetadata.create(StarlarkList.immutableOf(), Starlark.NONE)); - assertThat(e).hasMessageThat().isEqualTo( - "root_module_direct_deps and root_module_direct_dev_deps must both be specified or both be unspecified"); - } - - private static final Location TEST_ROOT_LOCATION = Location.fromFileLineColumn("/MODULE.bazel", 2, 1); - private static final ImmutableList TEST_USAGES = ImmutableList.of( - ModuleExtensionUsage.builder() - .setExtensionBzlFile("@mod//:extensions.bzl") - .setExtensionName("ext") - .setLocation(TEST_ROOT_LOCATION) - .setImports(ImmutableBiMap.of("indirect_dep", "indirect_dep", "indirect_dev_dep", - "indirect_dev_dep", "invalid_dep", "invalid_dep", "invalid_dev_dep", "invalid_dev_dep")) - .setDevImports(ImmutableSet.of("indirect_dev_dep", "invalid_dev_dep")) - .build(), - ModuleExtensionUsage.builder() - .setExtensionBzlFile("@mod//:extensions.bzl") - .setExtensionName("ext") - .setLocation(Location.fromFileLineColumn("mod@1.2.3/MODULE.bazel", 2, 1)) - .setImports(ImmutableBiMap.of("indirect_dep", "indirect_dep")) - .setDevImports(ImmutableSet.of()) - .build() - ); - private static final ImmutableSet TEST_ALL_REPOS = ImmutableSet.of("direct_dep", - "direct_dev_dep", "indirect_dep", "indirect_dev_dep"); - - @Test - public void testGenerateFixupMessage() throws EvalException { - var moduleExtensionMetadata = ModuleExtensionMetadata.create( - StarlarkList.immutableOf("direct_dep"), - StarlarkList.immutableOf("direct_dev_dep")); - var fixupMessage = moduleExtensionMetadata.generateFixupMessage(TEST_USAGES, TEST_ALL_REPOS); - - assertThat(fixupMessage.isPresent()).isTrue(); - assertThat(fixupMessage.get().getLocation()).isEqualTo(TEST_ROOT_LOCATION); - assertThat(fixupMessage.get().getKind()).isEqualTo(EventKind.WARNING); - assertThat(fixupMessage.get().getMessage()).isEqualTo( - "The module extension ext defined in @mod//:extensions.bzl reported incorrect imports of repositories via use_repo():\n" - + "\n" - + "Imported, but not created by the extension (will cause the build to fail):\n" - + " invalid_dep, invalid_dev_dep\n" - + "\n" - + "Not imported, but declared as direct dependencies (may cause the build to fail):\n" - + " direct_dep, direct_dev_dep\n" - + "\n" - + "Imported, but not declared as direct dependencies:\n" - + " indirect_dep, indirect_dev_dep\n" - + "\n" - + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these issues:\033[0m\n" - + "\n" - + "buildozer 'use_repo_add @mod//:extensions.bzl ext direct_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove @mod//:extensions.bzl ext indirect_dep invalid_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_add dev @mod//:extensions.bzl ext direct_dev_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove dev @mod//:extensions.bzl ext indirect_dev_dep invalid_dev_dep' //MODULE.bazel:all"); - } - - @Test - public void testGenerateFixupMessage_useAllRepos() throws EvalException { - var moduleExtensionMetadata = ModuleExtensionMetadata.create("all", StarlarkList.immutableOf()); - var fixupMessage = moduleExtensionMetadata.generateFixupMessage(TEST_USAGES, TEST_ALL_REPOS); - - assertThat(fixupMessage.isPresent()).isTrue(); - assertThat(fixupMessage.get().getLocation()).isEqualTo(TEST_ROOT_LOCATION); - assertThat(fixupMessage.get().getKind()).isEqualTo(EventKind.WARNING); - assertThat(fixupMessage.get().getMessage()).isEqualTo( - "The module extension ext defined in @mod//:extensions.bzl reported incorrect imports of repositories via use_repo():\n" - + "\n" - + "Imported, but not created by the extension (will cause the build to fail):\n" - + " invalid_dep, invalid_dev_dep\n" - + "\n" - + "Not imported, but declared as direct dependencies (may cause the build to fail):\n" - + " direct_dep, direct_dev_dep\n" - + "\n" - + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these issues:\033[0m\n" - + "\n" - + "buildozer 'use_repo_add @mod//:extensions.bzl ext direct_dep direct_dev_dep indirect_dev_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove @mod//:extensions.bzl ext invalid_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove dev @mod//:extensions.bzl ext indirect_dev_dep invalid_dev_dep' //MODULE.bazel:all"); - } - - @Test - public void testGenerateFixupMessage_useAllRepos_dev() throws EvalException { - var moduleExtensionMetadata = ModuleExtensionMetadata.create(Starlark.NONE, "all"); - var fixupMessage = moduleExtensionMetadata.generateFixupMessage(TEST_USAGES, TEST_ALL_REPOS); - - assertThat(fixupMessage.isPresent()).isTrue(); - assertThat(fixupMessage.get().getLocation()).isEqualTo(TEST_ROOT_LOCATION); - assertThat(fixupMessage.get().getKind()).isEqualTo(EventKind.WARNING); - assertThat(fixupMessage.get().getMessage()).isEqualTo( - "The module extension ext defined in @mod//:extensions.bzl reported incorrect imports of repositories via use_repo():\n" - + "\n" - + "Imported, but not created by the extension (will cause the build to fail):\n" - + " invalid_dep, invalid_dev_dep\n" - + "\n" - + "Not imported, but declared as direct dependencies (may cause the build to fail):\n" - + " direct_dep, direct_dev_dep\n" - + "\n" - + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these issues:\033[0m\n" - + "\n" - + "buildozer 'use_repo_remove @mod//:extensions.bzl ext indirect_dep invalid_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_add dev @mod//:extensions.bzl ext direct_dep direct_dev_dep indirect_dep' //MODULE.bazel:all\n" - + "buildozer 'use_repo_remove dev @mod//:extensions.bzl ext invalid_dev_dep' //MODULE.bazel:all"); - } -} diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 094c3406e517f5..9003996d4af31b 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.bazel.bzlmod.BzlmodTestUtil.createModuleKey; +import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount; import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.base.Suppliers; @@ -38,6 +39,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; @@ -76,6 +78,7 @@ import com.google.devtools.build.lib.skyframe.WorkspaceFileFunction; import com.google.devtools.build.lib.starlarkbuildapi.repository.RepositoryBootstrap; import com.google.devtools.build.lib.testutil.FoundationTestCase; +import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.FileStateKey; @@ -95,6 +98,7 @@ import com.google.devtools.build.skyframe.SkyKey; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -1560,33 +1564,257 @@ public void extensionMetadata() throws Exception { "bazel_dep(name='ext', version='1.0')", "bazel_dep(name='data_repo',version='1.0')", "ext = use_extension('@ext//:defs.bzl', 'ext')", - "use_repo(ext, 'indirect_dep', 'indirect_dep', 'indirect_dev_dep', 'indirect_dev_dep', 'invalid_dep', 'invalid_dep', 'invalid_dev_dep', 'invalid_dev_dep')"); + "use_repo(ext, 'direct_dep', 'indirect_dep', 'invalid_dep')", + "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'direct_dev_dep', 'indirect_dev_dep', 'invalid_dev_dep')"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); scratch.file( - workspaceRoot.getRelative("defs.bzl").getPathString(), + workspaceRoot.getRelative("data.bzl").getPathString(), + "load('@direct_dep//:data.bzl', direct_dep_data='data')", + "data = direct_dep_data"); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')", + "ext_dev = use_extension('//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'indirect_dev_dep')"); + scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), "load('@data_repo//:defs.bzl','data_repo')", "def _ext_impl(ctx):", - " data_repo(name='summarized_candy')", - "my_ext=module_extension(implementation=_ext_impl)"); + " data_repo(name='direct_dep')", + " data_repo(name='direct_dev_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='missing_direct_dev_dep')", + " data_repo(name='indirect_dep')", + " data_repo(name='indirect_dev_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps=['direct_dep', 'missing_direct_dep'],", + " root_module_direct_dev_deps=['direct_dev_dep', 'missing_direct_dev_dep'],", + " )", + "ext=module_extension(implementation=_ext_impl)"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + // Evaluation fails due to the import of a repository not generated by the extension, but we + // only want to assert that the warning is emitted. + reporter.removeHandler(failFastHandler); + EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), + evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertEventCount(1, eventCollector); + assertContainsEvent( + "WARNING /MODULE.bazel:3:20: The module extension ext defined in @ext//:defs.bzl reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Imported, but not created by the extension (will cause the build to fail):\n" + + " invalid_dep, invalid_dev_dep\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\n" + + " missing_direct_dep, missing_direct_dev_dep\n" + + "\n" + + "Imported, but reported as indirect dependencies by the extension:\n" + + " indirect_dep, indirect_dev_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_add @ext//:defs.bzl ext missing_direct_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove @ext//:defs.bzl ext indirect_dep invalid_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_add dev @ext//:defs.bzl ext missing_direct_dev_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext indirect_dev_dep invalid_dev_dep' //MODULE.bazel:all", + Set.of(EventKind.WARNING)); + } + @Test + public void extensionMetadata_all() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "bazel_dep(name='ext', version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('@ext//:defs.bzl', 'ext')", + "use_repo(ext, 'direct_dep', 'indirect_dep', 'invalid_dep')", + "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'direct_dev_dep', 'indirect_dev_dep', 'invalid_dev_dep')"); scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); scratch.file( workspaceRoot.getRelative("data.bzl").getPathString(), - "load('@summarized_candy//:data.bzl', data='data')", - "candy_data = 'candy: ' + data"); + "load('@direct_dep//:data.bzl', direct_dep_data='data')", + "data = direct_dep_data"); registry.addModule( createModuleKey("ext", "1.0"), "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')", + "ext_dev = use_extension('//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'indirect_dev_dep')"); + scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), + "load('@data_repo//:defs.bzl','data_repo')", + "def _ext_impl(ctx):", + " data_repo(name='direct_dep')", + " data_repo(name='direct_dev_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='missing_direct_dev_dep')", + " data_repo(name='indirect_dep')", + " data_repo(name='indirect_dev_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps='all',", + " root_module_direct_dev_deps=[],", + " )", + "ext=module_extension(implementation=_ext_impl)"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + reporter.removeHandler(failFastHandler); + EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), + evaluationContext); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()).hasMessageThat().isEqualTo( + "module extension \"ext\" from \"@ext~1.0//:defs.bzl\" does not generate repository " + + "\"invalid_dep\", yet it is imported as \"invalid_dep\" in the usage at " + + "/MODULE.bazel:3:20"); + + assertEventCount(1, eventCollector); + assertContainsEvent( + "WARNING /MODULE.bazel:3:20: The module extension ext defined in @ext//:defs.bzl reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Imported, but not created by the extension (will cause the build to fail):\n" + + " invalid_dep, invalid_dev_dep\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\n" + + " missing_direct_dep, missing_direct_dev_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_add @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep missing_direct_dep missing_direct_dev_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove @ext//:defs.bzl ext invalid_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep invalid_dev_dep' //MODULE.bazel:all", + Set.of(EventKind.WARNING)); + } + + @Test + public void extensionMetadata_allDev() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "bazel_dep(name='ext', version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('@ext//:defs.bzl', 'ext')", + "use_repo(ext, 'direct_dep', 'indirect_dep', 'invalid_dep')", + "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'direct_dev_dep', 'indirect_dev_dep', 'invalid_dev_dep')"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + "load('@direct_dep//:data.bzl', direct_dep_data='data')", + "data = direct_dep_data"); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')", + "ext_dev = use_extension('//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'indirect_dev_dep')"); + scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), + "load('@data_repo//:defs.bzl','data_repo')", + "def _ext_impl(ctx):", + " data_repo(name='direct_dep')", + " data_repo(name='direct_dev_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='missing_direct_dev_dep')", + " data_repo(name='indirect_dep')", + " data_repo(name='indirect_dev_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps=[],", + " root_module_direct_dev_deps='all',", + " )", + "ext=module_extension(implementation=_ext_impl)"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + // Evaluation fails due to the import of a repository not generated by the extension, but we + // only want to assert that the warning is emitted. + reporter.removeHandler(failFastHandler); + EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), + evaluationContext); + assertThat(result.hasError()).isTrue(); + assertThat(result.getError().getException()).hasMessageThat().isEqualTo( + "module extension \"ext\" from \"@ext~1.0//:defs.bzl\" does not generate repository " + + "\"invalid_dep\", yet it is imported as \"invalid_dep\" in the usage at " + + "/MODULE.bazel:3:20"); + + assertEventCount(1, eventCollector); + assertContainsEvent( + "WARNING /MODULE.bazel:3:20: The module extension ext defined in @ext//:defs.bzl reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Imported, but not created by the extension (will cause the build to fail):\n" + + " invalid_dep, invalid_dev_dep\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the build to fail):\n" + + " missing_direct_dep, missing_direct_dev_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_remove @ext//:defs.bzl ext direct_dep indirect_dep invalid_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_add dev @ext//:defs.bzl ext direct_dep indirect_dep missing_direct_dep missing_direct_dev_dep' //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext invalid_dev_dep' //MODULE.bazel:all", + Set.of(EventKind.WARNING)); + } + + @Test + public void extensionMetadata_noRootUsage() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "bazel_dep(name='ext', version='1.0')", "bazel_dep(name='data_repo',version='1.0')"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')", + "ext_dev = use_extension('//:defs.bzl', 'ext', dev_dependency = True)", + "use_repo(ext_dev, 'indirect_dev_dep')"); scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); scratch.file( modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), "load('@data_repo//:defs.bzl','data_repo')", "def _ext_impl(ctx):", - " data_repo(name='candy', data='cotton candy')", - " data_repo(name='exposed_candy', data='lollipops')", - "ext = module_extension(implementation=_ext_impl)"); + " data_repo(name='direct_dep')", + " data_repo(name='direct_dev_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='missing_direct_dev_dep')", + " data_repo(name='indirect_dep', data='indirect_dep_data')", + " data_repo(name='indirect_dev_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps='all',", + " root_module_direct_dev_deps=[],", + " )", + "ext=module_extension(implementation=_ext_impl)"); + scratch.file( + modulesRoot.getRelative("ext~1.0/data.bzl").getPathString(), + "load('@indirect_dep//:data.bzl', indirect_dep_data='data')", + "data = indirect_dep_data"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("@ext~1.0//:data.bzl")); + EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), + evaluationContext); + assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("indirect_dep_data"); + + assertEventCount(0, eventCollector); } private EvaluationResult evaluateSimpleModuleExtension( diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index 7e6985b991c971..77653ef3333710 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -459,7 +459,8 @@ public void testModuleExtensions_good() throws Exception { "maven.dep(coord='guava')"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); - SkyKey skyKey = ModuleFileValue.key(createModuleKey("mymod", "1.0"), null); + ModuleKey myMod = createModuleKey("mymod", "1.0"); + SkyKey skyKey = ModuleFileValue.key(myMod, null); EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); if (result.hasError()) { @@ -475,6 +476,7 @@ public void testModuleExtensions_good() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext1") + .setUsingModule(myMod) .setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 2, 23)) .setImports(ImmutableBiMap.of("repo1", "repo1")) .setDevImports(ImmutableSet.of()) @@ -495,6 +497,7 @@ public void testModuleExtensions_good() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext2") + .setUsingModule(myMod) .setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 5, 23)) .setImports(ImmutableBiMap.of("other_repo1", "repo1", "repo2", "repo2")) .setDevImports(ImmutableSet.of()) @@ -527,6 +530,7 @@ public void testModuleExtensions_good() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@rules_jvm_external//:defs.bzl") .setExtensionName("maven") + .setUsingModule(myMod) .setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 10, 22)) .setImports( ImmutableBiMap.of("mvn", "maven", "junit", "junit", "guava", "guava")) @@ -592,6 +596,7 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@//:defs.bzl") .setExtensionName("myext") + .setUsingModule(ModuleKey.ROOT) .setLocation(Location.fromFileLineColumn("/MODULE.bazel", 1, 23)) .setImports( ImmutableBiMap.of( @@ -672,7 +677,8 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { "use_repo(myext4, 'delta')"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); - SkyKey skyKey = ModuleFileValue.key(createModuleKey("mymod", "1.0"), null); + ModuleKey myMod = createModuleKey("mymod", "1.0"); + SkyKey skyKey = ModuleFileValue.key(myMod, null); EvaluationResult result = evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); if (result.hasError()) { @@ -687,6 +693,7 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext") + .setUsingModule(myMod) .setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 5, 23)) .setImports(ImmutableBiMap.of("beta", "beta", "delta", "delta")) .setDevImports(ImmutableSet.of()) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java index f3e7b420710cf8..5f1f95f8315ae8 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java @@ -44,6 +44,7 @@ private static ModuleExtensionUsage.Builder getBaseUsageBuilder() { return ModuleExtensionUsage.builder() .setExtensionBzlFile("//:rje.bzl") .setExtensionName("maven") + .setUsingModule(ModuleKey.ROOT) .setLocation(Location.BUILTIN) .setImports(ImmutableBiMap.of()) .setDevImports(ImmutableSet.of());