Avoid prematurely recording toolstates #73285
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@bors rollup=never p=1 (When this is approved we should get it in quickly, as it will fix toolstate tracking, at least according to local testing). |
|
After your previous change with the clippy tool, the PR didn't fail even though clippy wasn't testing successfully. Any idea what's up with that? |
|
@bors r+ |
|
📌 Commit 6baf9db8fb09e573b4caf815bf9f0d33b5ca9b56 has been approved by |
bors
added
S-waiting-on-bors
|
I believe that's because clippy was failing the build but that build was explicitly allowed to fail on the tools builder. This moves clippy out of that permissible failure invocation which should fix that bug. |
|
I think my "technically not required" is still true because we would fail the build with these changes if the checking was left as before too, since the check step now properly verifies that all known tools are present. |
I am very confused. We are guaranteed to save TestFail, and as a consequence they become TestPass? What is happening?^^ |
|
Hm, maybe my wording isn't clear; what I'm trying to get across is that previously in dry_run the tool build and test steps both would save off "all is great" statuses because we don't actually run anything. That meant that e.g. if you ran |
Based on the first sentence, I would have expected the last word in the 2nd sentence to be "TestPass". How can "we always save off all is great" cause a "TestFail" result? |
|
Because when we check building, and we see that the build succeeded, we call that testfail (it's really build pass, but that doesn't exist AFAIK). |
Oh. That is really confusing naming in this context ("something passed" leads to a state with "fail" in its name), and I think it is what confused be here. In the final toolstate report, it makes more sense, because there the contrast is test-fail vs test-pass. |
|
Yep, we could easily add a build-pass state but since we always try to run tests if the build passed it's not currently useful. I guess in the future we might want tools that are built but not tested, in which case it would make more sense as a dedicated state. I'd not personally object to adding a dedicated state now, too, just for easier interpretation, but I myself probably don't have the time to do so :) |
|
⌛ Testing commit 6baf9db8fb09e573b4caf815bf9f0d33b5ca9b56 with merge 9c548b5ca54e6e887ffff1205453f6335054388f... |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
Clippy tests currently actually fail, which is why this can't land. |
When we're running with dry_run enabled (i.e. all builds do this initially), we're guaranteed to save of a toolstate of TestFail for tools that aren't tested. In practice, we do test tools as well, so for those tools we would initially record them as being TestPass, and then later on re-record the correct state after actually testing them. However, this would not work well if the build failed for whatever reason (e.g. panicking in bootstrap, or as was the case in 73097, clippy failing to test successfully), we would just go on believing that things passed when they in practice did not. This commit also adjusts saving toolstate to never record clippy explicitly (otherwise, it would be recorded when building it); eventually that'll likely move to other tools as well but not yet. This is deemed simpler than checking everywhere we generically save toolstate. We also move clippy out of the "toolstate" no-fail-fast build into a separate x.py invocation; this should no longer be technically required but provides the nice state of letting us check toolstate for all tools and only then check clippy (giving full results on every build).
Mark-Simulacrum
force-pushed
the
clippy-fail
branch
from
6baf9db
to
399bf38
Compare
|
I'll approve this to get toolstate fixed. Clippy is in no worse a condition with this PR than before. @bors r=RalfJung,oli-obk |
|
📌 Commit 399bf38 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=2 |
|
⌛ Testing commit 399bf38 with merge f22c5e19c643f301307d6dad6de3f1a90a8aa513... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
Windows i686-mingw-2 took 4h15min, just barely too long. :/ |
|
☀️ Test successful - checks-azure |
|
📣 Toolstate changed by #73285! Tested on commit 435f97c. 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). |
Tested on commit rust-lang/rust@435f97c. Direct link to PR: <rust-lang/rust#73285> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 rls on windows: test-pass → build-fail (cc @Xanewok). 💔 rls on linux: test-pass → build-fail (cc @Xanewok). 💔 rustfmt on windows: test-pass → build-fail (cc @topecongiro). 💔 rustfmt on linux: test-pass → build-fail (cc @topecongiro).
When we're running with dry_run enabled (i.e. all builds do this initially), we're
guaranteed to save of a toolstate of TestFail for tools that aren't tested. In practice,
we do test tools as well, so for those tools we would initially record them as being
TestPass, and then later on re-record the correct state after actually testing them.
However, this would not work well if the build failed for whatever reason (e.g. panicking
in bootstrap, or as was the case in #73097, clippy failing to test successfully), we would
just go on believing that things passed when they in practice did not.
This commit also adjusts saving toolstate to never record clippy explicitly (otherwise, it
would be recorded when building it); eventually that'll likely move to other tools as well
but not yet. This is deemed simpler than checking everywhere we generically save
toolstate.
We also move clippy out of the "toolstate" no-fail-fast build into a separate x.py
invocation; this should no longer be technically required but provides the nice state of
letting us check toolstate for all tools and only then check clippy (giving full results
on every build).
r? @oli-obk
Supercedes #73275, also fixes #73274