compiletest: Stricter parsing of //@ normalize-* headers by Zalathar · Pull Request #126370 · rust-lang/rust (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation5 Commits2 Checks6 Files changed
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
[ Show hidden characters]({{ revealButtonHref }})
I noticed some problems with the existing parser for these headers:
- It is extremely lax, and basically ignores everything other than the text between two pairs of double-quote characters.
- Unlike other name-value headers, it doesn't even check for a colon after the header name, so the test suite contains a mixture of with-colon and without-colon normalization rules.
- If parsing fails, the header is silently ignored.
The latter is especially bad for platform-specific normalization rules, because the lack of normalization probably won't be noticed until the test mysteriously fails in one of the full CI jobs.
r? @oli-obk
rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
Some changes occurred in src/tools/compiletest
cc @jieyouxu
Yes, apparently some directives require the colon or else it silently fails cc #123760. If you notice compiletest directive handling related wonkiness, please do file specific issues and tag them with A-compiletest so at least it can be tracked and we are aware that certain footguns exist.
📌 Commit e09eedb has been approved by oli-obk
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
Rollup of 6 pull requests
Successful merges:
- rust-lang#126347 (Simplify
try_*'s signature onIterator) - rust-lang#126348 (Improve error message if dependency installation in tidy fails)
- rust-lang#126366 (Add a new trait to retrieve StableMir definition Ty)
- rust-lang#126370 (compiletest: Stricter parsing of
//@ normalize-*headers) - rust-lang#126379 (interpret: update doc comment for find_closest_untracked_caller_location)
- rust-lang#126384 (add tracking issue for is_none_or)
Failed merges:
- rust-lang#126388 (const-eval: make lint scope computation consistent)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#126370 - Zalathar:normalize, r=oli-obk
compiletest: Stricter parsing of //@ normalize-* headers
I noticed some problems with the existing parser for these headers:
- It is extremely lax, and basically ignores everything other than the text between two pairs of double-quote characters.
- Unlike other name-value headers, it doesn't even check for a colon after the header name, so the test suite contains a mixture of with-colon and without-colon normalization rules.
- If parsing fails, the header is silently ignored.
The latter is especially bad for platform-specific normalization rules, because the lack of normalization probably won't be noticed until the test mysteriously fails in one of the full CI jobs.
This was referenced
Jun 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request
Require a colon in //@ normalize-*: test headers
The previous parser for //@ normalize-* headers (before rust-lang#126370) was so lax that it did not require : after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds.
This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory.
(Because the normalization parser only runs after the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.)
Addresses one of the points of rust-lang#126372.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Require a colon in //@ normalize-*: test headers
The previous parser for //@ normalize-* headers (before rust-lang#126370) was so lax that it did not require : after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds.
This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory.
(Because the normalization parser only runs after the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.)
Addresses one of the points of rust-lang#126372.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Require a colon in //@ normalize-*: test headers
The previous parser for //@ normalize-* headers (before rust-lang#126370) was so lax that it did not require : after the header name. As a result, the test suite contained a mix of with-colon and without-colon normalize headers, both numbering in the hundreds.
This PR updates the without-colon headers to add a colon (matching the style used by other headers), and then updates the parser to make the colon mandatory.
(Because the normalization parser only runs after the header system identifies a normalize header, this will detect and issue an error for relevant headers that lack the colon.)
Addresses one of the points of rust-lang#126372.
Labels
Area: The testsuite used to check the correctness of rustc
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)