From 5f4fad5cad19af5222e2a6f20b43bce507529c6d Mon Sep 17 00:00:00 2001
From: Googler <wyv@google.com>
Date: Wed, 21 Feb 2024 08:52:02 -0800
Subject: [PATCH] Do not record any repo mapping entries in the
 RepoMappingRecorder for WORKSPACE repo rules

Follow-up to https://github.com/bazelbuild/bazel/commit/1cbf09dd45a4bb4cc73ace58ae344bb90e709752; 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 https://github.com/bazelbuild/bazel/issues/21451

PiperOrigin-RevId: 609010175
Change-Id: I90eb921b09068f327b42886ea0a1875374a94049
---
 .../starlark/StarlarkRepositoryFunction.java  | 28 +++++++++----------
 .../shell/bazel/starlark_repository_test.sh   | 19 +++++++++++++
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
index ba2bb1b083f4c7..3f98b7b02cfbcf 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
+++ b/src/main/java/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
index f0423297d4a67c..b3feef46fec2c1 100755
--- 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"