Path map command lines lazily during expansion #25368
Conversation
fmeum
force-pushed
the
lazy-path-mapping
branch
from
ae62071 to
f905c9a
Compare
fmeum
force-pushed
the
lazy-path-mapping
branch
from
3722f1f to
10d042e
Compare
| @@ -174,7 +185,7 @@ public ArgChunk mapCustomStarlarkArgs(ArgChunk chunk) { | |||
|
|
|||
| // TODO: b/327187486 - This materializes strings when totalArgLength() is called. Can it | |||
There was a problem hiding this comment.
@aranguyen Can we get rid of these special cased actions or are they still needed?
| if (execPath.subFragment(1, 2).getPathString().equals("tmp")) { | ||
| return execPath | ||
| .subFragment(0, 2) | ||
| .getRelative(FIXED_CONFIG_SEGMENT) | ||
| .getRelative(execPath.subFragment(3)); | ||
| } |
There was a problem hiding this comment.
@aranguyen I would like to get rid of this special case to simplify this PR. Could you provide some context on why it's needed? The commit message that introduced it is cut off.
There was a problem hiding this comment.
You mean f19fb8d?
This was the extra context:
adding output path mapping support for TreeArtfifact. cl/584066267 introduced a tree artifact to desugar_jdk_libs builds causing failures when path mapping is on. This change should not effect Bazel as the RBE is capable of returning TreeArtifact without wrapping them in a zip file.
The change would map tree artifact path such as blaze-out/tmp//bin/foo to blaze-out/tmp/cfg/bin/foo
That sounds concerned with the same Android actions you were asking about in your other comment.
What was our latest thinking on Android support? There was this snag but Android vs. Linux isn't the only dimension. There's also varying Android platforms for native deps. And the actions we're discussing in this PR aren't Java compilation actions.
I don't think it's clear if the Starlark APIs are powerful enough to upgrade the mnemonics in your other comment.
If there's value to extract from Android rules I think we should still try. But under the circumstances it seems reasonable to reconsider them from scratch. So I support cleaning up these exceptions.
There was a problem hiding this comment.
What was our latest thinking on Android support? There was this snag but Android vs. Linux isn't the only dimension. There's also varying Android platforms for native deps. And the actions we're discussing in this PR aren't Java compilation actions.
And dbg/opt/fastbuild. And ST-hashes. And target vs. exec. And *san.
I did a quick survey of Android-context javac compilations in Google and see plenty of instances of all these combinations.
There was a problem hiding this comment.
I'm definitely in favor of keeping the functionality, but I was hoping that we could still simplify the stripping logic.
The logic as it is before this PR would turn blaze-out/tmp//bin/foo (is the duplicated forward slash intentional?) into blaze-out/tmp/cfg/foo. Is that correct? I would have thought that you would rather leave the path as is since bin doesn't appear to be a config segment, but rather the bin directory of a different root. If that's true, wouldn't we want the result to be just blaze-out/tmp/bin/foo?
There was a problem hiding this comment.
This is specific to Google's remote executor (in Google's TreeArtifactSpawnHelper). The executor writes tree outputs to blaze-out/tmp/<config-prefix>/bin/foo. Sorry about the double slash confusion. GitHub comment markdown was getting in the way:
Without backslashes: blaze-out/tmp//bin/foo
Same text with backslashes: blaze-out/tmp/<config>/bin/foo
So this is indeed a different segment ordering.
280bbe2 also has some history. But it's really about weakness in Google's RE protocol: for lack of being able to track directory outputs, the executor zips them into a .zip file with a known name, returns then to Bazel, then Bazel unzips the file in these directories, with config hashes added back in, to figure out where to stage final results.
github-actions
bot
added
team-Performance
No description provided.