[7.2.0] Automatically map paths in Args#map_each
#22322
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
awaiting-review
|
@fmeum could you take a look at the failures please? |
When path mapping is enabled, `File` objects accessed in a user-supplied callback passed to `Args#map_each` automatically have their paths mapped. Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on `map_each` do not need to be modified to use path mapping. This is a reland of 955b31e, which got rolled back due to a 7% CPU time increase on a benchmark caused by frequent comparisons of equal but not reference equal `StarlarkSemantics` used as keys in the `StarlarkClassDescriptor` cache in `CallUtils`. Instead of overriding `equals` and `hashCode` for `PathMapper`, this change subclasses `StarlarkSemantics` to provide a different, reference equal instance as the cache key. This is safe since the value associated with the `path_mapper` key does not affect the availability of any Starlark field or method, just the behavior of their implementations. Work towards bazelbuild#6526 Closes bazelbuild#21952. PiperOrigin-RevId: 629546010 Change-Id: Ib21fa2371a28a02f0c868523b410c5a40c2c6c82 Closes bazelbuild#22221
Should be fixed |
keertk
requested review from
gregestren and
aranguyen
and removed request for
keertk
aranguyen
approved these changes
May 10, 2024
Merged
via the queue into
bazelbuild:release-7.2.0
with commit May 10, 2024
d4d852c
32 checks passed
github-actions
bot
removed
the
awaiting-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When path mapping is enabled,
Fileobjects accessed in a user-supplied callback passed toArgs#map_eachautomatically have their paths mapped.Automatic rewriting is preferable to e.g. passing a rewriter object into the callback: All paths emitted into command lines must be rewritten with path mapping as inputs and outputs are staged at the mapped locations, so the user would need to manually map all paths - there is no choice. As an added benefit, the automatic rewriting ensures that existing rules relying on
map_eachdo not need to be modified to use path mapping.This is a reland of 955b31e, which got rolled back due to a 7% CPU time increase on a benchmark caused by frequent comparisons of equal but not reference equal
StarlarkSemanticsused as keys in theStarlarkClassDescriptorcache inCallUtils. Instead of overridingequalsandhashCodeforPathMapper, this change subclassesStarlarkSemanticsto provide a different, reference equal instance as the cache key. This is safe since the value associated with thepath_mapperkey does not affect the availability of any Starlark field or method, just the behavior of their implementations.Work towards #6526
Closes #21952.
PiperOrigin-RevId: 629546010
Change-Id: Ib21fa2371a28a02f0c868523b410c5a40c2c6c82
Closes #22221