From 2b4c509ea2ffec6c3e6301b5967759490188d6d7 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Thu, 26 Feb 2026 10:44:35 +0000 Subject: [PATCH] Support external repository labels in package_group --- .../starlark/BazelBuildApiGlobals.java | 9 ++++- .../build/lib/packages/PackageGroup.java | 1 + .../lib/packages/PackageSpecification.java | 40 ++++++++++++++----- .../PackageGroupStaticInitializationTest.java | 2 + .../build/lib/packages/PackageGroupTest.java | 18 ++++----- .../lib/skyframe/BzlLoadFunctionTest.java | 6 +-- 6 files changed, 50 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java index 3277f92aa3606e..0ed3f29e02fc8e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java +++ b/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,12 +65,15 @@ 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 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 specStrings = Sequence.cast(value, String.class, "visibility list"); @@ -76,7 +81,7 @@ public void visibility(Object value, StarlarkThread thread) throws EvalException 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(); diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java index 071c35fb871ed1..fc3880befcc006 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java +++ b/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, diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java index ca518950703956..9634e471354211 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java +++ b/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, diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java index b136aa79831c72..0dba255ec34a7b 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java +++ b/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, diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java index 0703c60d91445e..e7f8313b67044f 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java +++ b/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); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java index 3db87c715487ab..6cf712754925db 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java @@ -973,7 +973,7 @@ 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"); @@ -981,9 +981,7 @@ public void testBzlVisibility_invalid_packageOutsideRepo() throws Exception { "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