From d001cdfe9e6a78c4713055ccc1b1830eb37225bb Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Wed, 30 Oct 2024 12:51:30 -0700
Subject: [PATCH] Allow `external` top-level dir in Bzlmod-managed non-main
 repos

Also remove an unused method.

RELNOTES: External repositories that are managed by Bzlmod can now contain a top-level `external` directory or package.

Closes #24126.

PiperOrigin-RevId: 691521324
Change-Id: I84d11f2163ce5a6e34578afb6a2cc9793d78f0b1
---
 .../devtools/build/lib/cmdline/Label.java     |  18 ++--
 .../build/lib/cmdline/RepositoryName.java     |  14 ---
 .../lib/skyframe/ProcessPackageDirectory.java |   8 +-
 .../lib/analysis/util/BuildViewTestCase.java  |   8 +-
 .../shell/bazel/external_integration_test.sh  | 102 ++++++++++++++++++
 5 files changed, 120 insertions(+), 30 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
index e8e1c062e485b8..6f5c7fff99bbeb 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -96,9 +96,7 @@ public final class Label implements Comparable<Label>, StarlarkValue, SkyKey, Co
           // Used for select's `//conditions:default` label (not a target)
           "conditions",
           // Used for the public and private visibility labels (not targets)
-          "visibility",
-          // There is only one //external package
-          LabelConstants.EXTERNAL_PACKAGE_NAME.getPathString());
+          "visibility");
 
   // Intern "__pkg__" and "__subpackages__" pseudo-targets, which appears in labels used for
   // visibility specifications. This saves a couple tenths of a percent of RAM off the loading
@@ -181,9 +179,17 @@ private static RepositoryName computeRepoNameWithRepoContext(
     if (parts.repo() == null) {
       // Certain package names when used without a "@" part are always absolutely in the main repo,
       // disregarding the current repo and repo mappings.
-      return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg())
-          ? RepositoryName.MAIN
-          : repoContext.currentRepo();
+      if (ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg())) {
+        return RepositoryName.MAIN;
+      }
+      // The legacy //external package can only be referenced by external repos defined in
+      // WORKSPACE, which never use strict visibility. For the main repo repoContext.currentRepo()
+      // is equal to RepositoryName.MAIN.
+      if (LabelConstants.EXTERNAL_PACKAGE_NAME.getPathString().equals(parts.pkg())
+          && repoContext.repoMapping().ownerRepo() == null) {
+        return RepositoryName.MAIN;
+      }
+      return repoContext.currentRepo();
     }
     if (parts.repoIsCanonical()) {
       // This label uses the canonical label literal syntax starting with two @'s ("@@foo//bar").
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
index fca59bbe0c93ff..6ad69911a5b002 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -253,20 +253,6 @@ public String getOwnerRepoDisplayString() {
     }
   }
 
