Fix `x.py clippy` #77351

jyn514 · 2020-09-29T23:51:59Z

I don't think this ever worked.

Fixes #77309. --fix support is a work in progress, but works for a very small subset of libtest.

This works by using the host cargo-clippy driver; it does not use stage0.txt at all. To mitigate confusion from this, it gives an error if you don't have rustc +nightly as the default rustc in $PATH. Additionally, it means that bootstrap can't set RUSTC; this makes it no longer possible for clippy to detect the sysroot itself. Instead, bootstrap passes the sysroot to cargo.

r? @ghost

jyn514 · 2020-09-30T00:22:26Z

Update: the issue is that clippy was looking at rustc, not $RUSTC. It could be fixed on the clippy side and then bootstrap wouldn't need to hack around it.

Mark-Simulacrum · 2020-09-30T00:29:16Z

Hm

Mark-Simulacrum · 2020-09-30T00:31:21Z

Hm, so I think this should be fixed in clippy. If it's using the wrong rustc this is at best a temporary patch - eventually a more serious problem than just the sysroot will arise, though I'm not entirely sure why clippy is running rustc at all.

We might also want to have x.py download clippy on demand (when running clippy) so that there's less version problems.

jyn514 · 2020-09-30T00:36:22Z

We might also want to have x.py download clippy on demand (when running clippy) so that there's less version problems.

This isn't a clippy version issue, this is a rustc version issue. Clippy runs the rustc in $PATH when it should run $RUSTC instead.

I'm not entirely sure why clippy is running rustc at all.

See rust-lang/rust-clippy#3546

I think this should be fixed in clippy

Ok, I'll do that here (cc @flip1995). I'm going to keep the bootstrap hack for now so I don't have to build rustc/clippy from source, but this is the fix:

