[Fix] Extend cache key scrubbing to Empty virtual input path #20926
Conversation
|
cc @tjgq |
github-actions
bot
added
team-Remote-Exec
sgowroji
added
awaiting-user-response
| if (scrubber != null && input instanceof ActionInput) { | ||
| String inputPath = input.equals(VirtualActionInput.EMPTY_MARKER) ? path.getPathString() : ((ActionInput)input).getExecPathString(); | ||
| if (scrubber.shouldOmitInput(inputPath)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Actually - I am now convinced that we should always scrub by PathFragment, not byActionInput.getExecPath().
The reason is runfile trees, which are the only situation in which path != actionInput.getExecPath(): the path refers to the location in the runfiles tree (e.g. bazel-out/xxx/bin/exe.runfiles/foo/bar) while the actionInput refers to the original artifact that will be staged there (e.g. bazel-out/yyy/bin/foo/bar). If we do the matching on the actionInput, a rule to scrub bazel-out/yyy won't apply because the path has a different prefix (bazel-out/xxx). This is basically the same bug you're seeing for empty files, but it applies equally to the non-empty ones.
In light of this, could you instead:
- Change
shouldOmitInputto take aPathFragmentinstead of anActionInput - Amend this logic to always match on the
path(no special case required for empty markers) - Amend the tests that are passing an
ActionInputtoshouldOmitInputsto pass in aPathFragmentinstead
There was a problem hiding this comment.
Makes sense. Will do this week. Thanks @tjgq !
There was a problem hiding this comment.
@oliviernotteghem are you still working on this? It would be great if it could make it into 7.1.0 (the first RC will be out today-ish, but we still have a few days to cherry-pick it)
There was a problem hiding this comment.
I will go ahead and do this myself, because it makes a followup change easier for me.
For runfiles, the effective path differs from the one reported by the ActionInput, and could even be in a different configuration prefix (e.g., an input bazel-out/xxx/foo may appear in a runfiles tree at bazel-out/yyy/bar.runfiles/foo). This is a more general version of bazelbuild#20926. Fixes bazelbuild#20926. PiperOrigin-RevId: 609327629 Change-Id: I8ff2eef626835f012091045a7a4adad52877c7e4
#21472) For runfiles, the effective path differs from the one reported by the ActionInput, and could even be in a different configuration prefix (e.g., an input bazel-out/xxx/foo may appear in a runfiles tree at bazel-out/yyy/bar.runfiles/foo). This is a more general version of #20926. Fixes #20926. PiperOrigin-RevId: 609327629 Change-Id: I8ff2eef626835f012091045a7a4adad52877c7e4
|
Excellent! Thanks for landing this. Sorry got unexpectedly busy last week. |
|
The changes in this PR have been included in Bazel 7.1.0 RC2. Please test out the release candidate and report any issues as soon as possible. |
For runfiles, the effective path differs from the one reported by the ActionInput, and could even be in a different configuration prefix (e.g., an input bazel-out/xxx/foo may appear in a runfiles tree at bazel-out/yyy/bar.runfiles/foo). This is a more general version of bazelbuild#20926. Fixes bazelbuild#20926. PiperOrigin-RevId: 609327629 Change-Id: I8ff2eef626835f012091045a7a4adad52877c7e4
Empty virtual path can also contain platform specific parts, therefore they should also be subject to xplat scrubbing.