Auto merge of rust-lang#117595 - jyn514:x-clippy, r=albertlarsan68

x clippy

thanks to `@asquared31415` `@albertlarsan68` for all their help, most of this pr is their work

note that this also adds x clippy --stage 0 -Awarnings to x86_64-gnu-llvm-15 to make sure it stays working; that won't gate on any clippy warnings, just enforce that clippy doesn't give a hard error. we can't add --stage 1 until clippy fixes its debug assertions not to panic.

note that `x clippy --stage 1` currently breaks when combined with download-rustc.

unlike the previous prs, this doesn't require changes to clippy (it works by using RUSTC_WRAPPER instead), and supports stage 0

read this commit-by-commit

closes rust-lang#107628; see also rust-lang#106394, rust-lang#97443. fixes rust-lang#95988. helps with rust-lang#76495.

r? bootstrap

src/bootstrap/bootstrap.py

@@ -616,22 +616,6 @@ def download_toolchain(self):
             with output(self.rustc_stamp()) as rust_stamp:
                 rust_stamp.write(key)
 
-    def _download_component_helper(
-        self, filename, pattern, tarball_suffix, rustc_cache,
-    ):
-        key = self.stage0_compiler.date
-
-        tarball = os.path.join(rustc_cache, filename)
-        if not os.path.exists(tarball):
-            get(
-                self.download_url,
-                "dist/{}/{}".format(key, filename),
-                tarball,
-                self.checksums_sha256,
-                verbose=self.verbose,
-            )
-        unpack(tarball, tarball_suffix, self.bin_root(), match=pattern, verbose=self.verbose)
-
     def should_fix_bins_and_dylibs(self):
         """Whether or not `fix_bin_or_dylib` needs to be run; can only be True
         on NixOS or if config.toml has `build.patch-binaries-for-nix` set.

src/bootstrap/src/bin/rustc.rs

@@ -20,7 +20,7 @@ use std::path::PathBuf;
 use std::process::{Child, Command};
 use std::time::Instant;
 
-use dylib_util::{dylib_path, dylib_path_var};
+use dylib_util::{dylib_path, dylib_path_var, exe};
 
 #[path = "../utils/bin_helpers.rs"]
 mod bin_helpers;
@@ -29,8 +29,10 @@ mod bin_helpers;
 mod dylib_util;
 
 fn main() {
-    let args = env::args_os().skip(1).collect::<Vec<_>>();
-    let arg = |name| args.windows(2).find(|args| args[0] == name).and_then(|args| args[1].to_str());
+    let orig_args = env::args_os().skip(1).collect::<Vec<_>>();
+    let mut args = orig_args.clone();
+    let arg =
+        |name| orig_args.windows(2).find(|args| args[0] == name).and_then(|args| args[1].to_str());
 
     // We don't use the stage in this shim, but let's parse it to make sure that we're invoked
     // by bootstrap, or that we provide a helpful error message if not.
@@ -47,7 +49,8 @@ fn main() {
     // determine the version of the compiler, the real compiler needs to be
     // used. Currently, these two states are differentiated based on whether
     // --target and -vV is/isn't passed.
-    let (rustc, libdir) = if target.is_none() && version.is_none() {
+    let is_build_script = target.is_none() && version.is_none();
+    let (rustc, libdir) = if is_build_script {
         ("RUSTC_SNAPSHOT", "RUSTC_SNAPSHOT_LIBDIR")
     } else {
         ("RUSTC_REAL", "RUSTC_LIBDIR")
@@ -56,12 +59,45 @@ fn main() {
     let sysroot = env::var_os("RUSTC_SYSROOT").expect("RUSTC_SYSROOT was not set");
     let on_fail = env::var_os("RUSTC_ON_FAIL").map(Command::new);
 
-    let rustc = env::var_os(rustc).unwrap_or_else(|| panic!("{:?} was not set", rustc));
+    let rustc_real = env::var_os(rustc).unwrap_or_else(|| panic!("{:?} was not set", rustc));
     let libdir = env::var_os(libdir).unwrap_or_else(|| panic!("{:?} was not set", libdir));
     let mut dylib_path = dylib_path();
     dylib_path.insert(0, PathBuf::from(&libdir));
 
-    let mut cmd = Command::new(rustc);
+    // if we're running clippy, trust cargo-clippy to set clippy-driver appropriately (and don't override it with rustc).
+    // otherwise, substitute whatever cargo thinks rustc should be with RUSTC_REAL.
+    // NOTE: this means we ignore RUSTC in the environment.
+    // FIXME: We might want to consider removing RUSTC_REAL and setting RUSTC directly?
+    let target_name = target
+        .map(|s| s.to_owned())
+        .unwrap_or_else(|| env::var("CFG_COMPILER_HOST_TRIPLE").unwrap());
+    let is_clippy = args[0].to_string_lossy().ends_with(&exe("clippy-driver", &target_name));
+    let rustc_driver = if is_clippy {
+        if is_build_script {
+            // Don't run clippy on build scripts (for one thing, we may not have libstd built with
+            // the appropriate version yet, e.g. for stage 1 std).
+            // Also remove the `clippy-driver` param in addition to the RUSTC param.
+            args.drain(..2);
+            rustc_real
+        } else {
+            args.remove(0)
+        }
+    } else {
+        // Cargo doesn't respect RUSTC_WRAPPER for version information >:(
+        // don't remove the first arg if we're being run as RUSTC instead of RUSTC_WRAPPER.
+        if args[0] == env::current_exe().expect("couldn't get path to rustc shim") {
+            args.remove(0);
+        }
+        rustc_real
+    };
+
+    let mut cmd = if let Some(wrapper) = env::var_os("RUSTC_WRAPPER_REAL") {
+        let mut cmd = Command::new(wrapper);
+        cmd.arg(rustc_driver);
+        cmd
+    } else {
+        Command::new(rustc_driver)
+    };
     cmd.args(&args).env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());
 
     // Get the name of the crate we're compiling, if any.

src/bootstrap/src/core/build_steps/compile.rs

@@ -1800,10 +1800,6 @@ pub fn run_cargo(
     is_check: bool,
     rlib_only_metadata: bool,
 ) -> Vec<PathBuf> {
-    if builder.config.dry_run() {
-        return Vec::new();
-    }
-
     // `target_root_dir` looks like $dir/$target/release
     let target_root_dir = stamp.parent().unwrap();
     // `target_deps_dir` looks like $dir/$target/release/deps
@@ -1910,6 +1906,10 @@ pub fn run_cargo(
         crate::exit!(1);
     }
 
+    if builder.config.dry_run() {
+        return Vec::new();
+    }
+
     // Ok now we need to actually find all the files listed in `toplevel`. We've
     // got a list of prefix/extensions and we basically just need to find the
     // most recent file in the `deps` folder corresponding to each one.
@@ -1965,9 +1965,6 @@ pub fn stream_cargo(
     cb: &mut dyn FnMut(CargoMessage<'_>),
 ) -> bool {
     let mut cargo = Command::from(cargo);
-    if builder.config.dry_run() {
-        return true;
-    }
     // Instruct Cargo to give us json messages on stdout, critically leaving
     // stderr as piped so we can get those pretty colors.
     let mut message_format = if builder.config.json_output {
@@ -1986,6 +1983,11 @@ pub fn stream_cargo(
     }
 
     builder.verbose(&format!("running: {cargo:?}"));
+
+    if builder.config.dry_run() {
+        return true;
+    }
+
     let mut child = match cargo.spawn() {
         Ok(child) => child,
         Err(e) => panic!("failed to execute command: {cargo:?}\nERROR: {e}"),

src/bootstrap/src/core/builder.rs

@@ -18,7 +18,9 @@ use crate::core::build_steps::{check, clean, compile, dist, doc, install, run, s
 use crate::core::config::flags::{Color, Subcommand};
 use crate::core::config::{DryRun, SplitDebuginfo, TargetSelection};
 use crate::utils::cache::{Cache, Interned, INTERNER};
-use crate::utils::helpers::{self, add_dylib_path, add_link_lib_path, exe, libdir, output, t};
+use crate::utils::helpers::{
+    self, add_dylib_path, add_link_lib_path, dylib_path, dylib_path_var, exe, libdir, output, t,
+};
 use crate::Crate;
 use crate::EXTRA_CHECK_CFGS;
 use crate::{Build, CLang, DocTests, GitRepo, Mode};
@@ -1151,6 +1153,43 @@ impl<'a> Builder<'a> {
         self.ensure(tool::Rustdoc { compiler })
     }
 
+    pub fn cargo_clippy_cmd(&self, run_compiler: Compiler) -> Command {
+        let initial_sysroot_bin = self.initial_rustc.parent().unwrap();
+        // Set PATH to include the sysroot bin dir so clippy can find cargo.
+        let path = t!(env::join_paths(
+            // The sysroot comes first in PATH to avoid using rustup's cargo.
+            std::iter::once(PathBuf::from(initial_sysroot_bin))
+                .chain(env::split_paths(&t!(env::var("PATH"))))
+        ));
+
+        if run_compiler.stage == 0 {
+            // `ensure(Clippy { stage: 0 })` *builds* clippy with stage0, it doesn't use the beta clippy.
+            let cargo_clippy = self.build.config.download_clippy();
+            let mut cmd = Command::new(cargo_clippy);
+            cmd.env("PATH", &path);
+            return cmd;
+        }
+
+        let build_compiler = self.compiler(run_compiler.stage - 1, self.build.build);
+        self.ensure(tool::Clippy {
+            compiler: build_compiler,
+            target: self.build.build,
+            extra_features: vec![],
+        });
+        let cargo_clippy = self.ensure(tool::CargoClippy {
+            compiler: build_compiler,
+            target: self.build.build,
+            extra_features: vec![],
+        });
+        let mut dylib_path = dylib_path();
+        dylib_path.insert(0, self.sysroot(run_compiler).join("lib"));
+
+        let mut cmd = Command::new(cargo_clippy.unwrap());
+        cmd.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());
+        cmd.env("PATH", path);
+        cmd
+    }
+
     pub fn rustdoc_cmd(&self, compiler: Compiler) -> Command {
         let mut cmd = Command::new(&self.bootstrap_out.join("rustdoc"));
         cmd.env("RUSTC_STAGE", compiler.stage.to_string())
@@ -1201,7 +1240,12 @@ impl<'a> Builder<'a> {
         target: TargetSelection,
         cmd: &str,
     ) -> Command {
-        let mut cargo = Command::new(&self.initial_cargo);
+        let mut cargo = if cmd == "clippy" {
+            self.cargo_clippy_cmd(compiler)
+        } else {
+            Command::new(&self.initial_cargo)
+        };
+
         // Run cargo from the source root so it can find .cargo/config.
         // This matters when using vendoring and the working directory is outside the repository.
         cargo.current_dir(&self.src);
@@ -1325,6 +1369,23 @@ impl<'a> Builder<'a> {
             compiler.stage
         };
 
+        // We synthetically interpret a stage0 compiler used to build tools as a
+        // "raw" compiler in that it's the exact snapshot we download. Normally
+        // the stage0 build means it uses libraries build by the stage0
+        // compiler, but for tools we just use the precompiled libraries that
+        // we've downloaded
+        let use_snapshot = mode == Mode::ToolBootstrap;
+        assert!(!use_snapshot || stage == 0 || self.local_rebuild);
+
+        let maybe_sysroot = self.sysroot(compiler);
+        let sysroot = if use_snapshot { self.rustc_snapshot_sysroot() } else { &maybe_sysroot };
+        let libdir = self.rustc_libdir(compiler);
+
+        let sysroot_str = sysroot.as_os_str().to_str().expect("sysroot should be UTF-8");
+        if !matches!(self.config.dry_run, DryRun::SelfCheck) {
+            self.verbose_than(0, &format!("using sysroot {sysroot_str}"));
+        }
+
         let mut rustflags = Rustflags::new(target);
         if stage != 0 {
             if let Ok(s) = env::var("CARGOFLAGS_NOT_BOOTSTRAP") {
@@ -1336,41 +1397,16 @@ impl<'a> Builder<'a> {
                 cargo.args(s.split_whitespace());
             }
             rustflags.env("RUSTFLAGS_BOOTSTRAP");
-            if cmd == "clippy" {
-                // clippy overwrites sysroot if we pass it to cargo.
-                // Pass it directly to clippy instead.
-                // NOTE: this can't be fixed in clippy because we explicitly don't set `RUSTC`,
-                // so it has no way of knowing the sysroot.
-                rustflags.arg("--sysroot");
-                rustflags.arg(
-                    self.sysroot(compiler)
-                        .as_os_str()
-                        .to_str()
-                        .expect("sysroot must be valid UTF-8"),
-                );
-                // Only run clippy on a very limited subset of crates (in particular, not build scripts).
-                cargo.arg("-Zunstable-options");
-                // Explicitly does *not* set `--cfg=bootstrap`, since we're using a nightly clippy.
-                let host_version = Command::new("rustc").arg("--version").output().map_err(|_| ());
-                let output = host_version.and_then(|output| {
-                    if output.status.success() {
-                        Ok(output)
-                    } else {
-                        Err(())
-                    }
-                }).unwrap_or_else(|_| {
-                    eprintln!(
-                        "ERROR: `x.py clippy` requires a host `rustc` toolchain with the `clippy` component"
-                    );
-                    eprintln!("HELP: try `rustup component add clippy`");
-                    crate::exit!(1);
-                });
-                if !t!(std::str::from_utf8(&output.stdout)).contains("nightly") {
-                    rustflags.arg("--cfg=bootstrap");
-                }
-            } else {
-                rustflags.arg("--cfg=bootstrap");
-            }
+            rustflags.arg("--cfg=bootstrap");
+        }
+
+        if cmd == "clippy" {
+            // clippy overwrites sysroot if we pass it to cargo.
+            // Pass it directly to clippy instead.
+            // NOTE: this can't be fixed in clippy because we explicitly don't set `RUSTC`,
+            // so it has no way of knowing the sysroot.
+            rustflags.arg("--sysroot");
+            rustflags.arg(sysroot_str);
         }
 
         let use_new_symbol_mangling = match self.config.rust_new_symbol_mangling {
@@ -1565,18 +1601,6 @@ impl<'a> Builder<'a> {
 
         let want_rustdoc = self.doc_tests != DocTests::No;
 
-        // We synthetically interpret a stage0 compiler used to build tools as a
-        // "raw" compiler in that it's the exact snapshot we download. Normally
-        // the stage0 build means it uses libraries build by the stage0
-        // compiler, but for tools we just use the precompiled libraries that
-        // we've downloaded
-        let use_snapshot = mode == Mode::ToolBootstrap;
-        assert!(!use_snapshot || stage == 0 || self.local_rebuild);
-
-        let maybe_sysroot = self.sysroot(compiler);
-        let sysroot = if use_snapshot { self.rustc_snapshot_sysroot() } else { &maybe_sysroot };
-        let libdir = self.rustc_libdir(compiler);
-
         // Clear the output directory if the real rustc we're using has changed;
         // Cargo cannot detect this as it thinks rustc is bootstrap/debug/rustc.
         //
@@ -1612,10 +1636,19 @@ impl<'a> Builder<'a> {
             )
             .env("RUSTC_ERROR_METADATA_DST", self.extended_error_dir())
             .env("RUSTC_BREAK_ON_ICE", "1");
-        // Clippy support is a hack and uses the default `cargo-clippy` in path.
-        // Don't override RUSTC so that the `cargo-clippy` in path will be run.
-        if cmd != "clippy" {
-            cargo.env("RUSTC", self.bootstrap_out.join("rustc"));
+
+        // Set RUSTC_WRAPPER to the bootstrap shim, which switches between beta and in-tree
+        // sysroot depending on whether we're building build scripts.
+        // NOTE: we intentionally use RUSTC_WRAPPER so that we can support clippy - RUSTC is not
+        // respected by clippy-driver; RUSTC_WRAPPER happens earlier, before clippy runs.
+        cargo.env("RUSTC_WRAPPER", self.bootstrap_out.join("rustc"));
+        // NOTE: we also need to set RUSTC so cargo can run `rustc -vV`; apparently that ignores RUSTC_WRAPPER >:(
+        cargo.env("RUSTC", self.bootstrap_out.join("rustc"));
+
+        // Someone might have set some previous rustc wrapper (e.g.
+        // sccache) before bootstrap overrode it. Respect that variable.
+        if let Some(existing_wrapper) = env::var_os("RUSTC_WRAPPER") {
+            cargo.env("RUSTC_WRAPPER_REAL", existing_wrapper);
         }
 
         // Dealing with rpath here is a little special, so let's go into some

src/bootstrap/src/core/download.rs

@@ -208,7 +208,10 @@ impl Config {
             Some(other) => panic!("unsupported protocol {other} in {url}"),
             None => panic!("no protocol in {url}"),
         }
-        t!(std::fs::rename(&tempfile, dest_path));
+        t!(
+            std::fs::rename(&tempfile, dest_path),
+            format!("failed to rename {tempfile:?} to {dest_path:?}")
+        );
     }
 
     fn download_http_with_retries(&self, tempfile: &Path, url: &str, help_on_error: &str) {
@@ -375,6 +378,32 @@ enum DownloadSource {
 
 /// Functions that are only ever called once, but named for clarify and to avoid thousand-line functions.
 impl Config {
+    pub(crate) fn download_clippy(&self) -> PathBuf {
+        self.verbose("downloading stage0 clippy artifacts");
+
+        let date = &self.stage0_metadata.compiler.date;
+        let version = &self.stage0_metadata.compiler.version;
+        let host = self.build;
+
+        let bin_root = self.out.join(host.triple).join("stage0");
+        let clippy_stamp = bin_root.join(".clippy-stamp");
+        let cargo_clippy = bin_root.join("bin").join(exe("cargo-clippy", host));
+        if cargo_clippy.exists() && !program_out_of_date(&clippy_stamp, &date) {
+            return cargo_clippy;
+        }
+
+        let filename = format!("clippy-{version}-{host}.tar.xz");
+        self.download_component(DownloadSource::Dist, filename, "clippy-preview", date, "stage0");
+        if self.should_fix_bins_and_dylibs() {
+            self.fix_bin_or_dylib(&cargo_clippy);
+            self.fix_bin_or_dylib(&cargo_clippy.with_file_name(exe("clippy-driver", host)));
+        }
+
+        cargo_clippy
+    }
+
+    /// NOTE: rustfmt is a completely different toolchain than the bootstrap compiler, so it can't
+    /// reuse target directories or artifacts
     pub(crate) fn maybe_download_rustfmt(&self) -> Option<PathBuf> {
         let RustfmtMetadata { date, version } = self.stage0_metadata.rustfmt.as_ref()?;
         let channel = format!("{version}-{date}");
@@ -544,6 +573,10 @@ impl Config {
         key: &str,
         destination: &str,
     ) {
+        if self.dry_run() {
+            return;
+        }
+
         let cache_dst = self.out.join("cache");
         let cache_dir = cache_dst.join(key);
         if !cache_dir.exists() {