Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiletest: Remove the -test suffix from normalize directives #134759

Merged
merged 3 commits into from
Dec 27, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Dec 25, 2024

This suffix was an artifact of using the same condition-checking engine as the ignore-* and only-* directives, but in practice we have only 2 tests that legitimately use a condition, and both of them only care about 32-bit vs 64-bit.

This PR detaches normalize-* directives from the condition checker, and replaces it with a much simpler system of four explicit NormalizeKind values. It then takes advantage of that simplicity to get rid of the -test suffix.


Addresses one of the points of #126372.

The new name-checking code is a bit quaint, but I think it's a definite improvement over the status quo.


The corresponding dev-guide update is rust-lang/rustc-dev-guide#2172.

r? jieyouxu

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 25, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Comment on lines -7 to -8
//@ normalize-stderr-64bit: "18446744073709551615" -> "SIZE"
//@ normalize-stderr-32bit: "4294967295" -> "SIZE"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun fact, this hasn't needed normalization since #106873.

@rust-log-analyzer

This comment has been minimized.

@Zalathar
Copy link
Contributor Author

Archaeological note: The two remaining tests that use conditional normalization are:

  • tests/ui/transmute/main.rs
  • tests/ui/consts/transmute-size-mismatch-before-typeck.rs

In both cases, there seems to have been a deliberate attempt to use pointer-size-dependent types, so for now I haven't disturbed them. It's possible that we could just restrict them to //@ only-64bit and remove conditional normalizations altogether, but I didn't want to have to justify that extra step in this PR.

@rust-log-analyzer

This comment has been minimized.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 26, 2024
…ieyouxu

Add more `begin_panic` normalizations to panic backtrace tests

Since rust-lang#123244, these tests have started failing locally on some systems (rust-lang#133997) due to minor variations in how `begin_panic` is printed in the backtrace.

The variation appears to occur on macOS when `rust.debuginfo-level = "line-tables-only"` is set, which is the default in `config.compiler.toml`. It does not occur when the debuginfo level is set to 1.

The variation doesn't seem relevant to these tests, so this PR simply adds another custom normalization rule to account for the variation.

---

Will conflict with rust-lang#134759.
@jieyouxu
Copy link
Member

This is going to take me a bit of time to review (scheduled for tmrw).

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 26, 2024
…ieyouxu

Add more `begin_panic` normalizations to panic backtrace tests

Since rust-lang#123244, these tests have started failing locally on some systems (rust-lang#133997) due to minor variations in how `begin_panic` is printed in the backtrace.

The variation appears to occur on macOS when `rust.debuginfo-level = "line-tables-only"` is set, which is the default in `config.compiler.toml`. It does not occur when the debuginfo level is set to 1.

The variation doesn't seem relevant to these tests, so this PR simply adds another custom normalization rule to account for the variation.

---

Will conflict with rust-lang#134759.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 26, 2024
Rollup merge of rust-lang#134781 - Zalathar:backtrace, r=SparrowLii,jieyouxu

Add more `begin_panic` normalizations to panic backtrace tests

Since rust-lang#123244, these tests have started failing locally on some systems (rust-lang#133997) due to minor variations in how `begin_panic` is printed in the backtrace.

The variation appears to occur on macOS when `rust.debuginfo-level = "line-tables-only"` is set, which is the default in `config.compiler.toml`. It does not occur when the debuginfo level is set to 1.

The variation doesn't seem relevant to these tests, so this PR simply adds another custom normalization rule to account for the variation.

---

Will conflict with rust-lang#134759.
@bors
Copy link
Contributor

bors commented Dec 27, 2024

☔ The latest upstream changes (presumably #134795) made this pull request unmergeable. Please resolve the merge conflicts.

@Zalathar
Copy link
Contributor Author

Rebased due to conflicts with #134781 (which added some more normalize directives); no changes.

@Zalathar
Copy link
Contributor Author

Added some FIXMEs for things I noticed but didn't clean up in this PR, since touching hundreds of tests is enough change for one PR (diff).

This is a little more verbose, but also more explicit, and avoids invoking the
full condition engine when only the pointer-width conditions are used.
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  • I reviewed the test diffs manually.
  • The normalization rule is a bit wonky still, but that's pre-existing so.

@jieyouxu
Copy link
Member

This modifies quite a lot of tests, so giving a higher p due to conflict-prone-ness

@bors r+ rollup p=1

@bors
Copy link
Contributor

bors commented Dec 27, 2024

📌 Commit 835fbcb has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors 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 Dec 27, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 27, 2024

(It can be rolled up if someone is doing rollups)
(EDIT: like me)

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#134606 (ptr::copy: fix docs for the overlapping case)
 - rust-lang#134622 (Windows: Use WriteFile to write to a UTF-8 console)
 - rust-lang#134759 (compiletest: Remove the `-test` suffix from normalize directives)
 - rust-lang#134787 (Spruce up the docs of several queries related to the type/trait system and const eval)
 - rust-lang#134806 (rustdoc: use shorter paths as preferred canonical paths)
 - rust-lang#134815 (Sort triples by name in platform_support.md)
 - rust-lang#134816 (tools: fix build failure caused by PR rust-lang#134420)
 - rust-lang#134819 (Fix mistake in windows file open)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bc3e891 into rust-lang:master Dec 27, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 27, 2024
@bors
Copy link
Contributor

bors commented Dec 27, 2024

⌛ Testing commit 835fbcb with merge 6d3db55...

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2024
Rollup merge of rust-lang#134759 - Zalathar:normalize, r=jieyouxu

compiletest: Remove the `-test` suffix from normalize directives

This suffix was an artifact of using the same condition-checking engine as the `ignore-*` and `only-*` directives, but in practice we have only 2 tests that legitimately use a condition, and both of them only care about 32-bit vs 64-bit.

This PR detaches `normalize-*` directives from the condition checker, and replaces it with a much simpler system of four explicit `NormalizeKind` values. It then takes advantage of that simplicity to get rid of the `-test` suffix.

---

Addresses one of the points of rust-lang#126372.

The new name-checking code is a bit quaint, but I think it's a definite improvement over the status quo.

---

The corresponding dev-guide update is rust-lang/rustc-dev-guide#2172.

r? jieyouxu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants