Repository: bazelbuild/bazel
Issue: bzlmod + build_python_zip breaks runfiles resolution
Labels: type: bug
P2
team-ExternalDeps
area-Bzlmod

### Description of the bug:


When building a py_binary with `--build_python_zip` and bzlmod enabled, the runfiles cannot be correctly resolved.
Things would work if I remove the `--build_python_zip` option or switch back to the old WORKSPACE style.
I suspect this has something to do with the way bzlmod handle repo mapping.

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


I have a very small example to reproduce the issue: https://github.com/coquelicot/broken-bzlmod-pyzip

Here's the log from my machine:

```bash
 » bazel run example
INFO: Build option --build_python_zip has changed, discarding analysis cache.
INFO: Analyzed target //:example (0 packages loaded, 539 targets configured).
INFO: Found 1 target...
Target //:example up-to-date:
  bazel-bin/example
INFO: Elapsed time: 0.280s, Critical Path: 0.01s
INFO: 4 processes: 4 internal.
INFO: Build completed successfully, 4 total actions
INFO: Running command line: bazel-bin/example
sample
```

```bash
 » bazel run --build_python_zip example
INFO: Build option --build_python_zip has changed, discarding analysis cache.
INFO: Analyzed target //:example (0 packages loaded, 539 targets configured).
INFO: Found 1 target...
Target //:example up-to-date:
  bazel-bin/example
  bazel-bin/example.zip
INFO: Elapsed time: 0.274s, Critical Path: 0.01s
INFO: 4 processes: 3 internal, 1 linux-sandbox.
INFO: Build completed successfully, 4 total actions
INFO: Running command line: bazel-bin/example
Traceback (most recent call last):
  File "/tmp/Bazel.runfiles_tuww8rqk/runfiles/_main/example.py", line 11, in <module>
    main()
  File "/tmp/Bazel.runfiles_tuww8rqk/runfiles/_main/example.py", line 6, in main
    with open(path, "r") as fin:
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/Bazel.runfiles_tuww8rqk/runfiles/pyzip/data.txt'
```

### Which operating system are you running Bazel on?


linux

### What is the output of `bazel info release`?


release 6.1.1

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Have you found anything relevant by searching the web?

The way runfiles are implemented seems to be different with bzlmod: https://github.com/bazelbuild/bazel/issues/16124

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
Looks like the Python rules do not add `symlinks` and `root_symlinks` to the ZIP file, which causes the `_repo_mapping` file to be missing. I will try to fix this.
https://github.com/bazelbuild/bazel/pull/17942 is the easy part, but this is unfortunately complicated on a deeper level: 

The `_repo_mapping` file is currently injected directly in https://cs.opensource.google/bazel/bazel/+/6f12510e39153e316f35ab0a1ec376ece7f1b0a9:src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java;l=421, which makes it invisible to the `runfiles` Starlark API.

We could instead add it in a way similar to https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java;l=125, but that would construct a new `Runfiles` object for every target.

~An alternative could be to add the artifact to every `Runfiles` object directly upon construction (as long as Bzlmod is enabled) and then only register the generating action in `RunfilesSupport`.~ Edit: This doesn't work as those `Runfiles` objects wouldn't be mergeable anymore.

@Wyverald @lberki I could use your help here as I don't have a good grasp on the potential performance implications of these approaches.
~Looked into this some more. Even if `RunfilesSupport` adds the repo mapping artifact to root symlinks, the Python rules still wouldn't see it as it is added only after the rule implementation has run. Maybe we need to expose it on `RuleContext`?~

*Edit*: Cleared up some confusion I had, which led me to this insight: Computing the repo mapping requires the runfiles, so it simply isn't known before the rule commits to its runfiles. Getting it in the context of the rule only seems to be necessary if the rule wants to build a runfiles structure manually for itself - if it did this for some other rule, that rule's repo mapping would be in its runfiles. So unless the Python rules can build the ZIP as part of a separate rule using a macro, maybe `RepoMappingManifestAction` would need to be exposed to Starlark so that users can build their own manifests for arbitrary `runfiles` objects?
Let me see if I understand this correctly: the current way Python rules build ZIP files is that they dissect a `Runfiles` object and put the path to artifact mapping on a command line. But repo mappings make it impossible to determine that mapping solely from the `Runfiles` object. Is that correct?

