Make repo marker files sensitive to repo mapping changes

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.

PiperOrigin-RevId: 603310262
Change-Id: I806f383e8579fed3533fac9b181efd8b75e76fcd

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;
@@ -240,6 +241,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")
@@ -328,6 +333,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

@@ -112,6 +112,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

@@ -415,6 +415,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

@@ -93,7 +93,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

@@ -1053,6 +1053,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

@@ -2615,4 +2615,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"