Write a repo mapping manifest in the runfiles directory by Wyverald · Pull Request #16321 · bazelbuild/bazel

Wyverald · 2022-09-21T16:58:51Z

To ensure we can use repo mappings in the runfiles library, this change writes an extra file "my_binary_target.runfiles/_repo_mapping", which contains a bunch of (base_repo_canonical_name, apparent_repo_name, canonical_repo_name) triples. See https://github.com/bazelbuild/proposals/blob/main/designs/2022-07-21-locating-runfiles-with-bzlmod.md for more information.

Work towards #16124

Wyverald · 2022-09-21T16:59:30Z

@fmeum @lberki

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java

lberki · 2022-09-23T07:28:09Z

src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java

+   * more).
+   */
+  @Before
+  public void setupSimpleBinaryRule() throws Exception {


mind adding test cases for:

dependencies caused by aspects

host/exec dependencies

dependencies caused by an late-bound attribute

Based on how the machinery that collects the set of transitive packages works, it looks like all these should work, but it's better to test it.

I'll get to this next. re "host/exec dependencies" though -- why would we want those in runfiles at all?

Files from host/exec dependencies should never be in the runfiles tree, but to the best of my knowledge, this is not enforced anywhere, which means that it's pretty certain that someone does it anyway.

Therefore, packages of host/exec dependencies must also be in the runfiles repo mapping (unless you convince me that the above property is enforced, but I'd be very surprised)

ah I see. I just added a new filtering so that only repos contributing runfiles would appear in the runfiles manifest (#16321 (comment)), so I think we should have this case covered (unless someone is actively including stuff from host/exec deps into their runfiles, in which case they can simply... not).

lberki · 2022-09-23T07:31:51Z

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

      runfilesInputManifest = null;
      runfilesManifest = null;
    }
+    Artifact repoMappingManifest = createRepoMappingManifestAction(ruleContext, owningExecutable);


Do I understand correctly that the same correctness guarantees will apply to the repo manifest as for the runfiles output manifest (and the runfiles tree), which is "nothing"? (I'm not saying this is a bad thing, I don't think you should be solving the correctness problems of runfiles here, just trying to validate my mental model against reality)

I don't quite understand what you mean by "correctness guarantees"; any examples?

Erm, sorry for being terse. My understanding is that Bazel doesn't guarantee that if one does

bazel build //a:b rm bazel-bin/a/b.runfiles/<some file> bazel build //a:b

the runfiles tree is rebuilt except if <some file> is MANIFEST. Do I understand correctly that this would be the same with the repo mapping manifest, i.e. deleting it or changing it would not cause the symlink tree creation action to be re-run?

I don't really know runfiles well enough to answer this question, but my guess is that yeah, the repo mapping manifest is just like the output manifest.

lberki · 2022-09-23T07:32:30Z

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java


    boolean runfilesEnabled = ruleContext.getConfiguration().runfilesEnabled();

    return new RunfilesSupport(


How will repo mapping files work with sandboxes? I suppose they should appear there, but how would that work?

I really don't know how to answer this question :( Does the existing runfiles manifest file work with sandboxes? If so, it doesn't look like the repo mapping manifest file should be any different.

Both sandboxed and remote execution go through https://cs.opensource.google/bazel/bazel/+/59e51fe625f485dc664928ae0ad93035808779f3:src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java;l=392;drc=901c75e459d737220cb8e29649c1b6ba24e2221d;bpv=1;bpt=1, so I think that this PR should just work with them. But of course a test will tell ;-)

Yep, a test case would dispel all my fears :)

The reason why I'm asking is that because I don't know off the top of my head how the runfiles manifest is plumbed to sandboxes and remote execution workers and since they are very special, I can't say with any confidence that the repo mapping file will be there. Ideally, by the time this changes is merged, we both would understand how runfiles output manifests appear in sandboxes / on RBE and be convinced that the repo mapping file does, too (and there would be a test also, of course!)

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java

fmeum · 2022-09-28T21:28:38Z

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

      runfilesManifest = null;
    }
+    Artifact repoMappingManifest =
+        createRepoMappingManifestAction(ruleContext, runfiles, owningExecutable);


Just noticed that we aren't adding the repo mapping manifest to the runfiles manifest, which makes it more difficult to find it at runtime on Windows. If we move this call further up, we could pass the Artifact into SourceManifestAction.

Wyverald · 2022-10-20T17:50:19Z

So that took a while.

In order to make all tests pass, I eventually moved the repo mapping manifest file up a level, so it's now a sibling (instead of a child) of the runfiles tree. This is to avoid any entanglement with SymlinkTreeAction, which nukes the runfiles tree before generating any symlinks.
- I also tried moving the repo mapping manifest writing logic into SymlinkTreeAction, but that breaks the interaction with the flag --nobuild_runfile_links, which delays the symlink tree generation to bazel run time and doesn't actually execute SymlinkTreeAction.
I also went back to the original approach of flattening the NestedSet<Package> in RunfilesSupport instead of in RepoMappingManifestAction because the latter approach breaks some Google-internal tests.
Additionally, the repo mapping manifest file is not created unless we're sure Bzlmod is enabled, which is also required make Google happy (as it creates much fewer actions across the repo).
- This could alternatively be solved by somehow inserting the repo mapping manifest writing logic into SourceManifestAction (the one that creates the input manifest), but that action is an AbstractFileWriteAction which only produces one output file, and it's rather tiresome to create a custom action class just so that it might write 2 files.

With luck, all the tests (both internal and external) should pass now.

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

fmeum · 2022-10-20T18:21:40Z

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

+  private static ImmutableList<Entry> collectRepoMappings(
+      NestedSet<Package> transitivePackages, Runfiles runfiles) {
+    ImmutableSet<RepositoryName> reposContributingRunfiles =
+        runfiles.getAllArtifacts().toList().stream()


This being a path not taken in Blaze and not taken in Bazel by default makes me a bit worried it may regress analysis phase performance outside Google. Were you able to get benchmark data on that or have other reasons to believe it's safe?

I didn't get benchmark data. This change was made out of necessity rather than convenience, since things break internally if a Package is used as to compute an action key in any way.

Also, this is actually still run in Blaze, so its effects will be visible. It's just not run in certain other modes of builds in Google (where the transitive package tracking doesn't happen).

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

fmeum · 2022-10-20T18:31:40Z

src/test/py/bazel/bzlmod/bazel_module_test.py

+
+    # Finally we get to build stuff!
+    self.RunBazel(['build', '//:me', '@bar//:bar'], allow_failure=False)
+    with open(self.Path('bazel-bin/me.repo_mapping'), 'r') as f:


Could you build this check into a binary executed as part of an action? That would give use confidence that the file exists in the sandbox, not just in the output tree.

That said, we may want a test that covers remote execution.

I made the bare_binary fail if it can't find the repo mapping manifest file, and changed bazel build to bazel run. The content assertion is still outside.

With the current way the repo mapping manifest file is generated, I don't think remote execution is a concern anymore -- it's just another normal artifact, and has no entanglement with the special handling around the symlink tree.

and of course the shell script doesn't work on windows... give me a second to fix that

Agreed on the remote execution part. By making the binary a test and running bazel test, you would get coverage for the sandbox case without more code.

src/test/py/bazel/bzlmod/bazel_module_test.py

To ensure we can use repo mappings in the runfiles library, this change writes an extra file "my_binary_target.repo_mapping", which contains a bunch of (base_repo_canonical_name, apparent_repo_name, canonical_repo_name) triples. See https://github.com/bazelbuild/proposals/blob/main/designs/2022-07-21-locating-runfiles-with-bzlmod.md for more information. The extra file is written using a new action "RepoMappingManifestAction", and it's only executed if we know for sure that Bzlmod is enabled. This avoid generating a lot of extra actions that are essentially useless for monorepo setups such as Google's. Work towards #16124 PiperOrigin-RevId: 475820334 Change-Id: I885b4df093bd2c783c57d19f995f420b9b29b53c

Wyverald · 2022-10-25T19:12:28Z

Merged as 527308c

ShreeM01 added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-user-response Awaiting a response from the author labels Sep 21, 2022

fmeum reviewed Sep 22, 2022

View reviewed changes

lberki reviewed Sep 23, 2022

View reviewed changes

Wyverald force-pushed the wyv-repomanifest branch from 0bcf791 to 95c4cc8 Compare September 27, 2022 17:22

This was referenced Sep 28, 2022

Implement "Locating runfiles with Bzlmod" proposal #16124

Closed

Add CurrentRepository() to Python runfiles library #16341

Closed

fmeum reviewed Sep 28, 2022

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java Outdated Show resolved Hide resolved

Wyverald force-pushed the wyv-repomanifest branch from 95c4cc8 to 1e1bc3e Compare September 28, 2022 14:26

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Sep 28, 2022

fmeum reviewed Sep 28, 2022

View reviewed changes

Wyverald force-pushed the wyv-repomanifest branch from 1e1bc3e to 3d2f4d6 Compare October 18, 2022 10:18

Wyverald requested a review from meteorcloudy as a code owner October 18, 2022 10:18

Wyverald force-pushed the wyv-repomanifest branch from 3d2f4d6 to d6de34e Compare October 20, 2022 17:42

Wyverald force-pushed the wyv-repomanifest branch from d6de34e to 05fb4f7 Compare October 20, 2022 17:56

fmeum reviewed Oct 20, 2022

View reviewed changes

src/test/py/bazel/bzlmod/bazel_module_test.py Outdated Show resolved Hide resolved

Wyverald force-pushed the wyv-repomanifest branch from 05fb4f7 to 8f2f6ea Compare October 21, 2022 12:25

fmeum approved these changes Oct 21, 2022

View reviewed changes

This was referenced Oct 25, 2022

Use "apparent name" instead of "local name" in Bzlmod docs #16548

Closed

Make Java runfiles library repo mapping aware #16549

Closed

Wyverald closed this Oct 25, 2022

Wyverald deleted the wyv-repomanifest branch October 25, 2022 19:12


		boolean runfilesEnabled = ruleContext.getConfiguration().runfilesEnabled();

		return new RunfilesSupport(

Conversation

Wyverald commented Sep 21, 2022

Uh oh!

Wyverald commented Sep 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lberki Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wyverald commented Oct 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wyverald commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lberki Sep 29, 2022 •

edited

Loading