-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
fix and (re-)enable Miri cross-target checks on macOS and Windows #103569
Conversation
This comment has been minimized.
This comment has been minimized.
b44939a
to
3ee5370
Compare
Wouldn't it make more sense to relax the sanity checks depending on the target instead of removing them entirely? |
These sanity checks make no sense for |
#103731 disables the cross-test to Windows -- I suspect this PR may actually fix the problem in any case for both macOS and Windows, and it's just coincidence that Windows appeared to work before. |
// Some environments don't want or need these tools, such as when testing Miri. | ||
if env::var_os("BOOTSTRAP_SKIP_TARGET_SANITY").is_some() { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's other checks similar to this (e.g., the cmake check for msvc below). I think my recommendation would be to see if we can refactor the sanity checking as a whole so there's a x.py flag that disables it. I think skipping it entirely won't work today, but if we move some of the detection logic to config.rs that might fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will probably require more digging into x.py than I will have time for, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I can probably make an attempt to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added another check of BOOTSTRAP_SKIP_TARGET_SANITY to the second loop that iterates all targets, which could help in the interim. (When writing this PR I did not notice there are two such loops.)
☔ The latest upstream changes (presumably #103731) made this pull request unmergeable. Please resolve the merge conflicts. |
c091568
to
c905fd2
Compare
@Mark-Simulacrum now that the release has gone through, do you think it'd make sense to merge this PR until someone has time to do the sanity check refactor you mentioned? |
r=me with a short comment on the env variable in bootstrap noting the desire for a refactor here (feel free to just point at this PR in terms of context/explanation) |
@bors r=Mark-Simulacrum rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (d69c33a): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
Delta here is pretty certainly noise, though it's also an improvement so either way fine. |
…ulacrum fix and (re-)enable Miri cross-target checks on macOS and Windows Fixes rust-lang#103519 r? `@Mark-Simulacrum`
Fixes #103519
r? @Mark-Simulacrum