-  // Must only be called if isVisible() returns true.
-  public String getOwnerModuleDisplayString() {
-    Preconditions.checkNotNull(ownerRepoIfNotVisible);
-    if (ownerRepoIfNotVisible.isMain()) {
-      return "root module";
-    } else {
-      return String.format(
-          "module '%s'",
-          ownerRepoIfNotVisible
-              .getName()
-              .substring(0, ownerRepoIfNotVisible.getName().indexOf('+')));
-    }
-  }
-
   /** Returns if this is the main repository. */
   public boolean isMain() {
     return equals(MAIN);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
index aa8d1383ab2cf2..1057078b756ee5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java
@@ -282,9 +282,11 @@ private Iterable<SkyKey> getSubdirDeps(
       }
       String basename = dirent.getName();
       PathFragment subdirectory = rootRelativePath.getRelative(basename);
-      if (!siblingRepositoryLayout && subdirectory.equals(LabelConstants.EXTERNAL_PACKAGE_NAME)) {
-        // Subpackages under //external can be processed only when
-        // --experimental_sibling_repository_layout is set.
+      if (!siblingRepositoryLayout
+          && subdirectory.equals(LabelConstants.EXTERNAL_PACKAGE_NAME)
+          && repositoryName.isMain()) {
+        // Subpackages under //external in the main repo can be processed only
+        // when --experimental_sibling_repository_layout is set.
         continue;
       }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 15b16766b7a4e7..652bc63f92d98b 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -109,7 +109,6 @@
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction;
 import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -1279,12 +1278,7 @@ protected Rule scratchRule(String packageName, String ruleName, String... lines)
       throws Exception {
     // Allow to create the BUILD file also in the top package.
     String buildFilePathString = packageName.isEmpty() ? "BUILD" : packageName + "/BUILD";
-    if (packageName.equals(LabelConstants.EXTERNAL_PACKAGE_NAME.getPathString())) {
-      buildFilePathString = "WORKSPACE";
-      scratch.overwriteFile(buildFilePathString, lines);
-    } else {
-      scratch.file(buildFilePathString, lines);
-    }
+    scratch.file(buildFilePathString, lines);
     skyframeExecutor.invalidateFilesUnderPathForTesting(
         reporter,
         new ModifiedFileSet.Builder().modify(PathFragment.create(buildFilePathString)).build(),
diff --git a/src/test/shell/bazel/external_integration_test.sh b/src/test/shell/bazel/external_integration_test.sh
index 527b61399e21b9..c63d26a5ab70ba 100755
--- a/src/test/shell/bazel/external_integration_test.sh
+++ b/src/test/shell/bazel/external_integration_test.sh
@@ -2950,6 +2950,52 @@ EOF
   expect_log "//external/nested:a2"
 }
 
+function test_query_external_packages_in_other_repo() {
+  cat > $(setup_module_dot_bazel) <<EOF
+local_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository")
+local_repository(
+  name="other_repo",
+  path="other_repo",
+)
+EOF
+
+  mkdir -p other_repo/external/nested
+  mkdir -p other_repo/not-external
+
+  touch other_repo/REPO.bazel
+
+  cat > other_repo/external/BUILD <<EOF
+filegroup(
+    name = "a1",
+    srcs = [],
+)
+EOF
+  cat > other_repo/external/nested/BUILD <<EOF
+filegroup(
+    name = "a2",
+    srcs = [],
+)
+EOF
+
+  cat > other_repo/not-external/BUILD <<EOF
+filegroup(
+    name = "b",
+    srcs = [],
+)
+EOF
+
+  bazel query @other_repo//... >& $TEST_log || fail "Expected build/run to succeed"
+  expect_log "@other_repo//not-external:b"
+  expect_log "@other_repo//external:a1"
+  expect_log "@other_repo//external/nested:a2"
+
+  bazel query --experimental_sibling_repository_layout @other_repo//... >& $TEST_log \ ||
+    fail "Expected build/run to succeed"
+  expect_log "@other_repo//not-external:b"
+  expect_log "@other_repo//external:a1"
+  expect_log "@other_repo//external/nested:a2"
+}
+
 function test_query_external_all_targets() {
   mkdir -p external/nested
   mkdir -p not-external
@@ -3136,4 +3182,60 @@ EOF
   expect_log "LAZYEVAL_KEY=xal3"
 }
 
+function test_external_package_in_other_repo() {
+  cat > $(setup_module_dot_bazel) <<EOF
+local_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository")
+local_repository(
+  name="other_repo",
+  path="other_repo",
+)
+EOF
+
+  mkdir -p other_repo/external/java/a
+  touch other_repo/REPO.bazel
+
+  cat > other_repo/external/java/a/BUILD <<EOF
+java_library(name='a', srcs=['A.java'])
+EOF
+
+  cat > other_repo/external/java/a/A.java << EOF
+package a;
+public class A {
+  public static void main(String[] args) {
+    System.out.println("hello world");
+  }
+}
+EOF
+
+  bazel build @other_repo//external/java/a:a || fail "build failed"
+}
+
+function test_external_dir_in_other_repo() {
+  cat > $(setup_module_dot_bazel) <<EOF
+local_repository = use_repo_rule("@bazel_tools//tools/build_defs/repo:local.bzl", "local_repository")
+local_repository(
+  name="other_repo",
+  path="other_repo",
+)
+EOF
+
+  mkdir -p other_repo/external/java/a
+  touch other_repo/REPO.bazel
+
+  cat > other_repo/BUILD <<EOF
+java_library(name='a', srcs=['external/java/a/A.java'])
+EOF
+
+  cat > other_repo/external/java/a/A.java << EOF
+package a;
+public class A {
+  public static void main(String[] args) {
+    System.out.println("hello world");
+  }
+}
+EOF
+
+  bazel build @other_repo//:a || fail "build failed"
+}
+
 run_suite "external tests"