Skip to content

Commit 21d7fec

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Fix crash when mixing use_repo_rule and --inject_repository
Bazel crashes at HEAD when `use_repo_rule` is used with `local_repository` while also using `--inject_repository`. Make bugs like this less likely by extracting out a safe "get or create" helper for extension usages. Fixes #27953 Closes #27967. PiperOrigin-RevId: 844683022 Change-Id: I1edcf1e7c72ef8d46c67e51b4f9ffd6a6ce82ec8
1 parent c795f5d commit 21d7fec

File tree

4 files changed

+49
-33
lines changed

4 files changed

+49
-33
lines changed

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -658,8 +658,7 @@ private static void injectRepos(
658658
}
659659
// Use the innate extension backing use_repo_rule.
660660
ModuleExtensionUsageBuilder usageBuilder =
661-
new ModuleExtensionUsageBuilder(
662-
context,
661+
context.getOrCreateExtensionUsageBuilder(
663662
"//:MODULE.bazel",
664663
"@bazel_tools//tools/build_defs/repo:local.bzl local_repository",
665664
/* isolate= */ false);
@@ -686,7 +685,6 @@ private static void injectRepos(
686685
"by --inject_repository",
687686
thread.getCallStack());
688687
}
689-
context.getExtensionUsageBuilders().add(usageBuilder);
690688
}
691689

692690
/**

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java‎

Lines changed: 9 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -468,27 +468,17 @@ public ModuleExtensionProxy useExtension(
468468
.setContainingModuleFilePath(context.getCurrentModuleFilePath());
469469

470470
String extensionBzlFile = normalizeLabelString(context.getModuleBuilder(), rawExtensionBzlFile);
471-
var newUsageBuilder =
472-
new ModuleExtensionUsageBuilder(context, extensionBzlFile, extensionName, isolate);
473471

474472
if (context.shouldIgnoreDevDeps() && devDependency) {
475473
// This is a no-op proxy.
476-
return new ModuleExtensionProxy(newUsageBuilder, proxyBuilder);
474+
return new ModuleExtensionProxy(
475+
new ModuleExtensionUsageBuilder(context, extensionBzlFile, extensionName, isolate),
476+
proxyBuilder);
477477
}
478478

479-
// Find an existing usage builder corresponding to this extension. Isolated usages need to get
480-
// their own proxy.
481-
if (!isolate) {
482-
for (ModuleExtensionUsageBuilder usageBuilder : context.getExtensionUsageBuilders()) {
483-
if (usageBuilder.isForExtension(extensionBzlFile, extensionName)) {
484-
return new ModuleExtensionProxy(usageBuilder, proxyBuilder);
485-
}
486-
}
487-
}
488-
489-
// If no such proxy exists, we can just use a new one.
490-
context.getExtensionUsageBuilders().add(newUsageBuilder);
491-
return new ModuleExtensionProxy(newUsageBuilder, proxyBuilder);
479+
return new ModuleExtensionProxy(
480+
context.getOrCreateExtensionUsageBuilder(extensionBzlFile, extensionName, isolate),
481+
proxyBuilder);
492482
}
493483

494484
private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile)
@@ -826,16 +816,9 @@ public RepoRuleProxy useRepoRule(String bzlFile, String ruleName, StarlarkThread
826816
String extensionName = bzlFile + ' ' + ruleName;
827817
// Find or create the builder for the singular "innate" extension of this repo rule for this
828818
// module.
829-
for (ModuleExtensionUsageBuilder usageBuilder : context.getExtensionUsageBuilders()) {
830-
if (usageBuilder.isForExtension("//:MODULE.bazel", extensionName)) {
831-
return new RepoRuleProxy(usageBuilder);
832-
}
833-
}
834-
ModuleExtensionUsageBuilder newUsageBuilder =
835-
new ModuleExtensionUsageBuilder(
836-
context, "//:MODULE.bazel", extensionName, /* isolate= */ false);
837-
context.getExtensionUsageBuilders().add(newUsageBuilder);
838-
return new RepoRuleProxy(newUsageBuilder);
819+
return new RepoRuleProxy(
820+
context.getOrCreateExtensionUsageBuilder(
821+
"//:MODULE.bazel", extensionName, /* isolate= */ false));
839822
}
840823

841824
@StarlarkBuiltin(name = "repo_rule_proxy", documented = false)

‎src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,21 @@ public void addDep(Optional<String> repoName, DepSpec depSpec) {
151151
}
152152
}
153153

154-
List<ModuleExtensionUsageBuilder> getExtensionUsageBuilders() {
155-
return extensionUsageBuilders;
154+
ModuleExtensionUsageBuilder getOrCreateExtensionUsageBuilder(
155+
String extensionBzlFile, String extensionName, boolean isolate) {
156+
// Isolated extensions have to always get a new builder, non-isolated ones have to reuse an
157+
// existing one if it exists so that they all contribute usages to the same row in a table.
158+
if (!isolate) {
159+
for (var builder : extensionUsageBuilders) {
160+
if (builder.isForExtension(extensionBzlFile, extensionName)) {
161+
return builder;
162+
}
163+
}
164+
}
165+
var newBuilder =
166+
new ModuleExtensionUsageBuilder(this, extensionBzlFile, extensionName, isolate);
167+
extensionUsageBuilders.add(newBuilder);
168+
return newBuilder;
156169
}
157170

158171
static class ModuleExtensionUsageBuilder {
@@ -189,7 +202,7 @@ void addProxyBuilder(ModuleExtensionUsage.Proxy.Builder builder) {
189202
proxyBuilders.add(builder);
190203
}
191204

192-
boolean isForExtension(String extensionBzlFile, String extensionName) {
205+
private boolean isForExtension(String extensionBzlFile, String extensionName) {
193206
return this.extensionBzlFile.equals(extensionBzlFile)
194207
&& this.extensionName.equals(extensionName)
195208
&& !this.isolate;

‎src/test/py/bazel/bzlmod/bazel_overrides_test.py‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -800,7 +800,7 @@ def testInjectRepositoryRepoNameSideEffects(self):
800800
)
801801

802802
# --inject_repository _must not_ affect `use_repo_rule` generated repo names
803-
#.
803+
# .
804804
_, stdout, _ = self.RunBazel([
805805
'mod',
806806
'dump_repo_mapping',
@@ -816,6 +816,28 @@ def testInjectRepositoryRepoNameSideEffects(self):
816816
'\n'.join(stdout),
817817
)
818818

819+
def testInjectRepositoryAndLocalRepository(self):
820+
# Regression test for https://github.com/bazelbuild/bazel/issues/27953
821+
self.ScratchFile(
822+
'MODULE.bazel',
823+
[
824+
(
825+
'local_repository ='
826+
' use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl",'
827+
' "local_repository")'
828+
),
829+
'local_repository(name = "local_repo", path = "local_repo")',
830+
],
831+
)
832+
self.ScratchFile('local_repo/REPO.bazel')
833+
self.ScratchFile('injected_repo/REPO.bazel')
834+
835+
self.RunBazel([
836+
'mod',
837+
'deps',
838+
'--inject_repository=injected_repo=%workspace%/injected_repo',
839+
])
840+
819841
def testOverrideRepositoryOnNonExistentRepo(self):
820842
self.ScratchFile('other_repo/REPO.bazel')
821843
self.ScratchFile('other_repo/BUILD', ['filegroup(name="target")'])

0 commit comments

Comments
 (0)