diff --git a/src/tools/clippy/src/driver.rs b/src/tools/clippy/src/driver.rs
index f4f2259ce..5e688d72d 100644
--- a/src/driver.rs
+++ b/src/driver.rs
@@ -323,7 +323,7 @@ pub fn main() {
                 toolchain_path(home, toolchain)
             })
             .or_else(|| {
-                Command::new("rustc")
+                Command::new(env::var("RUSTC").unwrap_or("rustc"))
                     .arg("--print")
                     .arg("sysroot")
                     .output()

flip1995 · 2020-09-30T00:46:13Z

I'm not entirely sure why clippy is running rustc at all.

Clippy needs some sysroot, so that clippy-driver can be used without cargo (practically just like rustc). Clippy tries to get the sysroot in the following order:

        // Get the sysroot, looking from most specific to this invocation to the least:
        // - command line
        // - runtime environment
        //    - SYSROOT
        //    - RUSTUP_HOME, MULTIRUST_HOME, RUSTUP_TOOLCHAIN, MULTIRUST_TOOLCHAIN
        // - sysroot from rustc in the path
        // - compile-time environment
        //    - SYSROOT
        //    - RUSTUP_HOME, MULTIRUST_HOME, RUSTUP_TOOLCHAIN, MULTIRUST_TOOLCHAIN

For the "- sysroot from rustc in the path", Clippy executes rustc --print sysroot. This uses the rustc from PATH, which is reasonable for most environments, but makes problems, if there are multiple rustc binaries that could be executed. Since this doesn't really happen in the wild, we never noticed this from the Clippy side.

Mark-Simulacrum · 2020-09-30T01:45:25Z

This isn't a clippy version issue, this is a rustc version issue. Clippy runs the rustc in $PATH when it should run $RUSTC instead.

Right now, yes, there's no version issue, but stable/nightly clippy isn't guaranteed to work.

Anyway, I'm fine with merging this for now, r=me.

jyn514 · 2020-10-01T01:41:50Z

Anyway, I'm fine with merging this for now, r=me.

Sorry, I didn't say - this does not fix x.py clippy, just gets it part of the way. It runs into issues while compiling libstd dependencies like cc:

error[E0463]: can't find crate for `std`
error: could not compile `cc`

I think it might need more of the logic in rustdoc.rs? @ehuss had some suggestions in https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Does.20cargo.20pass.20--sysroot.3F but they sounded a little tricky to implement.

jyn514 · 2020-10-01T05:18:28Z

Ok, this is actually working now :) Giant thank you to @ehuss for all the help with bootstrap!

jyn514 · 2020-10-01T05:25:23Z

The current error for --fix is

error[E0463]: can't find crate for `test`
 --> library/test/src/stats/tests.rs:3:1
  |
3 | extern crate test;
  | ^^^^^^^^^^^^^^^^^^ can't find crate

I figure that can be fixed in a follow-up, though.

Found while working on rust-lang#77351; these are just the ones that could be fixed automatically.

src/bootstrap/builder.rs

Mark-Simulacrum · 2020-10-01T11:22:38Z

src/bootstrap/builder.rs

+                    }
+                }) {
+                    eprintln!(
+                        "error: `x.py clippy` requires a nightly host `rustc` toolchain with the `clippy` component"


This seems like the wrong recommendation. Nightly clippy is not guaranteed to work; beta clippy is much more likely to.

Nightly Clippy is always available, since we use git subtree for Clippy. It may have a few more FPs, but generally it should work. Or am I misunderstanding something?

Nightly clippy is always available but may not work on the rustc tree -- it's usually outdated by several PRs (at most points in the day) which means that there's a pretty high likelihood of it not understanding some library feature or failing on stabilization etc. Beta clippy, like beta rustc, is something we're much more prepared for.

I was not able to get this to work with a beta clippy.

error[E0522]: definition of an unknown language item: `array` --> library/core/src/array/mod.rs:379:1 | 379 | #[lang = "array"] | ^^^^^^^^^^^^^^^^^ definition of unknown language item `array` ... various other E0522 errors ... error[E0118]: no base type found for inherent implementation --> library/core/src/array/mod.rs:380:25 | 380 | impl<T, const N: usize> [T; N] { | ^^^^^^ impl requires a base type error: aborting due to 20 previous errors; 53 warnings emitted

This is with cfg=bootstrap. I think it's related to not setting RUSTC somehow.

So, I am okay with us merging fixes to nightly clippy working, but I am unwilling to actually actively recommend nightly. I think that RUSTC getting set just needs to get fixed.

I tried setting RUSTC to no effect, I think because there's no CLIPPY analogue to RUSTC or RUSTFMT.

I'm not sure exactly what needs to happen to get beta clippy to work. Why does x.py check work with beta rustc when x.py clippy doesn't? I don't know what's going wrong.

This started working after the beta bump, although I'm not sure why ... maybe I had an old version of beta or something? Anyway, I tested with both nightly and beta and it seems to be working.

…-Simulacrum Fix some clippy lints Found while working on rust-lang#77351; these are just the ones that could be fixed automatically.

Clippy does its own runtime detection of the sysroot, which was incorrect in this case (it used the beta sysroot). This overrides the sysroot to use `stage0-sysroot` instead. - Get `x.py clippy` to work on nightly - Give a nice error message if nightly clippy isn't installed

jyn514 · 2020-10-27T13:47:19Z

--fix now works for everything except rustdoc. See https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/cargo.20check.20--no-tests.3F for why rustdoc is special. Note x.py clippy still works on everything, just not clippy --fix.

Fix some more clippy warnings Found while working on rust-lang#77351. It turns out that `x.py clippy --fix` does work on that branch as long as you pass `CARGOFLAGS=--lib`.

jyn514 · 2020-11-01T00:39:09Z

ping @Mark-Simulacrum

Mark-Simulacrum

I am still pretty unhappy with this additional code being added to fix this, but it's not in the critical complicated pieces of bootstrap sufficiently for me to block it.

r=me with comments resolved (they're both nit-ish so I don't feel strongly about re-reviewing unless you think that's necessary)

Mark-Simulacrum · 2020-11-05T20:41:45Z

src/bootstrap/builder.rs

+                let host_version = Command::new("rustc").arg("--version").output().map_err(|_| ());
+                let output = host_version.and_then(|output| {
+                    if output.status.success()
+                    {


This brace looks like it should be on the previous line (i.e., if ... {\n)

Mark-Simulacrum · 2020-11-05T20:44:14Z

src/bootstrap/builder.rs

@@ -850,7 +850,42 @@ impl<'a> Builder<'a> {
                cargo.args(s.split_whitespace());
            }
            rustflags.env("RUSTFLAGS_BOOTSTRAP");
-            rustflags.arg("--cfg=bootstrap");
+            if cmd == "clippy" {
+                // clippy overwrites any sysroot we pass on the command line.


I am confused, this sysroot is being passed on the command line? If that gets overwritten, then what's the point?

The comment is wrong - it gets overwritten if we pass it to cargo, not if we pass it to clippy.

https://github.com/rust-lang/rust-clippy/blob/a771557ee92af5e313714f127bb48a1d787a1cff/src/driver.rs#L310

Here's the error for rustdoc: ``` Checking rustdoc artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) error: no library targets found in package `rustdoc-tool` ```

jyn514 · 2020-11-05T23:10:15Z

@bors r+

bors · 2020-11-05T23:10:17Z

📌 Commit 8d2fa72 has been approved by jyn514

jyn514 · 2020-11-05T23:10:30Z

err

@bors r-

@bors r=Mark-Simulacrum

bors · 2020-11-05T23:10:32Z

📌 Commit 8d2fa72 has been approved by Mark-Simulacrum

…acrum Fix `x.py clippy` I don't think this ever worked. Fixes rust-lang#77309. `--fix` support is a work in progress, but works for a very small subset of `libtest`. This works by using the host `cargo-clippy` driver; it does not use `stage0.txt` at all. To mitigate confusion from this, it gives an error if you don't have `rustc +nightly` as the default rustc in `$PATH`. Additionally, it means that bootstrap can't set `RUSTC`; this makes it no longer possible for clippy to detect the sysroot itself. Instead, bootstrap passes the sysroot to cargo. r? `@ghost`

bors · 2020-11-06T11:31:28Z

⌛ Testing commit 8d2fa72 with merge dc06a36...

bors · 2020-11-06T13:58:24Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing dc06a36 to master...

jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 29, 2020

Mark-Simulacrum closed this Sep 30, 2020

Mark-Simulacrum reopened this Sep 30, 2020

jyn514 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 1, 2020

jyn514 changed the title ~~[WIP] fix x.py clippy~~ Fix x.py clippy Oct 1, 2020

jyn514 force-pushed the clippy-sysroot branch 2 times, most recently from a5e2fda to dccd80e Compare October 1, 2020 05:16

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2020

jyn514 force-pushed the clippy-sysroot branch from dccd80e to 8b6aaf6 Compare October 1, 2020 05:28

jyn514 added a commit to jyn514/rust that referenced this pull request Oct 1, 2020

Fix some clippy issues

8164218

Found while working on rust-lang#77351; these are just the ones that could be fixed automatically.

jyn514 mentioned this pull request Oct 1, 2020

Fix some clippy lints #77389

Merged

Mark-Simulacrum reviewed Oct 1, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

jyn514 force-pushed the clippy-sysroot branch from de67002 to d78ac2c Compare October 10, 2020 04:32

This comment has been minimized.

Sign in to view

jyn514 added 2 commits October 26, 2020 18:56

Add --fix support to x.py clippy

51f8076

jyn514 assigned Mark-Simulacrum Nov 1, 2020

Mark-Simulacrum reviewed Nov 5, 2020

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2020

Get --fix working for everything except rustdoc

8d2fa72

Here's the error for rustdoc: ``` Checking rustdoc artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu) error: no library targets found in package `rustdoc-tool` ```

jyn514 force-pushed the clippy-sysroot branch from 90d6853 to 8d2fa72 Compare November 5, 2020 23:06

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2020

Dylan-DPC-zz mentioned this pull request Nov 6, 2020

Rollup of 11 pull requests #78792

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2020

bors merged commit dc06a36 into rust-lang:master Nov 6, 2020

rustbot added this to the 1.49.0 milestone Nov 6, 2020

jyn514 deleted the clippy-sysroot branch November 6, 2020 14:05

jyn514 mentioned this pull request Mar 8, 2021

Add compiler-internal lints #49509

Open

12 tasks

jyn514 mentioned this pull request Apr 12, 2022

Fix x.py clippy to be less janky #95988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `x.py clippy` #77351

Fix `x.py clippy` #77351

jyn514 commented Sep 29, 2020 •

edited

Loading

jyn514 commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

jyn514 commented Sep 30, 2020

flip1995 commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

jyn514 commented Oct 1, 2020

jyn514 commented Oct 1, 2020

jyn514 commented Oct 1, 2020

Mark-Simulacrum Oct 1, 2020

flip1995 Oct 1, 2020

Mark-Simulacrum Oct 1, 2020

jyn514 Oct 1, 2020 •

edited

Loading

Mark-Simulacrum Oct 1, 2020

jyn514 Oct 1, 2020 •

edited

Loading

jyn514 Oct 10, 2020

This comment has been minimized.

This comment has been minimized.

jyn514 commented Oct 27, 2020

jyn514 commented Nov 1, 2020

Mark-Simulacrum left a comment

Mark-Simulacrum Nov 5, 2020

Mark-Simulacrum Nov 5, 2020

jyn514 Nov 5, 2020

jyn514 commented Nov 5, 2020

bors commented Nov 5, 2020

jyn514 commented Nov 5, 2020

bors commented Nov 5, 2020

bors commented Nov 6, 2020

bors commented Nov 6, 2020

Fix x.py clippy #77351

Fix x.py clippy #77351

Conversation

jyn514 commented Sep 29, 2020 • edited Loading

jyn514 commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

jyn514 commented Sep 30, 2020

flip1995 commented Sep 30, 2020

Mark-Simulacrum commented Sep 30, 2020

jyn514 commented Oct 1, 2020

jyn514 commented Oct 1, 2020

jyn514 commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Oct 27, 2020

jyn514 commented Nov 1, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Nov 5, 2020

bors commented Nov 5, 2020

jyn514 commented Nov 5, 2020

bors commented Nov 5, 2020

bors commented Nov 6, 2020

bors commented Nov 6, 2020

Fix `x.py clippy` #77351

Fix `x.py clippy` #77351

jyn514 commented Sep 29, 2020 •

edited

Loading

jyn514 Oct 1, 2020 •

edited

Loading

jyn514 Oct 1, 2020 •

edited

Loading