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

Try_run must only be used if toolstate is populated #73097

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jun 7, 2020

Clippy's tests were failing the build, but that failure was ignored in favor of checking toolstate. This is the correct behavior for toolstate-checked tools, but Clippy no longer updates its toolstate status as it should always build.

The previous PR of this kind didn't catch this as I expected x.py failures to always lead to a non-successful build in CI, but that's not the case specifically for tool testing.

Clippy's tests were failing the build, but that failure was ignored in favor of
checking toolstate. This is the correct behavior for toolstate-checked tools,
but Clippy no longer updates its toolstate status as it should always build.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 8, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2020

📌 Commit 6f01576 has been approved by oli-obk

@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 Jun 8, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 11, 2020
Try_run must only be used if toolstate is populated

Clippy's tests were failing the build, but that failure was ignored in favor of checking toolstate. This is the correct behavior for toolstate-checked tools, but Clippy no longer updates its toolstate status as it should always build.

The previous PR of this kind didn't catch this as I expected x.py failures to always lead to a non-successful build in CI, but that's not the case specifically for tool testing.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 11, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#72180 (remove extra space from crate-level doctest names)
 - rust-lang#73012 (Show `SyntaxContext` in formatted `Span` debug output)
 - rust-lang#73097 (Try_run must only be used if toolstate is populated)
 - rust-lang#73169 (Handle assembler warnings properly)
 - rust-lang#73182 (Track span of function in method calls, and use this in #[track_caller])
 - rust-lang#73207 (Clean up E0648 explanation)
 - rust-lang#73230 (Suggest including unused asm arguments in a comment to avoid error)

Failed merges:

r? @ghost
@bors bors merged commit 8d62099 into rust-lang:master Jun 11, 2020
@RalfJung
Copy link
Member

I think this broke toolstate tracking: #73274

@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2020

This PR solely touches clippy, not sure how it could have broken miri

@RalfJung
Copy link
Member

The rollup where this landed (#73246) also marked rls and rustfmt as non-broken, which is probably bogus. It could be another PR in that rollup, but this seemed the most likely...

@RalfJung
Copy link
Member

Or maybe rls and rustfmt actually were fixed, and that rollup has nothing to do with #73274, and something else broke Miri toolstate tracking?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2020

@Mark-Simulacrum Mark-Simulacrum deleted the clippy-fail branch June 12, 2020 13:36
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2020
…ng,oli-obk

Avoid prematurely recording toolstates

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 rust-lang#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 rust-lang#73275, also fixes rust-lang#73274
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants