Portably escape pipe characters in diff --ignore-matching-lines regexes
#111171
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
This snapshot fell out of sync while the `diff --ignore-matching-lines` regexes were silently broken on Linux.
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
Over in Zulip I mentioned that I was going to add some extra checks as well, to ensure that I did try writing some checks, but they ended up being so complex and fragile that they ultimately didn't seem to be a net improvement, so I've left them out for now. |
|
I might have a go at implementing instantiation-sorting by piping the output through a Python script, so I can get rid of all this |
|
Closing this in favour of the Python-based sort filter that I've written. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 6, 2023
…acrum Fix instrument-coverage tests by using Python to sort instantiation groups rust-lang#110942 was intended to fix a set of `-Cinstrument-coverage` tests, but it ended up silently *breaking* those tests on Linux, for annoying reasons detailed at rust-lang#111171. Dealing with `diff --ignore-matching-lines` across multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output of `llvm-cov show`, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation of `diff`. This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 6, 2023
…acrum Fix instrument-coverage tests by using Python to sort instantiation groups rust-lang#110942 was intended to fix a set of `-Cinstrument-coverage` tests, but it ended up silently *breaking* those tests on Linux, for annoying reasons detailed at rust-lang#111171. Dealing with `diff --ignore-matching-lines` across multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output of `llvm-cov show`, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation of `diff`. This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 12, 2023
…acrum Fix instrument-coverage tests by using Python to sort instantiation groups rust-lang#110942 was intended to fix a set of `-Cinstrument-coverage` tests, but it ended up silently *breaking* those tests on Linux, for annoying reasons detailed at rust-lang#111171. Dealing with `diff --ignore-matching-lines` across multiple platforms has been such a hassle that I've instead written a simple Python script that can detect instantiation groups in the output of `llvm-cov show`, and sort them in a predictable order so that they can be used as snapshots for an ordinary invocation of `diff`. This approach should be much less error-prone, because it can't accidentally ignore the wrong lines, and any unforeseen problems will tend to result in a Python exception or a failing diff.
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.
#110942 was supposed to fix the regexes used by
diff --ignore-matching-linesin the instrument-coverage tests.But based on my investigations in #111116, it turns out that that change successfully fixed the tests on Mac, while silently breaking them on Linux (which is where they actually get run in CI).
The underlying cause is that GNU
diffand Apple/FreeBSDdiffuse slightly different regex dialects, with a crucial difference:|as a literal pipe character, and treats\|as a regex OR operator|as a regex OR operator, and treats\|as a literal pipe character.This is very frustrating, and ideally I would rip out all of this code and replace it with something written in Rust or Python. But for now, my focus is on making a small fix that will restore the tests to proper working order on both Linux and Mac.
Fortunately, there is a way to portably match a literal pipe character in both
diffimplementations: wrap it in a character class[|]. So that's what this PR does.