-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Wire up PathMapper
in sandbox code
#19719
Conversation
7651bf7
to
01cc169
Compare
@larsrc-google Could you review this PR? Happy to provide more context. Could you perhaps also provide an update on #18159 (comment)? Multiplexed sandboxing support for the Javac worker would allow path mapping to work with default Bazel settings. Is there anything I can do to help with this effort? Could it be enabled for Bazel but not Blaze if there are remaining Google-internal issues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are inputs handled with sandboxing?
i.e. I just see references to outputs in this diff.
Inputs are handled centrally in SpawnInputExpander, so that's already covered by the other PR. |
`PathMapper`s rewrite paths in command lines to make them more cache friendly, which requires executor support to stage files at the rewritten paths. This commit wires up the `PathMapper` used by a given `Spawn` in `SpawnInputsExpander`, which takes care of this for inputs to `Spawn`s executed in a sandbox or remotely. Constructs specific to Blaze, filesets and archived tree artifacts, are not covered by this change. An end-to-end test will be added in #18155, but requires #19718, #19719, and #19721. Work towards #6526 Closes #19718. PiperOrigin-RevId: 571109361 Change-Id: Ia38464011f658178ab2a1981a3ddaf5aead7c8fa
@larsrc-google has moved projects, and is currently OOO. Unfortunately, the other person who knows the current state best is also on leave (probably until mid Dec). @hvadehra might have some context on what is blocking multiplexed sandboxing suport but my high-level understanding is that plugins are an issue since they are using absolute paths. When trying to work around that (see code in https://bazel-review.googlesource.com/c/bazel/+/179090), Lars got stuck on the goodtime plugin. Hemanshu might be able to suggest a path forward. |
Thanks for the update, I will take a look at the PR. Fortunately multiplexed sandboxing for Javac isn't really a hard blocker for the path mapping feature, it would just make it easier to use and probably more efficien. @meisterT Could you review this PR as Lars isn't available? |
Tiago thankfully agreed to have a look. |
src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java
Outdated
Show resolved
Hide resolved
`PathMapper`s rewrite paths in command lines to make them more cache friendly, which requires executor support to stage files at the rewritten paths. This commit wires up the `PathMapper` used by a given `Spawn` with sandbox outputs logic for sandboxed and worker sandboxed execution.
01cc169
to
f5848b9
Compare
`PathMapper`s rewrite paths in command lines to make them more cache friendly, which requires executor support to stage files at the rewritten paths. This commit wires up the `PathMapper` used by a given `Spawn` in `RemoteExecutionService`, which ensures that paths of inputs and outputs are correctly mapped before being sent off to the remote executor and mapped back to the correct local paths when downloading the results. An end-to-end test will be added in #18155, but requires #19718, #19719, and #19721. Work towards #6526 Closes #19721. PiperOrigin-RevId: 573806130 Change-Id: Ibbd4ff641eb301d78f5ec54813e65788d786fcea
PathMapper
s rewrite paths in command lines to make them more cache friendly, which requires executor support to stage files at the rewritten paths. This commit wires up thePathMapper
used by a givenSpawn
with sandbox outputs logic for sandboxed and worker sandboxed execution.An end-to-end test will be added in #18155, but requires #19718, #19719, and #19721.
Work towards #6526