Revert to using matches instead of find with remote_download_regex
#16476
Conversation
This file contains hidden or 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
It is much easier to accidentally write regexes with pathological performance with `find` than with `matches`. If needed, the `find` functionality can always be obtained with `matches` by prepending and appending `.*` as needed. In addition, common usage scenarios such as matching by file extension are easier to get right: With `matches`, `jar` will visibly fail to have an effect and is easily corrected to `.*jar` (or even `.*\.jar`), whereas with `find` it will silently fetch entire directories that contain the substring `jar`, potentially causing performance regressions.
coeuvre
approved these changes
Oct 14, 2022
|
@kshyanashree In case e8278ed is cherry-picked into 5.3.2, this should probably be cherry-picked as well. |
sgowroji
added
team-Remote-Exec
|
@fmeum I'll include it in the PR. |
brentleyjones
pushed a commit
to brentleyjones/bazel
that referenced
this pull request
Oct 14, 2022
… regular expressions. PiperOrigin-RevId: 470707773 Change-Id: I9cec072e32b641fc4cc068d53d83d95a5fe9c55d (cherry picked from commit e8278ed) Also includes the change in bazelbuild#16476.
fmeum
changed the title
matches insted of find with remote_download_regexmatches instead of find with remote_download_regex
brentleyjones
pushed a commit
to brentleyjones/bazel
that referenced
this pull request
Oct 14, 2022
… regular expressions. PiperOrigin-RevId: 470707773 Change-Id: I9cec072e32b641fc4cc068d53d83d95a5fe9c55d (cherry picked from commit e8278ed) Also includes the change in bazelbuild#16476.
ShreeM01
pushed a commit
that referenced
this pull request
Oct 14, 2022
… regular expressions. (#16478) PiperOrigin-RevId: 470707773 Change-Id: I9cec072e32b641fc4cc068d53d83d95a5fe9c55d (cherry picked from commit e8278ed) Also includes the change in #16476. Co-authored-by: Googler <[email protected]>
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.
Reverts a change made in e8278ed alongside enabling
allowMultiplefor--experimental_remote_download_regex.It is much easier to accidentally write regexes with pathological performance with
findthan withmatches. If needed, thefindfunctionality can always be obtained withmatchesby prepending and appending.*as needed.In addition, common usage scenarios such as matching by file extension are easier to get right: With
matches,jarwill visibly fail to have an effect and is easily corrected to.*jar(or even.*\.jar), whereas withfindit will silently fetch entire directories that contain the substringjar, potentially causing performance regressions.