Support external repository labels in package_group

src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java

@@ -15,6 +15,8 @@
 package com.google.devtools.build.lib.analysis.starlark;
 
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.BazelModuleContext;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.BzlInitThreadContext;
 import com.google.devtools.build.lib.packages.BzlVisibility;
@@ -63,20 +65,23 @@ public void visibility(Object value, StarlarkThread thread) throws EvalException
       throw Starlark.errorf("load visibility may not be set more than once");
     }
 
+    BazelModuleContext moduleContext = BazelModuleContext.ofInnermostBzlOrThrow(thread);
+    RepositoryMapping repoMapping = moduleContext.repoMapping();
+
     RepositoryName repo = context.getBzlFile().getRepository();
     ImmutableList<PackageSpecification> specs;
     if (value instanceof String) {
       // `visibility("public")`, `visibility("private")`, visibility("//pkg")
       specs =
-          ImmutableList.of(PackageSpecification.fromStringForBzlVisibility(repo, (String) value));
+          ImmutableList.of(PackageSpecification.fromStringForBzlVisibility(repoMapping, repo, (String) value));
     } else if (value instanceof StarlarkList) {
       // `visibility(["//pkg1", "//pkg2", ...])`
       List<String> specStrings = Sequence.cast(value, String.class, "visibility list");
       ImmutableList.Builder<PackageSpecification> specsBuilder =
           ImmutableList.builderWithExpectedSize(specStrings.size());
       for (String specString : specStrings) {
         PackageSpecification spec =
-            PackageSpecification.fromStringForBzlVisibility(repo, specString);
+            PackageSpecification.fromStringForBzlVisibility(repoMapping, repo, specString);
         specsBuilder.add(spec);
       }
       specs = specsBuilder.build();

src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java

@@ -61,6 +61,7 @@ public PackageGroup(
       try {
         specification =
             PackageSpecification.fromString(
+                pkg.getMetadata().repositoryMapping(),
                 label.getRepository(),
                 packageSpecification,
                 allowPublicPrivate,

src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java

@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
@@ -122,6 +123,7 @@ public String toString() {
   // TODO(#16365): Remove allowPublicPrivate.
   // TODO(#16324): Remove legacy behavior and repoRootMeansCurrentRepo param.
   public static PackageSpecification fromString(
+      RepositoryMapping repositoryMapping,
       RepositoryName repositoryName,
       String spec,
       boolean allowPublicPrivate,
@@ -151,22 +153,35 @@ public static PackageSpecification fromString(
       }
     }
     PackageSpecification packageSpecification =
-        fromStringPositive(repositoryName, spec, repoRootMeansCurrentRepo);
+        fromStringPositive(repositoryMapping, repositoryName, spec, repoRootMeansCurrentRepo);
     return negative ? new NegativePackageSpecification(packageSpecification) : packageSpecification;
   }
 
   private static PackageSpecification fromStringPositive(
-      RepositoryName repositoryName, String spec, boolean repoRootMeansCurrentRepo)
+      RepositoryMapping repositoryMapping, RepositoryName repositoryName, String spec, boolean repoRootMeansCurrentRepo)
       throws InvalidPackageSpecificationException {
     if (spec.equals(PUBLIC_VISIBILITY)) {
       return AllPackages.INSTANCE;
     } else if (spec.equals(PRIVATE_VISIBILITY)) {
       return NoPackages.INSTANCE;
     }
-    if (!spec.startsWith("//")) {
+
+    if (!spec.startsWith("//") && !spec.startsWith("@")) {
       throw new InvalidPackageSpecificationException(
           String.format(
-              "invalid package name '%s': must start with '//' or be 'public' or 'private'", spec));
+              "invalid package name '%s': must start with '//', '@', or be 'public' or 'private'", spec));
+    }
+
+    try {
+      Label label =
+          Label.parseWithRepoContext(
+              spec, Label.RepoContext.of(repositoryName, repositoryMapping));
+      PackageSpecification mappedSpec = fromLabel(label);
+      if (mappedSpec != null) {
+        return mappedSpec;
+      }
+    } catch (LabelSyntaxException e) {
+      // Fall through to parse as a package path (e.g. //foo/...)
     }
 
     String pkgPath;
@@ -182,22 +197,24 @@ private static PackageSpecification fromStringPositive(
           // Legacy behavior: //... is "public".
           return AllPackages.INSTANCE;
         }
+      } else if (spec.endsWith("//...")) {
+        // spec was "@repo//..."
+        pkgPath += "/";
       }
     } else {
       pkgPath = spec;
     }
 
-    PackageIdentifier unqualifiedPkgId;
+    PackageIdentifier pkgId;
     try {
-      unqualifiedPkgId = PackageIdentifier.parse(pkgPath);
+      pkgId =
+          Label.parseWithRepoContext(
+                  pkgPath + ":__pkg__", Label.RepoContext.of(repositoryName, repositoryMapping))
+              .getPackageIdentifier();
     } catch (LabelSyntaxException e) {
       throw new InvalidPackageSpecificationException(
           String.format("invalid package name '%s': %s", spec, e.getMessage()));
     }
-    Verify.verify(unqualifiedPkgId.getRepository().isMain());
-
-    PackageIdentifier pkgId =
-        PackageIdentifier.create(repositoryName, unqualifiedPkgId.getPackageFragment());
     return allBeneath ? new AllPackagesBeneath(pkgId) : new SinglePackage(pkgId);
   }
 
