Stage repository mapping manifest as a root symlink by fmeum · Pull Request #16652 · bazelbuild/bazel

fmeum · 2022-11-03T19:03:23Z

By adding the repository mapping manifest to runfiles as a root symlink, it is staged as foo.runfiles/_repo_mapping with all execution strategies. This includes sandboxed and remote execution, which previously did not stage the manifest at all.

As a side effect, runfiles libraries can now find the repository mapping manifest via rlocation("_repo_mapping").

Fixes #16643
Work towards #16124

fmeum · 2022-11-03T19:08:51Z

@lberki @Wyverald

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

fmeum · 2022-11-03T19:49:54Z

@Wyverald Had to fix a check on Windows and added another one for good measure.

lberki · 2022-11-07T13:53:58Z

This looks reasonable:

_repo_mapping cannot clash with repository names
Runfiles manifests are not currently in sandboxes / remote execution so there is no prior art with which we would be inconstent with
I just learned that we do create "symlink trees" on Windows by copying, which also works with this approach

I have one suggestion: this creates an extra Runfiles object, which may be a memory hog. How about adding an extra field to RunfilesSupplier and tweaking it so that it adds that to the runfiles trees it returns instead of creating that extra Runfiles instance?

Wyverald · 2022-11-07T14:13:40Z

to confirm, does this mean we might in some cases have exactly one of foo.runfiles/_repo_mapping and foo.repo_mapping present? And that's fine because it'll always be found by rlocation("_repo_mapping")?

fmeum · 2022-11-07T14:21:29Z

I have one suggestion: this creates an extra Runfiles object, which may be a memory hog. How about adding an extra field to RunfilesSupplier and tweaking it so that it adds that to the runfiles trees it returns instead of creating that extra Runfiles instance?

I will try to implement it that way as I share this concern.

to confirm, does this mean we might in some cases have exactly one of foo.runfiles/_repo_mapping and foo.repo_mapping present? And that's fine because it'll always be found by rlocation("_repo_mapping")?

Yes, with --noenable_runfiles, only foo.repo_mapping will be present, but pointed to by an entry in the runfiles manifest. rlocation("_repo_mapping") will always work (the integration test should ensure this).

fmeum · 2022-11-07T14:47:36Z

@lberki There are number of places that directly consume Runfiles.getRunfilesInputs and don't go through RunfilesSupplier, e.g. SymlinkTreeStrategy and SourceManifestAction. Do you have a suggestion for how I should deal with them?

fmeum · 2022-11-07T22:39:30Z

@lberki I pushed a commit that wires up the root symlink without allocating a new Runfiles in the most straightforward way I could find. It does seem to require passing the manifest into SourceManifestAction and SymlinkTreeAction, which is not so nice.

fmeum · 2022-11-08T22:11:27Z

@bazel-io flag

lberki

Yeah, this is kind of ugly, but I don't have a better idea, either (other than the previous "create new Runfiles instance" one, which I think is worse on the balance. Maybe add a comment to Runfiles.getRunfilesInputs() that explains why the repo mapping manifest can't be there?

Conceptually, the issue is that Runfiles is now two things:

What a configured target propagates upwards (and there is never a repo mapping manifest there)
What is put in the symlink tree (there may be a repo mapping manifest here)

So maybe the best would be to create two separate classes for these two use cases, but I won't insist.

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

By adding the repository mapping manifest to runfiles as a root symlink, it is staged as `foo.runfiles/_repo_mapping` with all execution strategies. This includes sandboxed and remote execution, which previously did not stage the manifest at all. As a side effect, runfiles libraries can now find the repository mapping manifest via `rlocation("_repo_mapping")`.

fmeum · 2022-11-09T08:24:31Z

Maybe add a comment to Runfiles.getRunfilesInputs() that explains why the repo mapping manifest can't be there?

I added a comment.

So maybe the best would be to create two separate classes for these two use cases, but I won't insist.

Fully agree, but that would be a bit over my head right now.

meteorcloudy · 2022-11-09T09:41:40Z

@bazel-io fork 6.0.0

By adding the repository mapping manifest to runfiles as a root symlink, it is staged as `foo.runfiles/_repo_mapping` with all execution strategies. This includes sandboxed and remote execution, which previously did not stage the manifest at all. As a side effect, runfiles libraries can now find the repository mapping manifest via `rlocation("_repo_mapping")`. Fixes bazelbuild#16643 Work towards bazelbuild#16124 Closes bazelbuild#16652. PiperOrigin-RevId: 487532254 Change-Id: I9774b8930337c5967fce92a861cc0db71dea2f0f

