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 4b7e27fbb2cbbf..500553ab50320b 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 @@ -49,7 +49,6 @@ import net.starlark.java.eval.StarlarkValue; import net.starlark.java.eval.Structure; import net.starlark.java.eval.Tuple; -import net.starlark.java.syntax.Location; /** A collection of global Starlark build API functions that apply to MODULE.bazel files. */ @GlobalMethods(environment = Environment.MODULE) @@ -171,11 +170,10 @@ public void module( } if (repoName.isEmpty()) { repoName = name; - context.addRepoNameUsage(name, "as the current module name", thread.getCallerLocation()); + context.addRepoNameUsage(name, "as the current module name", thread.getCallStack()); } else { RepositoryName.validateUserProvidedRepoName(repoName); - context.addRepoNameUsage( - repoName, "as the module's own repo name", thread.getCallerLocation()); + context.addRepoNameUsage(repoName, "as the module's own repo name", thread.getCallStack()); } Version parsedVersion; try { @@ -293,7 +291,7 @@ public void bazelDep( name, parsedVersion, maxCompatibilityLevel.toInt("max_compatibility_level"))); } - context.addRepoNameUsage(repoName, "by a bazel_dep", thread.getCallerLocation()); + context.addRepoNameUsage(repoName, "by a bazel_dep", thread.getCallStack()); } @StarlarkMethod( @@ -541,9 +539,13 @@ static class ModuleExtensionProxy implements Structure, StarlarkExportable { usageBuilder.addProxyBuilder(proxyBuilder); } - void addImport(String localRepoName, String exportedName, String byWhat, Location location) + void addImport( + String localRepoName, + String exportedName, + String byWhat, + ImmutableList stack) throws EvalException { - usageBuilder.addImport(localRepoName, exportedName, byWhat, location); + usageBuilder.addImport(localRepoName, exportedName, byWhat, stack); proxyBuilder.addImport(localRepoName, exportedName); } @@ -635,13 +637,13 @@ public void useRepo( throws EvalException { ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "use_repo()"); context.setNonModuleCalled(); - Location location = thread.getCallerLocation(); + ImmutableList stack = thread.getCallStack(); for (String arg : Sequence.cast(args, String.class, "args")) { - extensionProxy.addImport(arg, arg, "by a use_repo() call", location); + extensionProxy.addImport(arg, arg, "by a use_repo() call", stack); } for (Map.Entry entry : Dict.cast(kwargs, String.class, String.class, "kwargs").entrySet()) { - extensionProxy.addImport(entry.getKey(), entry.getValue(), "by a use_repo() call", location); + extensionProxy.addImport(entry.getKey(), entry.getValue(), "by a use_repo() call", stack); } } @@ -829,7 +831,7 @@ public void call( .setContainingModuleFilePath( usageBuilder.getContext().getCurrentModuleFilePath())); extensionProxy.getValue(tagName).call(kwargs, thread); - extensionProxy.addImport(name, name, "by a repo rule", thread.getCallerLocation()); + extensionProxy.addImport(name, name, "by a repo rule", thread.getCallStack()); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java index 2bd90a32d20bdb..904b94de910b29 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java @@ -91,14 +91,21 @@ Location location() { } } - record RepoNameUsage(String how, Location where) {} + record RepoNameUsage(String how, ImmutableList stack) { + Location location() { + // Skip over the override_repo builtin frame. + return stack.reverse().get(1).location; + } + } - public void addRepoNameUsage(String repoName, String how, Location where) throws EvalException { - RepoNameUsage collision = repoNameUsages.put(repoName, new RepoNameUsage(how, where)); + public void addRepoNameUsage( + String repoName, String how, ImmutableList stack) + throws EvalException { + RepoNameUsage collision = repoNameUsages.put(repoName, new RepoNameUsage(how, stack)); if (collision != null) { throw Starlark.errorf( "The repo name '%s' is already being used %s at %s", - repoName, collision.how(), collision.where()); + repoName, collision.how(), collision.location()); } } @@ -176,16 +183,20 @@ boolean isForExtension(String extensionBzlFile, String extensionName) { && !this.isolate; } - void addImport(String localRepoName, String exportedName, String byWhat, Location location) + void addImport( + String localRepoName, + String exportedName, + String byWhat, + ImmutableList stack) throws EvalException { RepositoryName.validateUserProvidedRepoName(localRepoName); RepositoryName.validateUserProvidedRepoName(exportedName); - context.addRepoNameUsage(localRepoName, byWhat, location); + context.addRepoNameUsage(localRepoName, byWhat, stack); if (imports.containsValue(exportedName)) { String collisionRepoName = imports.inverse().get(exportedName); throw Starlark.errorf( "The repo exported as '%s' by module extension '%s' is already imported at %s", - exportedName, extensionName, context.repoNameUsages.get(collisionRepoName).where()); + exportedName, extensionName, context.repoNameUsages.get(collisionRepoName).location()); } imports.put(localRepoName, exportedName); } @@ -250,6 +261,15 @@ ModuleExtensionUsage buildUsage() throws EvalException { } String importedAs = imports.inverse().get(overriddenRepoName); if (importedAs != null) { + if (!override.getValue().mustExist) { + throw Starlark.errorf( + "Cannot import repo '%s' that has been injected into module extension '%s' at %s. Please refer to @%s directly.", + overriddenRepoName, + extensionName, + override.getValue().location(), + overridingRepoName) + .withCallStack(context.repoNameUsages.get(importedAs).stack); + } context.overriddenRepos.put(importedAs, override.getValue()); } context.overridingRepos.put(overridingRepoName, override.getValue()); @@ -308,7 +328,7 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti } deps.put(builtinModule, DepSpec.create(builtinModule, Version.EMPTY, -1)); try { - addRepoNameUsage(builtinModule, "as a built-in dependency", Location.BUILTIN); + addRepoNameUsage(builtinModule, "as a built-in dependency", ImmutableList.of()); } catch (EvalException e) { throw new EvalException( e.getMessage() 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 55e12bcc1778c7..735f38f4cdcb45 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 @@ -2048,6 +2048,7 @@ public void extensionMetadata() throws Exception { " 'dev_as_non_dev_dep',", " my_direct_dep = 'direct_dep',", ")", + "inject_repo(ext, my_data_repo = 'data_repo')", "ext_dev = use_extension('@ext//:defs.bzl', 'ext', dev_dependency = True)", "use_repo(", " ext_dev,", @@ -3250,7 +3251,7 @@ public void overrideRepo_inject() throws Exception { """ bazel_dep(name = "data_repo", version = "1.0") ext = use_extension("//:defs.bzl","ext") - use_repo(ext, "bar", module_foo = "foo") + use_repo(ext, "bar") data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") data_repo(name = "foo", data = "overridden_data") inject_repo(ext, "foo") @@ -3310,73 +3311,4 @@ def _ext_impl(ctx): assertThat(fooData).isInstanceOf(String.class); assertThat(fooData).isEqualTo("overridden_data"); } - - @Test - public void overrideRepo_inject_onExistingRepoFails() throws Exception { - scratch.file( - workspaceRoot.getRelative("MODULE.bazel").getPathString(), - """ - bazel_dep(name = "data_repo", version = "1.0") - ext = use_extension("//:defs.bzl","ext") - use_repo(ext, "bar", module_foo = "foo") - data_repo = use_repo_rule("@data_repo//:defs.bzl", "data_repo") - data_repo(name = "override", data = "overridden_data") - inject_repo(ext, foo = "override") - """); - scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); - scratch.file( - workspaceRoot.getRelative("data.bzl").getPathString(), - """ - load("@bar//:list.bzl", _bar_list = "list") - load("@override//:data.bzl", _override_data = "data") - load("@module_foo//:data.bzl", _foo_data = "data") - bar_list = _bar_list - foo_data = _foo_data - override_data = _override_data - """); - scratch.file( - workspaceRoot.getRelative("defs.bzl").getPathString(), - """ - load("@data_repo//:defs.bzl", "data_repo") - def _list_repo_impl(ctx): - ctx.file("WORKSPACE") - ctx.file("BUILD") - labels = [str(Label(l)) for l in ctx.attr.labels] - labels += [str(Label("@module_foo//:target3"))] - ctx.file("list.bzl", "list = " + repr(labels) + " + [str(Label('@foo//:target4'))]") - list_repo = repository_rule( - implementation = _list_repo_impl, - attrs = { - "labels": attr.label_list(), - }, - ) - def _fail_repo_impl(ctx): - fail("This rule should not be evaluated") - fail_repo = repository_rule(implementation = _fail_repo_impl) - def _ext_impl(ctx): - fail_repo(name = "foo") - list_repo( - name = "bar", - labels = [ - # lazy extension implementation function repository mapping - "@foo//:target1", - # module repo repository mapping - "@module_foo//:target2", - ], - ) - 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 \"//:defs.bzl\" generates repository \"foo\", yet it is" - + " injected via inject_repo() at /ws/MODULE.bazel:6:12. Use override_repo()" - + " instead to override an existing repository."); - } } 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 fed6ec031e54c0..575a8febad4632 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 @@ -2012,4 +2012,33 @@ public void testOverrideRepo_cycle_multipleExtensions() throws Exception { extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, \ which is not supported."""); } + + @Test + public void testInjectRepo_imported() throws Exception { + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + """ + module(name='aaa') + bazel_dep(name = 'my_repo', version = "1.0") + ext = use_extension('//:defs.bzl', 'ext') + inject_repo(ext, foo = 'my_repo') + use_repo(ext, bar = 'foo') + """); + + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + """ + ERROR /workspace/MODULE.bazel:5:9: Traceback (most recent call last): + \tFile "/workspace/MODULE.bazel", line 5, column 9, in + \t\tuse_repo(ext, bar = 'foo') + Error in use_repo: Cannot import repo 'foo' that has been injected into \ + module extension 'ext' at /workspace/MODULE.bazel:4:12. Please refer \ + to @my_repo directly.\ + """); + } }