@@ -212,11 +229,12 @@ private static PackageSpecification fromStringPositive(
    * --incompatible_fix_package_group_reporoot_syntax} are enabled.
    */
   public static PackageSpecification fromStringForBzlVisibility(
-      RepositoryName repositoryName, String spec) throws EvalException {
+      RepositoryMapping mapper, RepositoryName repositoryName, String spec) throws EvalException {
     PackageSpecification result;
     try {
       result =
           fromString(
+              mapper,
               repositoryName,
               spec,
               /*allowPublicPrivate=*/ true,

src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java

@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
 import com.google.devtools.build.lib.testutil.TestThread;
@@ -45,6 +46,7 @@ public void testNoDeadlockOnPackageGroupCreation() throws Exception {
                     Label.parseCanonicalUnchecked("//context").getRepository();
                 groupQueue.put(
                     PackageSpecification.fromString(
+                        RepositoryMapping.EMPTY,
                         defaultRepoName,
                         "//fruits/...",
                         /* allowPublicPrivate= */ true,

src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java

@@ -18,6 +18,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
@@ -95,23 +96,22 @@ public void testPackagesWithoutDoubleSlashDoNotWork() throws Exception {
   }
 
   @Test
-  public void testPackagesWithRepositoryDoNotWork() throws Exception {
+  public void testPackagesWithRepositoryWorks() throws Exception {
     scratch.file(
         "fruits/BUILD",
         """
         package_group(
             name = "banana",
-            packages = ["@veggies//:cucumber"],
+            packages = ["@@veggies//cucumber"],
         )
         """);
 
-    reporter.removeHandler(failFastHandler);
-    getPackageGroup("fruits", "banana");
-    assertContainsEvent("invalid package name '@veggies//:cucumber'");
+    PackageGroup grp = getPackageGroup("fruits", "banana");
+    assertThat(grp.contains(pkgId("veggies", "cucumber"))).isTrue();
   }
 
   @Test
-  public void testAllPackagesInMainRepositoryDoesNotWork() throws Exception {
+  public void testAllPackagesInMainRepositoryWorks() throws Exception {
     scratch.file(
         "fruits/BUILD",
         """
@@ -121,9 +121,8 @@ public void testAllPackagesInMainRepositoryDoesNotWork() throws Exception {
         )
         """);
 
-    reporter.removeHandler(failFastHandler);
-    getPackageGroup("fruits", "apple");
-    assertContainsEvent("invalid package name '@//...'");
+    PackageGroup grp = getPackageGroup("fruits", "apple");
+    assertThat(grp.contains(pkgId("anything"))).isTrue();
   }
 
   // TODO(brandjon): It'd be nice to include a test here that you can cross repositories via
@@ -546,6 +545,7 @@ public void testReduceForSerialization() throws Exception {
   /** Convenience method for obtaining a PackageSpecification. */
   private PackageSpecification pkgSpec(RepositoryName repository, String spec) throws Exception {
     return PackageSpecification.fromString(
+        RepositoryMapping.EMPTY,
         repository, spec, /*allowPublicPrivate=*/ true, /*repoRootMeansCurrentRepo=*/ true);
   }
 

src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java

@@ -973,17 +973,15 @@ public void testBzlVisibility_invalid_badElementType() throws Exception {
   }
 
   @Test
-  public void testBzlVisibility_invalid_packageOutsideRepo() throws Exception {
+  public void testBzlVisibility_packageWithRepoWorks() throws Exception {
     setBuildLanguageOptions("--experimental_bzl_visibility=true");
 
     scratch.file("a/BUILD");
     scratch.file(
         "a/foo.bzl", //
         "visibility([\"@repo//b\"])");
 
-    reporter.removeHandler(failFastHandler);
-    checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed");
-    assertContainsEvent("invalid package name '@repo//b': must start with '//'");
+    checkSuccessfulLookup("//a:foo.bzl");
   }
 
   @Test