By adding the repository mapping manifest to runfiles as a root symlink, it is staged as `foo.runfiles/_repo_mapping` with all execution strategies. This includes sandboxed and remote execution, which previously did not stage the manifest at all. As a side effect, runfiles libraries can now find the repository mapping manifest via `rlocation("_repo_mapping")`. Fixes #16643 Work towards #16124 Closes #16652. PiperOrigin-RevId: 487532254 Change-Id: I9774b8930337c5967fce92a861cc0db71dea2f0f

fmeum force-pushed the 16643-repo-mapping-root-symlink branch from 26e7211 to cbcf695 Compare November 3, 2022 19:08

fmeum marked this pull request as ready for review November 3, 2022 19:08

fmeum requested review from Wyverald and meteorcloudy as code owners November 3, 2022 19:08

fmeum changed the title ~~Stage repository mapping manifest in sandboxes~~ Stage repository mapping manifest as a root symlink Nov 3, 2022

fmeum force-pushed the 16643-repo-mapping-root-symlink branch from cbcf695 to c09ed02 Compare November 3, 2022 19:09

Wyverald reviewed Nov 3, 2022

View reviewed changes

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

Wyverald approved these changes Nov 3, 2022

View reviewed changes

Wyverald requested review from lberki and removed request for meteorcloudy November 3, 2022 19:23

fmeum force-pushed the 16643-repo-mapping-root-symlink branch 2 times, most recently from 74c2ae1 to 9a94a20 Compare November 3, 2022 19:35

ShreeM01 added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Nov 3, 2022

fmeum mentioned this pull request Nov 6, 2022

Make Java runfiles library repo mapping aware #16549

Closed

fmeum force-pushed the 16643-repo-mapping-root-symlink branch from 9a94a20 to 46f5df9 Compare November 7, 2022 21:00

fmeum requested a review from oquenchil as a code owner November 7, 2022 21:00

fmeum force-pushed the 16643-repo-mapping-root-symlink branch from 46f5df9 to 5a2365d Compare November 7, 2022 21:21

This was referenced Nov 8, 2022

Make Bash runfiles library repo mapping aware #16693

Closed

Use _repo_mapping in C++ runfiles library #16701

Closed

Implement "Locating runfiles with Bzlmod" proposal #16124

Closed

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 8, 2022

lberki approved these changes Nov 9, 2022

View reviewed changes

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

fmeum added 4 commits November 9, 2022 09:18

Fix Windows failure and check for manifest entry

d236bb3

Manually wire up _repo_mapping root symlink

d4f4ec9

Add and fix comments

82fa1cb

fmeum force-pushed the 16643-repo-mapping-root-symlink branch from 5a2365d to 82fa1cb Compare November 9, 2022 08:22

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 9, 2022

bazel-io mentioned this pull request Nov 9, 2022

[6.0.0] Stage repository mapping manifest as a root symlink #16717

Closed

Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 9, 2022

copybara-service bot closed this in d78d25e Nov 10, 2022

fmeum deleted the 16643-repo-mapping-root-symlink branch November 10, 2022 15:41

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stage repository mapping manifest as a root symlink#16652

Stage repository mapping manifest as a root symlink#16652
fmeum wants to merge 4 commits intobazelbuild:masterfrom
fmeum:16643-repo-mapping-root-symlink

fmeum commented Nov 3, 2022 •

edited

Loading

Uh oh!

fmeum commented Nov 3, 2022

Uh oh!

Uh oh!

fmeum commented Nov 3, 2022

Uh oh!

lberki commented Nov 7, 2022

Uh oh!

Wyverald commented Nov 7, 2022

Uh oh!

fmeum commented Nov 7, 2022

Uh oh!

fmeum commented Nov 7, 2022 •

edited

Loading

Uh oh!

fmeum commented Nov 7, 2022

Uh oh!

fmeum commented Nov 8, 2022

Uh oh!

lberki left a comment

Uh oh!

Uh oh!

fmeum commented Nov 9, 2022

Uh oh!

meteorcloudy commented Nov 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

fmeum commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Nov 3, 2022

Uh oh!

Uh oh!

fmeum commented Nov 3, 2022

Uh oh!

lberki commented Nov 7, 2022

Uh oh!

Wyverald commented Nov 7, 2022

Uh oh!

fmeum commented Nov 7, 2022

Uh oh!

fmeum commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Nov 7, 2022

Uh oh!

fmeum commented Nov 8, 2022

Uh oh!

lberki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fmeum commented Nov 9, 2022

Uh oh!

meteorcloudy commented Nov 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fmeum commented Nov 3, 2022 •

edited

Loading

fmeum commented Nov 7, 2022 •

edited

Loading