Skip to content

Commit

Permalink
Disallow importing injected repos
Browse files Browse the repository at this point in the history
This ensures that there can't be two different apparent names for the same non-main repo (ignoring `override_repo`).

Also verify that `bazel mod tidy` doesn't suggest importing injected repos.

Closes #23798.

PiperOrigin-RevId: 682297992
Change-Id: I4dc6ac63e47f0ff949ac327d3e16dbe387731ba0
  • Loading branch information
fmeum authored and copybara-github committed Oct 4, 2024
1 parent e5ab94b commit 8472c9d
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 89 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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<StarlarkThread.CallStackEntry> stack)
throws EvalException {
usageBuilder.addImport(localRepoName, exportedName, byWhat, location);
usageBuilder.addImport(localRepoName, exportedName, byWhat, stack);
proxyBuilder.addImport(localRepoName, exportedName);
}

Expand Down Expand Up @@ -635,13 +637,13 @@ public void useRepo(
throws EvalException {
ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "use_repo()");
context.setNonModuleCalled();
Location location = thread.getCallerLocation();
ImmutableList<StarlarkThread.CallStackEntry> 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<String, String> 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);
}
}

Expand Down Expand Up @@ -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());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,21 @@ Location location() {
}
}

record RepoNameUsage(String how, Location where) {}
record RepoNameUsage(String how, ImmutableList<StarlarkThread.CallStackEntry> 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<StarlarkThread.CallStackEntry> 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());
}
}

Expand Down Expand Up @@ -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<StarlarkThread.CallStackEntry> 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);
}
Expand Down Expand Up @@ -250,6 +261,16 @@ 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());
Expand Down Expand Up @@ -308,7 +329,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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,",
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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<BzlLoadValue> 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.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<RootModuleFileValue> 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 <toplevel>
\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.\
""");
}
}

0 comments on commit 8472c9d

Please sign in to comment.