Use the proper main repo mapping where appropriate

Use the main repo mapping (instead of ALWAYS_FALLBACK) to a) process .bzl load labels in the WORKSPACE file, and b) to process any labels passed to repo rules in either the WORKSPACE file or WORKSPACE-loaded macros. This change only has a noticeable effect for when the workspace name (specified using the `workspace` directive in the WORKSPACE file) is used in labels as `@workspace_name//some:thing`. Before this change, such a label would actually point to `@workspace_name//some:thing`, which is a different target than `@//some:thing`, even though they're backed by the same source. This is because we insert an implicit `local_repository(name='workspace_name',path='.')` clause into the WORKSPACE file (see also #15657 for a similar issue when Bzlmod is enabled). This quirk can cause many subtle bugs, including toolchains being missing because `@workspace_name//:toolchain_type` and `@//:toolchain_type` are in fact distinct toolchain types! Closes #15666. Fixes #15667. PiperOrigin-RevId: 454644054 Change-Id: Icd3742cfdf85eb5cf05281dd043b02ddc6a4e3c1
bazelbuild · Jun 13, 2022 · 5de967b · 5de967b
1 parent 2454a4c
commit 5de967b
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 21 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java
@@ -17,6 +17,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
@@ -62,6 +63,7 @@ public static Rule createRule(
                 Root.fromPath(directories.getWorkspace()),
                 LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
             "dummy_name",
+            RepositoryMapping.ALWAYS_FALLBACK,
             semantics);
     BuildLangTypedAttributeValuesMap attributeValues =
         new BuildLangTypedAttributeValuesMap(attributes);

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -843,13 +843,14 @@ public static Builder newExternalPackageBuilder(
       PackageSettings helper,
       RootedPath workspacePath,
       String workspaceName,
+      RepositoryMapping mainRepoMapping,
       StarlarkSemantics starlarkSemantics) {
     return new Builder(
             helper,
             LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
             workspaceName,
             starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
-            RepositoryMapping.ALWAYS_FALLBACK)
+            mainRepoMapping)
         .setFilename(workspacePath);
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -445,9 +445,12 @@ public boolean isImmutable() {
 
   @VisibleForTesting // exposed to WorkspaceFileFunction and BzlmodRepoRuleFunction
   public Package.Builder newExternalPackageBuilder(
-      RootedPath workspacePath, String workspaceName, StarlarkSemantics starlarkSemantics) {
+      RootedPath workspacePath,
+      String workspaceName,
+      RepositoryMapping mainRepoMapping,
+      StarlarkSemantics starlarkSemantics) {
     return Package.newExternalPackageBuilder(
-        packageSettings, workspacePath, workspaceName, starlarkSemantics);
+        packageSettings, workspacePath, workspaceName, mainRepoMapping, starlarkSemantics);
   }
 
   // This function is public only for the benefit of skyframe.PackageFunction,

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 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.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
@@ -238,9 +239,33 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     // -- start of historical WorkspaceFileFunction --
     // TODO(adonovan): reorganize and simplify.
 
+    // Get the state at the end of the previous chunk.
+    WorkspaceFileValue prevValue = null;
+    if (key.getIndex() > 0) {
+      prevValue =
+          (WorkspaceFileValue)
+              env.getValue(WorkspaceFileValue.key(workspaceFile, key.getIndex() - 1));
+      if (prevValue == null) {
+        return null;
+      }
+      if (prevValue.next() == null) {
+        return prevValue;
+      }
+    }
+    RepositoryMapping repoMapping;
+    if (prevValue == null) {
+      repoMapping = RepositoryMapping.ALWAYS_FALLBACK;
+    } else {
+      repoMapping =
+          RepositoryMapping.createAllowingFallback(
+              prevValue
+                  .getRepositoryMapping()
+                  .getOrDefault(RepositoryName.MAIN, ImmutableMap.of()));
+    }
+
     Package.Builder builder =
         packageFactory.newExternalPackageBuilder(
-            workspaceFile, ruleClassProvider.getRunfilesPrefix(), starlarkSemantics);
+            workspaceFile, ruleClassProvider.getRunfilesPrefix(), repoMapping, starlarkSemantics);
 
     if (chunks.isEmpty()) {
       return new WorkspaceFileValue(
@@ -254,28 +279,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)
           ImmutableMap.of());
     }
 