Shooting from the hip, with the current toolbox, I could imagine creating a "fake" Python binary which is then put on the inputs of an action and then that action does not execute it, but simply extracts its runfiles tree. Would that work?

@lberki That's exactly correct. If forcing such rules to become macros that obtain the runfiles from an actual target is acceptable, then that is certainly the easiest solution.

@rickeylev Could the executable Python rules go through a separate target to generate the runfiles that are then packaged up into the executable ZIP file?
Is the limitation that makes a macro necessary that `FilesToRunProvider` can only be created once per rule and outside the control of Starlark code and that the only way one can currently add a runfiles tree to the inputs of an action is by using `ctx.actions.run(tools=)` ?
> Is the limitation that makes a macro necessary that `FilesToRunProvider` can only be created once per rule and outside the control of Starlark code and that the only way one can currently add a runfiles tree to the inputs of an action is by using `ctx.actions.run(tools=)` ?

That is part of the reason. But even if there were a way to turn `Runfiles` into a `FilesToRunProvider` from Starlark, the logic that adds the repository mapping manifest is part of `RunfilesSupport` and `RunfilesSupplier` and may thus not invoked correctly in that way. This may require additional changes.


> Could the executable Python rules go through a separate target to generate the runfiles that are then packaged up into the executable ZIP file?

Maybe? That last mile logic to generate the executable for the zip-file case is a bit hairy.

Does every target have the repo mapping? Or just certain ones? I'm thinking just have an extra target generated along side with the necessary info, e.g.

```
def py_binary(name, ...
  repo_mapping("_" + name + "_repo_map")
  py_binary_rule(..., _INTERNAL_ONLY_REPO_MAP = "_" + name + "_repo_map")

def repo_mapping_impl(ctx):
  return [DefaultInfo()] # repo mapping automatically added, right?

def py_binary_impl(ctx):
  repo_map = ctx.attr._INTERNAL_ONLY_REPO_MAP.runfiles
```

Couple other thoughts:
* PyBuiltins.java is where various Java-hooks can go. If the necessary info can be easily gotten via Java, that's the place to put such hooks.
* The `input_manifests` arg _might_ be of help? There's a (now unused) helper in PyBuiltins that converts a runfiles object into a RunfilesSupplier.
@rickeylev With Bzlmod enabled, every target has the repo mapping as a root symlinks that is injected in `getRunfilesInputs`: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java;drc=0ce17480390cdced2df8d59249561613e81f446f;l=421
It may thus not be accessible by rule implementations directly.

The artifact itself is created in `RunfilesSupport`: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java;l=135;bpv=1;bpt=1

We may have to make it available from there.
Exposing the RepoMappingManifestAction is part of the problem, but most of what it does is computed beforehand and are inputs into that action. Manually doing that logic does look feasible, though.

Looking at how RunfilesSupport builds the repo mapping, the two pieces we need are `ruleContext.getTransitivePackagesForRunfileRepoMappingManifest()` and something to get `Package.getPackageIdentifier().getRepository()` (or an equivalent).

Unfortunately, these both use the `Package` object, which I don't think has a Starlark equivalent. Can an object that doesn't implement the Starlark classes be passed from Java to Starlark? If so, we can just pass them onto another helper in PyBuiltins to operate on. If not, we'll have to convert Package to something else (probably a string or struct), or move more logic to Java (the map building is pretty simple, so this looks quite feasible).

Making a Starlark implemented rule's RunfilesSupport available to it during its execution is somewhere near impossible, in particular for the Python rules, so that isn't really an option. Problem 1 is RunfilesSupport is created _after_ the rule function completes. If you try to manually create a RunfilesSupport object during execution (`PyBuiltins.new_empty_runfiles_with_middleman` does such), then it creates the same actions multiple times, which triggers duplicate action conflict detection logic later. Problem 2 is, for the Python rules, the duplicate detection ends up triggering the empty runfiles supplier code multiple times during the analysis phase, which requires flattening the runfiles, and the net effect is a huge CPU and memory regression. If there is some sort of solution involving Starlark+RunfilesSupport, I don't know it :).

Er, hm. Is that repo mapping code flattening the runfiles during analysis phase? That doesn't sound good, well, a problem for another time.

If the action did more of the heavy lifting (computing the map), it would make this easier overall _and_ probably also solve the runfiles-being-flattened-during-analysis issue. We could probably reduce it to a simple helper in PyBuiltins that looked something like:

