[7.1.0] Make repo marker files sensitive to repo mapping changes (#21172

)

Similar to the fix for #20721,
we write recorded repo mapping entries into the marker file so that repo
fetching is sensitive to any relevant repo mapping changes.

Fixes #20722.

Closes #21131.

Commit
9edaddd

PiperOrigin-RevId: 603310262
Change-Id: I806f383e8579fed3533fac9b181efd8b75e76fcd

Co-authored-by: Xdng Yng <wyverald@gmail.com>

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Table;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.bazel.repository.RepositoryResolvedEvent;
@@ -242,6 +243,10 @@ private RepositoryDirectoryValue.Builder fetchInternal(
     try (Mutability mu = Mutability.create("Starlark repository")) {
       StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics);
       thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener()));
+      var repoMappingRecorder = new Label.RepoMappingRecorder();
+      repoMappingRecorder.mergeEntries(
+          rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries());
+      thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder);
 
       new BazelStarlarkContext(
               BazelStarlarkContext.Phase.LOADING, // ("fetch")
@@ -330,6 +335,16 @@ private RepositoryDirectoryValue.Builder fetchInternal(
         markerData.put("ENV:" + envKey, clientEnvironment.get(envKey));
       }
 
+      for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
+          repoMappingRecorder.recordedEntries().cellSet()) {
+        markerData.put(
+            "REPO_MAPPING:"
+                + repoMappings.getRowKey().getName()
+                + ","
+                + repoMappings.getColumnKey(),
+            repoMappings.getValue().getName());
+      }
+
       env.getListener().post(resolved);
     } catch (NeedsSkyframeRestartException e) {
       // A dependency is missing, cleanup and returns null

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java

@@ -113,6 +113,11 @@ public StarlarkCallable repositoryRule(
     BazelModuleContext moduleContext = BazelModuleContext.ofInnermostBzlOrThrow(thread);
     builder.setRuleDefinitionEnvironmentLabelAndDigest(
         moduleContext.label(), moduleContext.bzlTransitiveDigest());
+    Label.RepoMappingRecorder repoMappingRecorder =
+        thread.getThreadLocal(Label.RepoMappingRecorder.class);
+    if (repoMappingRecorder != null) {
+      builder.setRuleDefinitionEnvironmentRepoMappingEntries(repoMappingRecorder.recordedEntries());
+    }
     builder.setWorkspaceOnly();
     return new RepositoryRuleFunction(
         builder,

src/main/java/com/google/devtools/build/lib/rules/BUILD

@@ -414,6 +414,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_value_dirtiness_checker",
         "//src/main/java/com/google/devtools/build/lib/util",

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java

@@ -86,7 +86,7 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
 
   // The marker file version is inject in the rule key digest so the rule key is always different
   // when we decide to update the format.
-  private static final int MARKER_FILE_VERSION = 3;
+  private static final int MARKER_FILE_VERSION = 4;
 
   // Mapping of rule class name to RepositoryFunction.
   private final ImmutableMap<String, RepositoryFunction> handlers;

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.repository;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -40,6 +41,7 @@
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
+import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -50,6 +52,7 @@
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyframeLookupResult;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.LinkedHashMap;
@@ -205,15 +208,49 @@ private static ImmutableSet<String> getEnviron(Rule rule) {
   public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
       throws InterruptedException {
     return verifyEnvironMarkerData(markerData, env, getEnviron(rule))
-        && verifyMarkerDataForFiles(rule, markerData, env)
-        && verifySemanticsMarkerData(markerData, env);
+        && verifySemanticsMarkerData(markerData, env)
+        && verifyRepoMappingMarkerData(markerData, env)
+        && verifyMarkerDataForFiles(rule, markerData, env);
   }
 
   protected boolean verifySemanticsMarkerData(Map<String, String> markerData, Environment env)
       throws InterruptedException {
     return true;
   }
 
+  private static boolean verifyRepoMappingMarkerData(
+      Map<String, String> markerData, Environment env) throws InterruptedException {
+    ImmutableSet<SkyKey> skyKeys =
+        markerData.keySet().stream()
+            .filter(k -> k.startsWith("REPO_MAPPING:"))
+            // grab the part after the 'REPO_MAPPING:' and before the comma
+            .map(k -> k.substring("REPO_MAPPING:".length(), k.indexOf(',')))
+            .map(k -> RepositoryMappingValue.key(RepositoryName.createUnvalidated(k)))
+            .collect(toImmutableSet());
+    SkyframeLookupResult result = env.getValuesAndExceptions(skyKeys);
+    if (env.valuesMissing()) {
+      return false;
+    }
+    for (Map.Entry<String, String> entry : markerData.entrySet()) {
+      if (!entry.getKey().startsWith("REPO_MAPPING:")) {
+        continue;
+      }
+      int commaIndex = entry.getKey().indexOf(',');
+      RepositoryName fromRepo =
+          RepositoryName.createUnvalidated(
+              entry.getKey().substring("REPO_MAPPING:".length(), commaIndex));
+      String apparentRepoName = entry.getKey().substring(commaIndex + 1);
+      RepositoryMappingValue repoMappingValue =
+          (RepositoryMappingValue) result.get(RepositoryMappingValue.key(fromRepo));
+      if (repoMappingValue == RepositoryMappingValue.NOT_FOUND_VALUE
+          || !RepositoryName.createUnvalidated(entry.getValue())
+              .equals(repoMappingValue.getRepositoryMapping().get(apparentRepoName))) {
+        return false;
+      }
+    }
+    return true;
+  }
+
   private static boolean verifyLabelMarkerData(Rule rule, String key, String value, Environment env)
       throws InterruptedException {
     Preconditions.checkArgument(key.startsWith("FILE:"));

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

@@ -1050,6 +1050,7 @@ private static RuleClass newRuleClass(
         /* optionReferenceFunction= */ RuleClass.NO_OPTION_REFERENCE,
         /* ruleDefinitionEnvironmentLabel= */ null,
         /* ruleDefinitionEnvironmentDigest= */ null,
+        /* ruleDefinitionEnvironmentRepoMappingEntries= */ null,
         new ConfigurationFragmentPolicy.Builder()
             .requiresConfigurationFragments(allowedConfigurationFragments)
             .build(),

src/test/shell/bazel/starlark_repository_test.sh

@@ -2611,4 +2611,93 @@ EOF
   assert_contains 'WORKSPACE' output
 }
 
+function test_repo_mapping_change_in_rule_impl() {
+  # regression test for #20722
+  create_new_workspace
+  cat > MODULE.bazel <<EOF
+r = use_repo_rule("//:r.bzl", "r")
+r(name = "r")
+bazel_dep(name="foo", repo_name="data")
+local_path_override(module_name="foo", path="foo")
+bazel_dep(name="bar")
+local_path_override(module_name="bar", path="bar")
+EOF
+  touch BUILD
+  cat > r.bzl <<EOF
+def _r(rctx):
+  print("I see: " + str(Label("@data")))
+  rctx.file("BUILD", "filegroup(name='r')")
+r=repository_rule(_r)
+EOF
+  mkdir foo
+  cat > foo/MODULE.bazel <<EOF
+module(name="foo")
+EOF
+  mkdir bar
+  cat > bar/MODULE.bazel <<EOF
+module(name="bar")
+EOF
+
+  bazel build @r >& $TEST_log || fail "expected bazel to succeed"
+  expect_log "I see: @@foo~override//:data"
+
+  # So far, so good. Now we make `@data` point to bar instead!
+  cat > MODULE.bazel <<EOF
+r = use_repo_rule("//:r.bzl", "r")
+r(name = "r")
+bazel_dep(name="foo")
+local_path_override(module_name="foo", path="foo")
+bazel_dep(name="bar", repo_name="data")
+local_path_override(module_name="bar", path="bar")
+EOF
+  # for the repo `r`, nothing except the repo mapping has changed.
+  bazel build @r >& $TEST_log || fail "expected bazel to succeed"
+  expect_log "I see: @@bar~override//:data"
+}
+
+function test_repo_mapping_change_in_bzl_init() {
+  # same as above, but tests .bzl init time repo mapping usages
+  create_new_workspace
+  cat > MODULE.bazel <<EOF
+r = use_repo_rule("//:r.bzl", "r")
+r(name = "r")
+bazel_dep(name="foo", repo_name="data")
+local_path_override(module_name="foo", path="foo")
+bazel_dep(name="bar")
+local_path_override(module_name="bar", path="bar")
+EOF
+  touch BUILD
+  cat > r.bzl <<EOF
+CONSTANT = Label("@data")
+def _r(rctx):
+  print("I see: " + str(CONSTANT))
+  rctx.file("BUILD", "filegroup(name='r')")
+r=repository_rule(_r)
+EOF
+  mkdir foo
+  cat > foo/MODULE.bazel <<EOF
+module(name="foo")
+EOF
+  mkdir bar
+  cat > bar/MODULE.bazel <<EOF
+module(name="bar")
+EOF
+
+  bazel build @r >& $TEST_log || fail "expected bazel to succeed"
+  expect_log "I see: @@foo~override//:data"
+
+  # So far, so good. Now we make `@data` point to bar instead!
+  cat > MODULE.bazel <<EOF
+r = use_repo_rule("//:r.bzl", "r")
+r(name = "r")
+bazel_dep(name="foo")
+local_path_override(module_name="foo", path="foo")
+bazel_dep(name="bar", repo_name="data")
+local_path_override(module_name="bar", path="bar")
+EOF
+  # for the repo `r`, nothing except the repo mapping has changed.
+  bazel build @r >& $TEST_log || fail "expected bazel to succeed"
+  expect_log "I see: @@bar~override//:data"
+}
+
 run_suite "local repository tests"