-    // Get the state at the end of the previous chunk.
-    WorkspaceFileValue prevValue = null;
-    if (key.getIndex() > 0) {
-      prevValue =
-          (WorkspaceFileValue)
-              env.getValue(WorkspaceFileValue.key(workspaceFile, key.getIndex() - 1));
-      if (prevValue == null) {
-        return null;
-      }
-      if (prevValue.next() == null) {
-        return prevValue;
-      }
-    }
-
     List<StarlarkFile> chunk = chunks.get(key.getIndex());
 
     // Parse the labels in the chunk's load statements.
     ImmutableList<Pair<String, Location>> programLoads =
         BzlLoadFunction.getLoadsFromStarlarkFiles(chunk);
     ImmutableList<Label> loadLabels =
-        BzlLoadFunction.getLoadLabels(
-            env.getListener(), programLoads, rootPackage, RepositoryMapping.ALWAYS_FALLBACK);
+        BzlLoadFunction.getLoadLabels(env.getListener(), programLoads, rootPackage, repoMapping);
     if (loadLabels == null) {
       NoSuchPackageException e =
           PackageFunction.PackageFunctionException.builder()

diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD
@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark",
+        "//src/main/java/com/google/devtools/build/lib/cmdline:cmdline-primitives",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",
@@ -68,6 +69,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark",
+        "//src/main/java/com/google/devtools/build/lib/cmdline:cmdline-primitives",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",

diff --git a/...om/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/...om/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java
@@ -26,6 +26,7 @@
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.io.CharStreams;
 import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Package;
@@ -139,6 +140,7 @@ protected void setUpContextForRule(
             DefaultPackageSettings.INSTANCE,
             RootedPath.toRootedPath(root, workspaceFile),
             "runfiles",
+            RepositoryMapping.ALWAYS_FALLBACK,
             starlarkSemantics);
     ExtendedEventHandler listener = Mockito.mock(ExtendedEventHandler.class);
     Rule rule =

diff --git a/src/test/java/com/google/devtools/build/lib/packages/BUILD b/src/test/java/com/google/devtools/build/lib/packages/BUILD
@@ -20,6 +20,7 @@ java_library(
     name = "PackageTestsUtil",
     srcs = ["WorkspaceFactoryTestHelper.java"],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/cmdline:cmdline-primitives",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/vfs",

diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
@@ -123,7 +123,10 @@ public void testCreateWorkspaceRule() throws Exception {
     Path myPkgPath = scratch.resolve("/workspace/WORKSPACE");
     Package.Builder pkgBuilder =
         packageFactory.newExternalPackageBuilder(
-            RootedPath.toRootedPath(root, myPkgPath), "TESTING", StarlarkSemantics.DEFAULT);
+            RootedPath.toRootedPath(root, myPkgPath),
+            "TESTING",
+            RepositoryMapping.ALWAYS_FALLBACK,
+            StarlarkSemantics.DEFAULT);
 
     Map<String, Object> attributeValues = new HashMap<>();
     attributeValues.put("name", "foo");

diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java
@@ -20,6 +20,7 @@
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.Package.Builder.DefaultPackageSettings;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -35,6 +36,7 @@
 /** Parses a WORKSPACE file with the given content. */
 // TODO(adonovan): delete this junk class.
 final class WorkspaceFactoryTestHelper {
+
   private final Root root;
   private Package.Builder builder;
   private StarlarkSemantics starlarkSemantics;
@@ -67,6 +69,7 @@ void parse(String... args) {
             DefaultPackageSettings.INSTANCE,
             RootedPath.toRootedPath(root, workspaceFilePath),
             "",
+            RepositoryMapping.ALWAYS_FALLBACK,
             StarlarkSemantics.DEFAULT);
     WorkspaceFactory factory =
         new WorkspaceFactory(