[7.1.0] Do not record any repo mapping entries in the RepoMappingReco…

…rder for WORKSPACE repo rules (#21457) Follow-up to 1cbf09d; beyond not recording those entries in the marker file, we shouldn't even record them in memory in the first place. This avoids nasty problems with unexported repo rules (gah) and should be an extremely tiny performance win as a bonus... Fixes #21451 Commit e84d053 PiperOrigin-RevId: 609010175 Change-Id: I90eb921b09068f327b42886ea0a1875374a94049 Co-authored-by: Googler <[email protected]>
bazelbuild · Feb 21, 2024 · 74e928b · 74e928b
1 parent a109d5c
commit 74e928b
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 14 deletions.
diff --git a/...a/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/...a/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
@@ -231,9 +231,14 @@ private RepositoryDirectoryValue.Builder fetchInternal(
       StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics);
       thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener()));
       var repoMappingRecorder = new Label.RepoMappingRecorder();
-      repoMappingRecorder.mergeEntries(
-          rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries());
-      thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder);
+      // For repos defined in Bzlmod, record any used repo mappings in the marker file.
+      // Repos defined in WORKSPACE are impossible to verify given the chunked loading (we'd have to
+      // record which chunk the repo mapping was used in, and ain't nobody got time for that).
+      if (!isWorkspaceRepo(rule)) {
+        repoMappingRecorder.mergeEntries(
+            rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries());
+        thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder);
+      }
 
       new BazelStarlarkContext(
               BazelStarlarkContext.Phase.LOADING, // ("fetch")
@@ -321,17 +326,12 @@ private RepositoryDirectoryValue.Builder fetchInternal(
             new RepoRecordedInput.EnvVar(envKey), clientEnvironment.get(envKey));
       }
 
-      // For repos defined in Bzlmod, record any used repo mappings in the marker file.
-      // Repos defined in WORKSPACE are impossible to verify given the chunked loading (we'd have to
-      // record which chunk the repo mapping was used in, and ain't nobody got time for that).
-      if (!isWorkspaceRepo(rule)) {
-        for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
-            repoMappingRecorder.recordedEntries().cellSet()) {
-          recordedInputValues.put(
-              new RepoRecordedInput.RecordedRepoMapping(
-                  repoMappings.getRowKey(), repoMappings.getColumnKey()),
-              repoMappings.getValue().getName());
-        }
+      for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
+          repoMappingRecorder.recordedEntries().cellSet()) {
+        recordedInputValues.put(
+            new RepoRecordedInput.RecordedRepoMapping(
+                repoMappings.getRowKey(), repoMappings.getColumnKey()),
+            repoMappings.getValue().getName());
       }
 
       env.getListener().post(resolved);

diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
@@ -3137,4 +3137,23 @@ EOF
   expect_log "I'm running!"
 }
 
+function test_unexported_rule() {
+  # alas, we still need to support this while WORKSPACE is around...
+  create_new_workspace
+  touch MODULE.bazel
+  touch BUILD
+  cat > r.bzl <<EOF
+def _impl(rctx):
+  rctx.file('BUILD', 'filegroup(name="r")')
+def r():
+  repository_rule(_impl)(name = "r")
+EOF
+  cat > WORKSPACE.bzlmod <<EOF
+load('//:r.bzl', 'r')
+r()
+EOF
+
+  bazel build @r >& $TEST_log || fail "expected bazel to succeed"
+}
+
 run_suite "local repository tests"