Allow @repo_name labels in override attributes

This is made possible by a refactoring that moves the label parsing of `RepoSpec` attributes coming from module overrides as well as `.bzl` file labels containing repo rules from `BzlmodRepoRuleFunction` into `ModuleFileGlobals`. Also adds a TODO to `BzlmodRepoRuleFunction` to further simplify the logic after the next lockfile version bumps, as old lockfiles are now the only source of non-canonical labels in `RepoSpec`s.

Work towards #19301

Closes #21188.

PiperOrigin-RevId: 605517195
Change-Id: Id34c81fa9474fef2354ff1fbc898e518a9044640

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

@@ -25,7 +25,7 @@ public abstract class ArchiveOverride implements NonRegistryOverride {
 
   public static ArchiveOverride create(
       ImmutableList<String> urls,
-      ImmutableList<String> patches,
+      ImmutableList<Object> patches,
       ImmutableList<String> patchCmds,
       String integrity,
       String stripPrefix,
@@ -37,8 +37,8 @@ public static ArchiveOverride create(
   /** The URLs pointing at the archives. Can be HTTP(S) or file URLs. */
   public abstract ImmutableList<String> getUrls();
 
-  /** The patches to apply after extracting the archive. Should be a list of labels. */
-  public abstract ImmutableList<String> getPatches();
+  /** The labels of patches to apply after extracting the archive. */
+  public abstract ImmutableList<Object> getPatches();
 
   /** The patch commands to execute after extracting the archive. Should be a list of commands. */
   public abstract ImmutableList<String> getPatchCmds();

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

@@ -27,7 +27,7 @@
  */
 public class ArchiveRepoSpecBuilder {
 
-  public static final String HTTP_ARCHIVE_PATH = "@bazel_tools//tools/build_defs/repo:http.bzl";
+  public static final String HTTP_ARCHIVE_PATH = "@@bazel_tools//tools/build_defs/repo:http.bzl";
 
   private final ImmutableMap.Builder<String, Object> attrBuilder;
 
@@ -54,7 +54,7 @@ public ArchiveRepoSpecBuilder setStripPrefix(String stripPrefix) {
   }
 
   @CanIgnoreReturnValue
-  public ArchiveRepoSpecBuilder setPatches(ImmutableList<String> patches) {
+  public ArchiveRepoSpecBuilder setPatches(ImmutableList<Object> patches) {
     attrBuilder.put("patches", patches);
     return this;
   }

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

@@ -25,7 +25,7 @@ public abstract class GitOverride implements NonRegistryOverride {
   public static GitOverride create(
       String remote,
       String commit,
-      ImmutableList<String> patches,
+      ImmutableList<Object> patches,
       ImmutableList<String> patchCmds,
       int patchStrip,
       boolean initSubmodules) {
@@ -39,8 +39,8 @@ public static GitOverride create(
   /** The commit hash to use. */
   public abstract String getCommit();
 
-  /** The patches to apply after fetching from Git. Should be a list of labels. */
-  public abstract ImmutableList<String> getPatches();
+  /** The labels of patches to apply after fetching from Git. */
+  public abstract ImmutableList<Object> getPatches();
 
   /** The patch commands to execute after fetching from Git. Should be a list of commands. */
   public abstract ImmutableList<String> getPatchCmds();

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

@@ -25,7 +25,7 @@
  */
 public class GitRepoSpecBuilder {
 
-  public static final String GIT_REPO_PATH = "@bazel_tools//tools/build_defs/repo:git.bzl";
+  public static final String GIT_REPO_PATH = "@@bazel_tools//tools/build_defs/repo:git.bzl";
 
   private final ImmutableMap.Builder<String, Object> attrBuilder;
 
@@ -71,7 +71,7 @@ public GitRepoSpecBuilder setStripPrefix(String stripPrefix) {
   }
 
   @CanIgnoreReturnValue
-  public GitRepoSpecBuilder setPatches(List<String> patches) {
+  public GitRepoSpecBuilder setPatches(List<Object> patches) {
     return setAttr("patches", patches);
   }
 
@@ -113,7 +113,7 @@ private GitRepoSpecBuilder setAttr(String name, boolean value) {
   }
 
   @CanIgnoreReturnValue
-  private GitRepoSpecBuilder setAttr(String name, List<String> value) {
+  private GitRepoSpecBuilder setAttr(String name, List<?> value) {
     if (value != null && !value.isEmpty()) {
       attrBuilder.put(name, value);
     }

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

@@ -15,6 +15,8 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.HashBiMap;
@@ -513,6 +515,27 @@ private String normalizeLabelString(String rawExtensionBzlFile) {
     }
   }
 
+  /**
+   * Returns a {@link Label} when the given string is a valid label, otherwise the string itself.
+   */
+  private Object parseOverrideLabelAttribute(String rawLabel) {
+    RepositoryMapping repoMapping =
+        RepositoryMapping.create(
+            ImmutableMap.<String, RepositoryName>builder()
+                .put("", RepositoryName.MAIN)
+                .put(module.getRepoName().orElse(module.getName()), RepositoryName.MAIN)
+                .buildKeepingLast(),
+            RepositoryName.MAIN);
+    try {
+      return Label.parseWithPackageContext(
+          rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
+    } catch (LabelSyntaxException e) {
+      // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
+      // let the module repo fail when fetched.
+      return rawLabel;
+    }
+  }
+
   class ModuleExtensionUsageBuilder {
     private final String extensionBzlFile;
     private final String extensionName;
@@ -848,7 +871,9 @@ public void singleVersionOverride(
         SingleVersionOverride.create(
             parsedVersion,
             registry,
-            Sequence.cast(patches, String.class, "patches").getImmutableList(),
+            Sequence.cast(patches, String.class, "patches").stream()
+                .map(this::parseOverrideLabelAttribute)
+                .collect(toImmutableList()),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             patchStrip.toInt("single_version_override.patch_strip")));
   }
@@ -984,7 +1009,9 @@ public void archiveOverride(
         moduleName,
         ArchiveOverride.create(
             urlList,
-            Sequence.cast(patches, String.class, "patches").getImmutableList(),
+            Sequence.cast(patches, String.class, "patches").stream()
+                .map(this::parseOverrideLabelAttribute)
+                .collect(toImmutableList()),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             integrity,
             stripPrefix,
@@ -1060,7 +1087,9 @@ public void gitOverride(
         GitOverride.create(
             remote,
             commit,
-            Sequence.cast(patches, String.class, "patches").getImmutableList(),
+            Sequence.cast(patches, String.class, "patches").stream()
+                .map(this::parseOverrideLabelAttribute)
+                .collect(toImmutableList()),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             patchStrip.toInt("git_override.patch_strip"),
             initSubmodules));

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

@@ -29,7 +29,8 @@
 public abstract class RepoSpec implements SkyValue {
 
   /**
-   * The label string for the bzl file this repository rule is defined in, empty for native rule.
+   * The unambiguous canonical label string for the bzl file this repository rule is defined in,
+   * empty for native rule.
    */
   @Nullable
   public abstract String bzlFile();
@@ -52,6 +53,8 @@ public static Builder builder() {
   public abstract static class Builder {
     public abstract Builder setBzlFile(String bzlFile);
 
+    abstract String bzlFile();
+
     public abstract Builder setRuleClassName(String name);
 
     public abstract Builder setAttributes(AttributeValues attributes);

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

@@ -33,7 +33,7 @@ public abstract class SingleVersionOverride implements RegistryOverride {
   public static SingleVersionOverride create(
       Version version,
       String registry,
-      ImmutableList<String> patches,
+      ImmutableList<Object> patches,
       ImmutableList<String> patchCmds,
       int patchStrip) {
     return new AutoValue_SingleVersionOverride(version, registry, patches, patchCmds, patchStrip);
@@ -48,8 +48,8 @@ public static SingleVersionOverride create(
   @Override
   public abstract String getRegistry();
 
-  /** The patches to apply after retrieving per the registry. Should be a list of labels. */
-  public abstract ImmutableList<String> getPatches();
+  /** The labels of patches to apply after retrieving per the registry. */
+  public abstract ImmutableList<Object> getPatches();
 
   /**
    * The patch commands to execute after retrieving per the registry. Should be a list of commands.

src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java

@@ -70,14 +70,11 @@ public final class BzlmodRepoRuleFunction implements SkyFunction {
   private final BlazeDirectories directories;
 
   /**
-   * An empty repo mapping anchored to the main repo.
-   *
-   * <p>None of the labels present in RepoSpecs can point to any repo other than the main repo
-   * or @bazel_tools, because at this point we don't know how any other repo is defined yet. The
-   * RepoSpecs processed by this class can only contain labels from the MODULE.bazel file (from
-   * overrides). In the future, they might contain labels from the lockfile, but those will need to
-   * be canonical label literals, which bypass repo mapping anyway.
+   * An empty repo mapping anchored to the main repo. Label strings in {@link RepoSpec}s are always
+   * in unambiguous canonical form and thus require no mapping, except instances read from old
+   * lockfiles.
    */
+  // TODO(fmeum): Make this mapping truly empty after bumping LOCK_FILE_VERSION.
   private static final RepositoryMapping EMPTY_MAIN_REPO_MAPPING =
       RepositoryMapping.create(
           ImmutableMap.of("", RepositoryName.MAIN, "bazel_tools", RepositoryName.BAZEL_TOOLS),

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

@@ -142,23 +142,85 @@ def testRegistryOverride(self):
   def testArchiveOverride(self):
     self.writeMainProjectFiles()
     archive_aaa_1_0 = self.main_registry.archives.joinpath('aaa.1.0.zip')
+    self.ScratchFile(
+        'aaa2.patch',
+        [
+            '--- a/aaa.cc',
+            '+++ b/aaa.cc',
+            '@@ -1,6 +1,6 @@',
+            ' #include <stdio.h>',
+            ' #include "aaa.h"',
+            ' void hello_aaa(const std::string& caller) {',
+            '-    std::string lib_name = "aaa@1.0 (locally patched)";',
+            '+    std::string lib_name = "aaa@1.0 (locally patched again)";',
+            '     printf("%s => %s\\n", caller.c_str(), lib_name.c_str());',
+            ' }',
+        ],
+    )
+    self.ScratchFile(
+        'aaa3.patch',
+        [
+            '--- a/aaa.cc',
+            '+++ b/aaa.cc',
+            '@@ -1,6 +1,6 @@',
+            ' #include <stdio.h>',
+            ' #include "aaa.h"',
+            ' void hello_aaa(const std::string& caller) {',
+            '-    std::string lib_name = "aaa@1.0 (locally patched again)";',
+            (
+                '+    std::string lib_name = "aaa@1.0 (locally patched again'
+                ' and again)";'
+            ),
+            '     printf("%s => %s\\n", caller.c_str(), lib_name.c_str());',
+            ' }',
+        ],
+    )
+    self.ScratchFile(
+        'aaa4.patch',
+        [
+            '--- a/aaa.cc',
+            '+++ b/aaa.cc',
+            '@@ -1,6 +1,6 @@',
+            ' #include <stdio.h>',
+            ' #include "aaa.h"',
+            ' void hello_aaa(const std::string& caller) {',
+            (
+                '-    std::string lib_name = "aaa@1.0 (locally patched again'
+                ' and again)";'
+            ),
+            (
+                '+    std::string lib_name = "aaa@1.0 (locally patched all over'
+                ' again)";'
+            ),
+            '     printf("%s => %s\\n", caller.c_str(), lib_name.c_str());',
+            ' }',
+        ],
+    )
     self.ScratchFile(
         'MODULE.bazel',
         [
+            'module(name = "main", repo_name = "my_main")',
             'bazel_dep(name = "aaa", version = "1.1")',
             'bazel_dep(name = "bbb", version = "1.1")',
             'archive_override(',
             '  module_name = "aaa",',
             '  urls = ["%s"],' % archive_aaa_1_0.as_uri(),
-            '  patches = ["//:aaa.patch"],',
+            '  patches = [',
+            '    "//:aaa.patch",',
+            '    "@//:aaa2.patch",',
+            '    "@my_main//:aaa3.patch",',
+            '    ":aaa4.patch",',
+            '  ],',
             '  patch_strip = 1,',
             ')',
         ],
     )
     _, stdout, _ = self.RunBazel(['run', '//:main'])
-    self.assertIn('main function => aaa@1.0 (locally patched)', stdout)
+    self.assertIn(
+        'main function => aaa@1.0 (locally patched all over again)', stdout
+    )
     self.assertIn('main function => bbb@1.1', stdout)
-    self.assertIn('bbb@1.1 => aaa@1.0 (locally patched)', stdout)
+    self.assertIn('bbb@1.1 => aaa@1.0 (locally patched all over again)', stdout)
 
   def testGitOverride(self):
     self.writeMainProjectFiles()