-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Prevent miri from being distributed if tests are failing #61656
Conversation
if !builder.config.toolstate.miri.testing() { | ||
println!("skipping Dist Miri stage{} ({})", stage, target); | ||
return None | ||
} |
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.
Any reason you do this after the tempdir is already created?
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.
Also, uh, this is in the Clippy
impl...? Shouldn't the same be added for Miri then?
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 was also about to ask what this testing
method tests, but CI already says it does not even exist. ;)
== TestPass
is anyway clearer, IMO.
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.
yea... I just did this via copy paste in the github interface. Working with a local build now to get this actually working
I don't know Cc @rust-lang/infra |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Tests aren't run in |
Not being able to ensure that the tools we distribute pass tests seems like a severe problem to me. |
aw, that's too bad.
So we are building everything twice? Once in the dist builders and once in the tool builders where we also run tests? Could we deduplicate that for non-cross |
We are building Miri 1 + N times: 1 test and N dist targets. This is fine since all jobs run in parallel. Deduplicating means we need to restructure the CI pipeline from "run everything in parallel" to some sort of DAG and share the built artifacts, which we won't consider in the short term (at least not before the Azure experiment becomes stable). |
We run tests for all tier 1 targets though, right? At least I have seen tool builders for Linux, macOS, Windows. So more like However, from what I gather those are currently not done on the same builder. And that's probably for the better actually, the "tools" builder sets |
Right, so we'll stay with the status quo. What we could do is make miri parse its corresponding toolstate and warn the user that it may be broken |
Uh, just because we currently can't fix this bug doesn't mean that the current behavior is reasonable, does it? I'd rather keep this open TBH. |
Oh sorry, this was the PR, not the bug. Ignore me.^^ |
@oli-obk Sorry yes you're right, should be 2 + N 😅 (Linux and Windows x86_64; we don't have enough capacity to test for Mac yet) |
fixes #60301
r? @RalfJung