Automated rollback of commit 4ccc21f.

*** Reason for rollback *** bazelbuild#15728 and bazelbuild#14852 have been fixed *** Original change description *** Automated rollback of commit 5de967b. *** Reason for rollback *** Temporarily rolling back until bazelbuild#14852 is fixed. *** Original change description *** 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... *** PiperOrigin-RevId: 460696618 Change-Id: I60be60120aff003ebdb0d8d44ef82d412d81af92
aranguyen · Jul 20, 2022 · 53c2853 · 53c2853
1 parent 6937331
commit 53c2853
Show file tree

Hide file tree

Showing 8 changed files with 47 additions and 31 deletions.
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
@@ -317,16 +317,7 @@ public ImmutableMap<String, RepositoryName> getRepositoryMapping(RepositoryName
       throw new UnsupportedOperationException("Can only access the external package repository"
           + "mappings from the //external package");
     }
-
-    // We are passed a repository name as seen from the main repository, not necessarily
-    // a canonical repository name. So, we first have to find the canonical name for the
-    // repository in question before we can look up the mapping for it.
-    RepositoryName actualRepositoryName =
-        externalPackageRepositoryMappings
-            .getOrDefault(RepositoryName.MAIN, ImmutableMap.of())
-            .getOrDefault(repository.getName(), repository);
-
-    return externalPackageRepositoryMappings.getOrDefault(actualRepositoryName, ImmutableMap.of());
+    return externalPackageRepositoryMappings.getOrDefault(repository, ImmutableMap.of());
   }
 
   /** Get the repository mapping for this package. */
@@ -874,13 +865,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
@@ -449,9 +449,12 @@ public boolean isImmutable() {
   }
 
   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;
@@ -240,21 +241,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     // -- start of historical WorkspaceFileFunction --
     // TODO(adonovan): reorganize and simplify.
 
-    Package.Builder builder =
-        packageFactory.newExternalPackageBuilder(
-            workspaceFile, ruleClassProvider.getRunfilesPrefix(), starlarkSemantics);
-
-    if (chunks.isEmpty()) {
-      return new WorkspaceFileValue(
-          buildAndReportEvents(builder, env),
-          /* loadedModules = */ ImmutableMap.<String, Module>of(),
-          /* loadToChunkMap = */ ImmutableMap.<String, Integer>of(),
-          /* bindings = */ ImmutableMap.<String, Object>of(),
-          workspaceFile,
-          /* idx = */ 0, // first fragment
-          /* hasNext = */ false);
-    }
-
     // Get the state at the end of the previous chunk.
     WorkspaceFileValue prevValue = null;
     if (key.getIndex() > 0) {
@@ -268,15 +254,39 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         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(), repoMapping, starlarkSemantics);
+
+    if (chunks.isEmpty()) {
+      return new WorkspaceFileValue(
+          buildAndReportEvents(builder, env),
+          /* loadedModules = */ ImmutableMap.<String, Module>of(),
+          /* loadToChunkMap = */ ImmutableMap.<String, Integer>of(),
+          /* bindings = */ ImmutableMap.<String, Object>of(),
+          workspaceFile,
+          /* idx = */ 0, // first fragment
+          /* hasNext = */ false);
+    }
 
     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",
         "//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",
@@ -67,6 +68,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",
         "//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",
         "//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
@@ -122,7 +122,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(