```
public void createRepoMappingManifest(StarlarkRuleContext starlarkCtx, Runfiles runfiles) {
  starlarkCtx.registerAction(new RepoMappingAction(
    starlarkCtx.getRuleContext().getTransitivePackagesForRunfileRepoMappingManifest(),
    runfiles
  ));
}
```
As far as I remember the repo mapping manifest action isn't really doing much work since actions must be serializable, but `Package`s are not. That's also why the `NestedSet` is indeed flattened during analysis time. 

We only need a tiny fraction of the Package information, so maybe that could be solved with some kind of custom serialization logic for `RepoMappingManifestAction` instead? I don't know much about this area though.
I was playing around with this tonight and may have come up with a somewhat simple work around: declare the repo mapping file and add it to the zip, but rely on the subsequent RunfilesSupport code to actually create the file. I think this might also have a bonus: because the same file isn't created twice, duplicate detection doesn't have to run. We can make it a bit less hacky by factoring out a "get file name" function from Runfiles.createRepoMappingManifestAction. I'll poke this some more tomorrow to see how promising it is.

As a side note, this is the second time I've encountered Starlark code wanting to get at some of the things in RunfilesSupport (the other case was par files, which wanted RunfilesSupport for a similar reason), so maybe there is some missing functionality somewhere.
Alright, today's experiment was trying to use a separate target, which looks iffy and unappealing.

* The repo mapping is only generated and added to the runfiles of executable/test targets. This is due to StarlarkRuleConfiguredTargetUtil#addSimpleProviders (see the RunfilesSupport.withExecutable() call, guarded by the "is executable or test" check
* When it is added, its added to the middleman artifact. See RunfilesSupport.createRunfilesMiddleman, where the repoMappingManifest is added to the nested set fed into the middleman
* The code in Runfiles.getRunfilesInputs adds repoMappingManifest to the runfiles manifest, but not the runfiles itself. (and I think this is the manifest that may not be present, depending on a flag?) (Ah, I seem to recall that the manifest and inputs work in tandem: the manifest tells what path to materialize a file under when the runfiles tree is created, and the inputs tell what inputs are actually available)

The net effect is, when you look at a target's runfiles, you don't see the repo mapping because it's in the middleman artifact, which I think is a secret dependency of the executable somehow.

So, I ran the target through `ctx.resolve_tools()`. This returns the middle man artifact (contained within a depset of all necessary files) and an Starlark-opaque RunfilesSupplier object. Well, we have the RunfilesSupplier now, so I fed that into a java hook to call RunfilesSuppler.getRepoMappingManifest(). This sort of works. It did give a repo mapping file.

However, the resulting repo mapping is only correct if the outer target and inner target have the same dependencies (including implicit deps). In my experiments, I used a stub executable with no dependencies as the "inner" target. It's repo map was just the current workspace. The actual repo map includes the various implicit dependencies of py_binary itself (rules_python, platforms, bazel_tools, some others).

