Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Java runfiles library repo mapping aware #16549

Closed
wants to merge 9 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Oct 25, 2022

Work towards #16124

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 25, 2022

@Wyverald Could you review this? It's useless without #16534 and #16321, but can be tested and reviewed in isolation.

@fmeum fmeum marked this pull request as ready for review October 26, 2022 08:46
@sgowroji sgowroji added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Oct 26, 2022
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@comius comius self-requested a review November 3, 2022 14:35
@comius
Copy link
Contributor

comius commented Nov 3, 2022

Hey, one more question. I noticed a pattern in the repo, that works and is slightly less invasive:

https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/android/dexer/DexFileSplitterTest.java;l=57-60;drc=0a23d46976c3fc999d44fbd1e37732ec2442d485
https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/android/dexer/BUILD;l=63-66;drc=7ce19e4128f7a5db219623aadf448d5690097140

What do you think about that? Could we ask users to use canonical repo names?

The benefit I see is, that we wouldn't need repo_mapping, changes to Runfiles library or an annotation (in pr #16534). The mechanism is neat, but needs some more effort from the user. Perhaps it's possible to improve on it?

The drawback is, that if you go forward with repo_mappings and annotation, you probably cancel out this mechanism.
WDYT?

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 3, 2022

@comius This pattern is just great for binaries and tests with small scope, which is exactly why I'm pushing so hard to get #16428 in without a flag: It would already be useful in today's WORKSPACE world and would be required with Bzlmod as the "hardcoded repo name + $(location)" approach breaks down there due to repo mapping.

It doesn't work well for binaries with more data deps and in particular doesn't work well with libraries that bring in data deps since the top-level binary would need to pass in all their paths explicitly, which gets infeasible pretty quickly.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 3, 2022

The drawback is, that if you go forward with repo_mappings and annotation, you probably cancel out this mechanism.
WDYT?

This is not a problem: The $(rlocationpath ...) or repo + $(location ...) strings are still accepted by Rlocation. This is possible since canonical repo names aren't valid apparent repo names and vice versa. If a canonical repo name is passed in, it will simply not be repo mapped and used as is.

@comius
Copy link
Contributor

comius commented Nov 3, 2022

This is not a problem: The $(rlocationpath ...) or repo + $(location ...) strings are still accepted by Rlocation. This is possible since canonical repo names aren't valid apparent repo names and vice versa. If a canonical repo name is passed in, it will simply not be repo mapped and used as is.

True.

@comius
Copy link
Contributor

comius commented Nov 3, 2022

It doesn't work well for binaries with more data deps and in particular doesn't work well with libraries that bring in data deps since the top-level binary would need to pass in all their paths explicitly, which gets infeasible pretty quickly.

Ah, thanks for the answer!

tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 8, 2022

I rebased onto #16534 and #16652 so that I can add an integration test.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 8, 2022

@comius I updated the documentation and made this work in sandboxes by looking up the repository mapping under its root symlink _repo_mapping introduced by #16652. Could you review the last three commits? The commits I rebased onto are all part of other PRs.

@comius
Copy link
Contributor

comius commented Nov 8, 2022

@comius I updated the documentation and made this work in sandboxes by looking up the repository mapping under its root symlink _repo_mapping introduced by #16652. Could you review the last three commits? The commits I rebased onto are all part of other PRs.

I did a quick review and the last three commits all look reasonable.

One more thing you should consider is making preload() method a lazy initializer / preloaded runfiles a singleton. Singleton pattern is usually avoided at all costs, however it looks at the moment Bazel's design forces you into a single global instance.

Making it a singleton might prevent involuntary duplication by the user. Libraries using runfiles might be coming from a different repository, that could follow a different philosophy and there might be no way of passing the Runfiles object down to those libraries.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 8, 2022

@comius I can make preload() return a singleton but keep preload(Map) around in case users rely on custom runfiles locations (e.g. when using manifests at install time). What do you think of that?

Should I use WeakReference or SoftReference for the preload() singleton? Not being able to free the manifests memory may otherwise be wasteful.

@comius
Copy link
Contributor

comius commented Nov 8, 2022

@comius I can make preload() return a singleton but keep preload(Map) around in case users rely on custom runfiles locations (e.g. when using manifests at install time). What do you think of that?

Sounds ok.

Should I use WeakReference or SoftReference for the preload() singleton? Not being able to free the manifests memory may otherwise be wasteful.

I think with or without it should be fine. Without it the could would be simpler and with it you can arguably save some memory in some cases. I can't say there's a strong case for one or the other decision.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 8, 2022

@comius I went with a softly cached singleton instance in a new commit.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 8, 2022

@bazel-io flag

@bazel-io 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
@meteorcloudy
Copy link
Member

@bazel-io fork 6.0.0

@bazel-io 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
@oquenchil oquenchil removed their request for review November 9, 2022 16:23
@Wyverald 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
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 10, 2022

Rebased on master, but remains stacked on #16534.

@fmeum
Copy link
Collaborator Author

fmeum commented Nov 10, 2022

Rebased and no longer stacked.

@fmeum fmeum deleted the 16124-java-rlocation branch November 11, 2022 22:58
fmeum added a commit to fmeum/bazel that referenced this pull request Nov 11, 2022
Work towards bazelbuild#16124

Closes bazelbuild#16549.

PiperOrigin-RevId: 487797147
Change-Id: Ic8e643898b145b7ea1e72f4a0deedfd4dfd50242
ShreeM01 pushed a commit that referenced this pull request Nov 13, 2022
Work towards #16124

Closes #16549.

PiperOrigin-RevId: 487797147
Change-Id: Ic8e643898b145b7ea1e72f4a0deedfd4dfd50242
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants