Do not record or check repo mapping entries for repos defined in WORKSPACE

Wyverald · copybara-github · commit 1cbf09dd45a4 · 2024-02-19T01:32:57.000-08:00
Mappings for repos defined in WORKSPACE are extremely annoying to verify given the chunked loading (we'd have to record which chunk the repo mapping was used in, and then load that chunk while verifying). This is extremely not worth the effort (especially since nobody really uses repo mappings in WORKSPACE) so we just don't record it. This also means, during verification, we can safely use the main repo mapping without WORKSPACE (since repos defined in Bzlmod can't see stuff from WORKSPACE anyway). Fixes #21289. Closes #21393. PiperOrigin-RevId: 608258493 Change-Id: Ife6a01221e6f5e1685d859eaa5acc8b370ab8483
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
@@ -319,12 +319,17 @@ private RepositoryDirectoryValue.Builder fetchInternal(
             new RepoRecordedInput.EnvVar(envKey), clientEnvironment.get(envKey));
       }
 
-      for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
-          repoMappingRecorder.recordedEntries().cellSet()) {
-        recordedInputValues.put(
-            new RepoRecordedInput.RecordedRepoMapping(
-                repoMappings.getRowKey(), repoMappings.getColumnKey()),
-            repoMappings.getValue().getName());
+      // 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());
+        }
       }
 
       env.getListener().post(resolved);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java
@@ -559,7 +559,12 @@ public String toStringInternal() {
 
     @Override
     public SkyKey getSkyKey(BlazeDirectories directories) {
-      return RepositoryMappingValue.key(sourceRepo);
+      // Since we only record repo mapping entries for repos defined in Bzlmod, we can request the
+      // WORKSPACE-less version of the main repo mapping (as no repos defined in Bzlmod can see
+      // stuff from WORKSPACE).
+      return sourceRepo.isMain()
+          ? RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS
+          : RepositoryMappingValue.key(sourceRepo);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -15,7 +15,6 @@
 
 package com.google.devtools.build.lib.rules.repository;
 
-import static com.google.devtools.build.lib.cmdline.LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -394,12 +393,9 @@ private boolean shouldUseVendorRepos(Environment env, RepositoryFunction handler
   }
 
   private boolean shouldExcludeRepoFromVendoring(RepositoryFunction handler, Rule rule) {
-    return handler.isLocal(rule) || handler.isConfigure(rule) || isWorkspaceRepo(rule);
-  }
-
-  private boolean isWorkspaceRepo(Rule rule) {
-    // All workspace repos are under //external, while bzlmod repo rules are not
-    return rule.getPackage().getPackageIdentifier().equals(EXTERNAL_PACKAGE_IDENTIFIER);
+    return handler.isLocal(rule)
+        || handler.isConfigure(rule)
+        || RepositoryFunction.isWorkspaceRepo(rule);
   }
 
   @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.repository;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.devtools.build.lib.cmdline.LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -137,6 +138,11 @@ public static void setupRepoRoot(Path repoRoot) throws RepositoryFunctionExcepti
     }
   }
 
+  public static boolean isWorkspaceRepo(Rule rule) {
+    // All workspace repos are under //external, while bzlmod repo rules are not
+    return rule.getPackage().getPackageIdentifier().equals(EXTERNAL_PACKAGE_IDENTIFIER);
+  }
+
   protected void setupRepoRootBeforeFetching(Path repoRoot) throws RepositoryFunctionException {
     setupRepoRoot(repoRoot);
   }
diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
@@ -2660,10 +2660,12 @@ bazel_dep(name="bar")
 local_path_override(module_name="bar", path="bar")
 EOF
   touch BUILD
+  echo 'load("@r//:r.bzl", "pi"); print(pi)' > WORKSPACE.bzlmod
   cat > r.bzl <<EOF
 def _r(rctx):
   print("I see: " + str(Label("@data")))
   rctx.file("BUILD", "filegroup(name='r')")
+  rctx.file("r.bzl", "pi=3.14")
 r=repository_rule(_r)
 EOF
   mkdir foo
@@ -2704,11 +2706,13 @@ bazel_dep(name="bar")
 local_path_override(module_name="bar", path="bar")
 EOF
   touch BUILD
+  echo 'load("@r//:r.bzl", "pi"); print(pi)' > WORKSPACE.bzlmod
   cat > r.bzl <<EOF
 CONSTANT = Label("@data")
 def _r(rctx):
   print("I see: " + str(CONSTANT))
   rctx.file("BUILD", "filegroup(name='r')")
+  rctx.file("r.bzl", "pi=3.14")
 r=repository_rule(_r)
 EOF
   mkdir foo

Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,7 @@`
`15`	`15`	`package com.google.devtools.build.lib.rules.repository;`
`16`	`16`
`17`	`17`	`import static com.google.common.base.Preconditions.checkState;`
	`18`	`+import static com.google.devtools.build.lib.cmdline.LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER;`
`18`	`19`
`19`	`20`	`import com.google.common.annotations.VisibleForTesting;`
`20`	`21`	`import com.google.common.base.Preconditions;`
`@@ -137,6 +138,11 @@ public static void setupRepoRoot(Path repoRoot) throws RepositoryFunctionExcepti`
`137`	`138`	`}`
`138`	`139`	`}`
`139`	`140`
	`141`	`+ public static boolean isWorkspaceRepo(Rule rule) {`
	`142`	`+ // All workspace repos are under //external, while bzlmod repo rules are not`
	`143`	`+ return rule.getPackage().getPackageIdentifier().equals(EXTERNAL_PACKAGE_IDENTIFIER);`
	`144`	`+ }`
	`145`	`+`
`140`	`146`	`protected void setupRepoRootBeforeFetching(Path repoRoot) throws RepositoryFunctionException {`
`141`	`147`	`setupRepoRoot(repoRoot);`
`142`	`148`	`}`