So to make this work, the inner target needs to copy all the passed in user values and the implicit deps of py_binary, forward along the transitive runfiles (if it doesn't, the repo map won't be correct) (and this particular runfiles object is unused), produce an unused executable file (to satisfy some starlark validations), and then the outer rule needs to dig out the repo mapping somehow. Note that all the attributes have to continue to exist on the outer rule so that things like aspects can still traverse them. I think there might be a more direct route to get the repo mapping than going through the middleman artifact (I think a java hook to go Target->FilesToRunProvider#getRunfilesSupport is possible), but that doesn't help with the generation of its content.

So, technically it's possible, but it is pretty wasteful. I'm very wary of generating essentially a copy of py_binary() -- the extra target isn't free; we already know this has a real cost. Within Google, we were forced to do this to make PAR files work and saw the effects in the performance metrics, and only deemed it OK because we didn't really have a choice and plan to make it an opt-in behavior instead of always-on behavior.

Ah -- I see the RepoMappingManifestAction PR got merged! I'll experiment with that -- looks pretty straight forward.
@bazel-io flag
@bazel-io fork 6.3.0
@fmeum @rickeylev should this be now cherry-picked and included in the release-6.3.0?

That would be great, yes.

Note that this fix (https://github.com/bazelbuild/bazel/commit/4e60992432a49dda255eb005544ae8c03b30e243) requires https://github.com/bazelbuild/bazel/pull/18419/commits/ee46d1a6e1a3ce65384d87e9f6cc5d939e631018
> That would be great, yes.
> 
> Note that this fix ([4e60992](https://github.com/bazelbuild/bazel/commit/4e60992432a49dda255eb005544ae8c03b30e243)) requires [ee46d1a](https://github.com/bazelbuild/bazel/commit/ee46d1a6e1a3ce65384d87e9f6cc5d939e631018)

@rickeylev @fmeum I did make sure the commit https://github.com/bazelbuild/bazel/commit/ee46d1a6e1a3ce65384d87e9f6cc5d939e631018 was merged. However, it  seems like additional commits needs to be cherry-picked first in order for the commit to be cherrypicked in the release branch. Could you please submit a PR or let us know what commits to include? Currently, there are 3 files that are missing in the release-6.3.0 branch (src/main/java/com/google/devtools/build/lib/rules/python/PyBuiltins.java,   src/main/starlark/builtins_bzl/common/python/py_executable_bazel.bzl,   src/main/starlark/builtins_bzl/common/python/py_internal.bzl).
Ahhh, I forgot most of the Starlark rewrite work didn't make the 6.0 cut. It won't be feasible to port this to 6.x. Even if we did, it'd require re-implementing the fix based on the (now deleted) Java implementation of the rules.
> Ahhh, I forgot most of the Starlark rewrite work didn't make the 6.0 cut. It won't be feasible to port this to 6.x. Even if we did, it'd require re-implementing the fix based on the (now deleted) Java implementation of the rules.

cc: @Wyverald @meteorcloudy 

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index c8b032e3eb9730..546afe042eb40c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -995,9 +995,9 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
+        "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/net/starlark/java/eval",
-        "//third_party:auto_value",
         "//third_party:guava",
         "//third_party:jsr305",
     ],
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java
index 2714680906f90a..59534d2c72f1be 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java
@@ -13,61 +13,69 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 import static java.util.Comparator.comparing;
 
-import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
+import com.google.devtools.build.lib.actions.CommandLineItem.MapFn;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.util.Fingerprint;
 import java.io.PrintWriter;
-import java.util.List;
+import java.util.Map.Entry;
 import java.util.UUID;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
 
 /** Creates a manifest file describing the repos and mappings relevant for a runfile tree. */
-public class RepoMappingManifestAction extends AbstractFileWriteAction {
-  private static final UUID MY_UUID = UUID.fromString("458e351c-4d30-433d-b927-da6cddd4737f");
-
-  private final ImmutableList<Entry> entries;
-  private final String workspaceName;
+public final class RepoMappingManifestAction extends AbstractFileWriteAction {
 
-  /** An entry in the repo mapping manifest file. */
-  @AutoValue
-  public abstract static class Entry {
-    public static Entry of(
-        RepositoryName sourceRepo, String targetRepoApparentName, RepositoryName targetRepo) {
-      return new AutoValue_RepoMappingManifestAction_Entry(
-          sourceRepo, targetRepoApparentName, targetRepo);
-    }
+  private static final UUID MY_UUID = UUID.fromString("458e351c-4d30-433d-b927-da6cddd4737f");
 
-    public abstract RepositoryName sourceRepo();
+  // Uses MapFn's args parameter just like Fingerprint#addString to compute a cacheable fingerprint
+  // of just the repo name and mapping of a given Package.
+  private static final MapFn<Package> REPO_AND_MAPPING_DIGEST_FN =
+      (pkg, args) -> {
+        args.accept(pkg.getPackageIdentifier().getRepository().getName());
 
-    public abstract String targetRepoApparentName();
+        var mapping = pkg.getRepositoryMapping().entries();
+        args.accept(String.valueOf(mapping.size()));
+        mapping.forEach(
+            (apparentName, canonicalName) -> {
+              args.accept(apparentName);
+              args.accept(canonicalName.getName());
+            });
+      };
 
-    public abstract RepositoryName targetRepo();
-  }
+  private final NestedSet<Package> transitivePackages;
+  private final NestedSet<Artifact> runfilesArtifacts;
+  private final String workspaceName;
 
   public RepoMappingManifestAction(
-      ActionOwner owner, Artifact output, List<Entry> entries, String workspaceName) {
+      ActionOwner owner,
+      Artifact output,
+      NestedSet<Package> transitivePackages,
+      NestedSet<Artifact> runfilesArtifacts,
+      String workspaceName) {
     super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /*makeExecutable=*/ false);
-    this.entries =
-        ImmutableList.sortedCopyOf(
-            comparing((Entry e) -> e.sourceRepo().getName())
-                .thenComparing(Entry::targetRepoApparentName),
-            entries);
+    this.transitivePackages = transitivePackages;
+    this.runfilesArtifacts = runfilesArtifacts;
     this.workspaceName = workspaceName;
   }
 
@@ -78,7 +86,7 @@ public String getMnemonic() {
 
   @Override
   protected String getRawProgressMessage() {
-    return "writing repo mapping manifest for " + getOwner().getLabel();
+    return "Writing repo mapping manifest for " + getOwner().getLabel();
   }
 
   @Override
@@ -88,35 +96,61 @@ protected void computeKey(
       Fingerprint fp)
       throws CommandLineExpansionException, EvalException, InterruptedException {
     fp.addUUID(MY_UUID);
+    actionKeyContext.addNestedSetToFingerprint(REPO_AND_MAPPING_DIGEST_FN, fp, transitivePackages);
+    actionKeyContext.addNestedSetToFingerprint(fp, runfilesArtifacts);
     fp.addString(workspaceName);
-    for (Entry entry : entries) {
-      fp.addString(entry.sourceRepo().getName());
-      fp.addString(entry.targetRepoApparentName());
-      fp.addString(entry.targetRepo().getName());
-    }
   }
 
   @Override
   public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx)
       throws InterruptedException, ExecException {
     return out -> {
-      PrintWriter writer = new PrintWriter(out, /*autoFlush=*/ false, ISO_8859_1);
-      for (Entry entry : entries) {
-        if (entry.targetRepoApparentName().isEmpty()) {
-          // The apparent repo name can only be empty for the main repo. We skip this line as
-          // Rlocation paths can't reference an empty apparent name anyway.
-          continue;
-        }
-        // The canonical name of the main repo is the empty string, which is not a valid name for a
-        // directory, so the "workspace name" is used the name of the directory under the runfiles
-        // tree for it.
-        String targetRepoDirectoryName =
-            entry.targetRepo().isMain() ? workspaceName : entry.targetRepo().getName();
-        writer.format(
-            "%s,%s,%s\n",
-            entry.sourceRepo().getName(), entry.targetRepoApparentName(), targetRepoDirectoryName);
-      }
+      PrintWriter writer = new PrintWriter(out, /* autoFlush= */ false, ISO_8859_1);
+
+      ImmutableSet<RepositoryName> reposContributingRunfiles =
+          runfilesArtifacts.toList().stream()
+              .filter(a -> a.getOwner() != null)
+              .map(a -> a.getOwner().getRepository())
+              .collect(toImmutableSet());
+      transitivePackages.toList().stream()
+          .collect(
+              toImmutableSortedMap(
+                  comparing(RepositoryName::getName),
+                  pkg -> pkg.getPackageIdentifier().getRepository(),
+                  Package::getRepositoryMapping,
+                  // All packages in a given repository have the same repository mapping, so the
+                  // particular way of resolving duplicates does not matter.
+                  (first, second) -> first))
+          .forEach(
+              (repoName, mapping) ->
+                  writeRepoMapping(writer, reposContributingRunfiles, repoName, mapping));
       writer.flush();
     };
   }
+
+  private void writeRepoMapping(
+      PrintWriter writer,
+      ImmutableSet<RepositoryName> reposContributingRunfiles,
+      RepositoryName repoName,
+      RepositoryMapping repoMapping) {
+    for (Entry<String, RepositoryName> mappingEntry :
+        ImmutableSortedMap.copyOf(repoMapping.entries()).entrySet()) {
+      if (mappingEntry.getKey().isEmpty()) {
+        // The apparent repo name can only be empty for the main repo. We skip this line as
+        // Rlocation paths can't reference an empty apparent name anyway.
+        continue;
+      }
+      if (!reposContributingRunfiles.contains(mappingEntry.getValue())) {
+        // We only write entries for repos that actually contribute runfiles.
+        continue;
+      }
+      // The canonical name of the main repo is the empty string, which is not a valid name for a
+      // directory, so the "workspace name" is used the name of the directory under the runfiles
+      // tree for it.
+      String targetRepoDirectoryName =
+          mappingEntry.getValue().isMain() ? workspaceName : mappingEntry.getValue().getName();
+      writer.format(
+          "%s,%s,%s\n", repoName.getName(), mappingEntry.getKey(), targetRepoDirectoryName);
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index 3772cf7cb700da..b6154a2ed4d4a5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -14,26 +14,20 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLine;
-import com.google.devtools.build.lib.analysis.RepoMappingManifestAction.Entry;
 import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
 import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
-import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
@@ -41,7 +35,6 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -558,45 +551,9 @@ private static Artifact createRepoMappingManifestAction(
             new RepoMappingManifestAction(
                 ruleContext.getActionOwner(),
                 repoMappingManifest,
-                collectRepoMappings(
-                    Preconditions.checkNotNull(
-                        ruleContext.getTransitivePackagesForRunfileRepoMappingManifest()),
-                    runfiles),
+                ruleContext.getTransitivePackagesForRunfileRepoMappingManifest(),
+                runfiles.getAllArtifacts(),
                 ruleContext.getWorkspaceName()));
     return repoMappingManifest;
   }
-
-  /** Returns the list of entries (unsorted) that should appear in the repo mapping manifest. */
-  private static ImmutableList<Entry> collectRepoMappings(
-      NestedSet<Package> transitivePackages, Runfiles runfiles) {
-    // NOTE: It might appear that the flattening of `transitivePackages` is better suited to the
-    // execution phase rather than here in the analysis phase, but we can't do that since it would
-    // necessitate storing `transitivePackages` in an action, which breaks skyframe serialization
-    // since packages cannot be serialized here.
-
-    ImmutableSet<RepositoryName> reposContributingRunfiles =
-        runfiles.getAllArtifacts().toList().stream()
-            .filter(a -> a.getOwner() != null)
-            .map(a -> a.getOwner().getRepository())
-            .collect(toImmutableSet());
-    Set<RepositoryName> seenRepos = new HashSet<>();
-    ImmutableList.Builder<Entry> entries = ImmutableList.builder();
-    for (Package pkg : transitivePackages.toList()) {
-      if (!seenRepos.add(pkg.getPackageIdentifier().getRepository())) {
-        // Any package from the same repo would have the same repo mapping.
-        continue;
-      }
-      for (Map.Entry<String, RepositoryName> repoMappingEntry :
-          pkg.getRepositoryMapping().entries().entrySet()) {
-        if (reposContributingRunfiles.contains(repoMappingEntry.getValue())) {
-          entries.add(
-              Entry.of(
-                  pkg.getPackageIdentifier().getRepository(),
-                  repoMappingEntry.getKey(),
-                  repoMappingEntry.getValue()));
-        }
-      }
-    }
-    return entries.build();
-  }
 }


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index d89744dfd73bd8..a7d493fe1de797 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -361,16 +361,14 @@ java_test(
     srcs = ["RunfilesRepoMappingManifestTest.java"],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
-        "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
+        "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/analysis:repo_mapping_manifest_action",
-        "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution_impl",
         "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
-        "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
+        "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs",
-        "//src/main/java/com/google/devtools/build/skyframe",
-        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//src/main/java/net/starlark/java/eval",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
         "//third_party:guava",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java
index edb5955f579b64..377164876ce25a 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java
@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelLockFileFunction;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleResolutionFunction;
@@ -30,8 +31,10 @@
 import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue.Injected;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.Path;
 import java.util.Map.Entry;
+import net.starlark.java.eval.EvalException;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -95,10 +98,22 @@ public void setupBareBinaryRule() throws Exception {
         "bare_binary(name='bare_binary')");
   }
 
-  private ImmutableList<String> getRepoMappingManifestForTarget(String label) throws Exception {
+  private RepoMappingManifestAction getRepoMappingManifestActionForTarget(String label)
+      throws Exception {
     Action action = getGeneratingAction(getRunfilesSupport(label).getRepoMappingManifest());
     assertThat(action).isInstanceOf(RepoMappingManifestAction.class);
-    return ((RepoMappingManifestAction) action)
+    return (RepoMappingManifestAction) action;
+  }
+
+  private String computeKey(RepoMappingManifestAction action)
+      throws CommandLineExpansionException, EvalException, InterruptedException {
+    Fingerprint fp = new Fingerprint();
+    action.computeKey(actionKeyContext, /* artifactExpander= */ null, fp);
+    return fp.hexDigestAndReset();
+  }
+
+  private ImmutableList<String> getRepoMappingManifestForTarget(String label) throws Exception {
+    return getRepoMappingManifestActionForTarget(label)
         .newDeterministicWriter(null)
         .getBytes()
         .toStringUtf8()
@@ -228,4 +243,82 @@ public void runfilesFromToolchain() throws Exception {
             "tooled_rule~1.0,bare_rule,bare_rule~1.0")
         .inOrder();
   }
+
+  @Test
+  public void actionRerunsOnRepoMappingChange_workspaceName() throws Exception {
+    rewriteWorkspace("workspace(name='aaa_ws')");
+    scratch.overwriteFile(
+        "MODULE.bazel",
+        "module(name='aaa',version='1.0')",
+        "bazel_dep(name='bare_rule',version='1.0')");
+    scratch.overwriteFile(
+        "BUILD", "load('@bare_rule//:defs.bzl', 'bare_binary')", "bare_binary(name='aaa')");
+
+    RepoMappingManifestAction actionBeforeChange = getRepoMappingManifestActionForTarget("//:aaa");
+
+    rewriteWorkspace("workspace(name='not_aaa_ws')");
+
+    RepoMappingManifestAction actionAfterChange = getRepoMappingManifestActionForTarget("//:aaa");
+    assertThat(computeKey(actionBeforeChange)).isNotEqualTo(computeKey(actionAfterChange));
+  }
+
+  @Test
+  public void actionRerunsOnRepoMappingChange_repoName() throws Exception {
+    rewriteWorkspace("workspace(name='aaa_ws')");
+    scratch.overwriteFile(
+        "MODULE.bazel",
+        "module(name='aaa',version='1.0')",
+        "bazel_dep(name='bare_rule',version='1.0')");
+    scratch.overwriteFile(
+        "BUILD", "load('@bare_rule//:defs.bzl', 'bare_binary')", "bare_binary(name='aaa')");
+
+    RepoMappingManifestAction actionBeforeChange = getRepoMappingManifestActionForTarget("//:aaa");
+
+    scratch.overwriteFile(
+        "MODULE.bazel",
+        "module(name='aaa',version='1.0',repo_name='not_aaa')",
+        "bazel_dep(name='bare_rule',version='1.0')");
+    invalidatePackages();
+
+    RepoMappingManifestAction actionAfterChange = getRepoMappingManifestActionForTarget("//:aaa");
+    assertThat(computeKey(actionBeforeChange)).isNotEqualTo(computeKey(actionAfterChange));
+  }
+
+  @Test
+  public void actionRerunsOnRepoMappingChange_newEntry() throws Exception {
+    rewriteWorkspace("workspace(name='aaa_ws')");
+    scratch.overwriteFile(
+        "MODULE.bazel",
+        "module(name='aaa',version='1.0')",
+        "bazel_dep(name='bare_rule',version='1.0')");
+    scratch.overwriteFile(
+        "BUILD", "load('@bare_rule//:defs.bzl', 'bare_binary')", "bare_binary(name='aaa')");
+
+    registry.addModule(
+        createModuleKey("bbb", "1.0"),
+        "module(name='bbb',version='1.0')",
+        "bazel_dep(name='bare_rule',version='1.0')");
+    scratch.overwriteFile(
+        moduleRoot.getRelative("bbb~1.0").getRelative("WORKSPACE").getPathString());
+    scratch.overwriteFile(moduleRoot.getRelative("bbb~1.0").getRelative("BUILD").getPathString());
+    scratch.overwriteFile(
+        moduleRoot.getRelative("bbb~1.0").getRelative("def.bzl").getPathString(), "BBB = '1'");
+
+    RepoMappingManifestAction actionBeforeChange = getRepoMappingManifestActionForTarget("//:aaa");
+
+    scratch.overwriteFile(
+        "MODULE.bazel",
+        "module(name='aaa',version='1.0')",
+        "bazel_dep(name='bbb',version='1.0')",
+        "bazel_dep(name='bare_rule',version='1.0')");
+    scratch.overwriteFile(
+        "BUILD",
+        "load('@bare_rule//:defs.bzl', 'bare_binary')",
+        "load('@bbb//:def.bzl', 'BBB')",
+        "bare_binary(name='aaa')");
+    invalidatePackages();
+
+    RepoMappingManifestAction actionAfterChange = getRepoMappingManifestActionForTarget("//:aaa");
+    assertThat(computeKey(actionBeforeChange)).isNotEqualTo(computeKey(actionAfterChange));
+  }
 }