Skip to content

Commit

Permalink
Auto merge of rust-lang#117595 - jyn514:x-clippy, r=albertlarsan68
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bors
bors committed Dec 16, 2023
2 parents 02ad667 + a078c3a commit 4451777
Show file tree
Hide file tree
Showing 10 changed files with 245 additions and 93 deletions.
16 changes: 0 additions & 16 deletions src/bootstrap/bootstrap.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
52 changes: 45 additions & 7 deletions src/bootstrap/src/bin/rustc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
//! never get replaced.

use std::env;
use std::path::PathBuf;
use std::path::{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;
Expand All @@ -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());

let stage = bin_helpers::parse_rustc_stage();
let verbose = bin_helpers::parse_rustc_verbose();
Expand All @@ -45,7 +47,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")
Expand All @@ -54,12 +57,47 @@ 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?
// NOTE: we intentionally pass the name of the host, not the target.
let host = env::var("CFG_COMPILER_BUILD_TRIPLE").unwrap();
let is_clippy = args[0].to_string_lossy().ends_with(&exe("clippy-driver", &host));
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.
// Cargo also sometimes doesn't pass the `.exe` suffix on Windows - add it manually.
let current_exe = env::current_exe().expect("couldn't get path to rustc shim");
let arg0 = exe(args[0].to_str().expect("only utf8 paths are supported"), &host);
if Path::new(&arg0) == current_exe {
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.
Expand Down
16 changes: 9 additions & 7 deletions src/bootstrap/src/core/build_steps/compile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1809,10 +1809,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
Expand Down Expand Up @@ -1919,6 +1915,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.
Expand Down Expand Up @@ -1974,9 +1974,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 {
Expand All @@ -1995,6 +1992,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}"),
Expand Down
140 changes: 88 additions & 52 deletions src/bootstrap/src/core/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,44 @@ 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.
// FIXME: once rust-clippy#11944 lands on beta, set `CARGO` directly instead.
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 = helpers::dylib_path();
dylib_path.insert(0, self.sysroot(run_compiler).join("lib"));

let mut cmd = Command::new(cargo_clippy.unwrap());
cmd.env(helpers::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())
Expand Down Expand Up @@ -1200,7 +1238,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);
Expand Down Expand Up @@ -1324,6 +1367,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") {
Expand All @@ -1335,41 +1395,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 {
Expand Down Expand Up @@ -1564,18 +1599,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.
//
Expand Down Expand Up @@ -1611,10 +1634,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
Expand Down Expand Up @@ -1991,7 +2023,11 @@ impl<'a> Builder<'a> {
// Environment variables *required* throughout the build
//
// FIXME: should update code to not require this env var

// The host this new compiler will *run* on.
cargo.env("CFG_COMPILER_HOST_TRIPLE", target.triple);
// The host this new compiler is being *built* on.
cargo.env("CFG_COMPILER_BUILD_TRIPLE", compiler.host.triple);

// Set this for all builds to make sure doc builds also get it.
cargo.env("CFG_RELEASE_CHANNEL", &self.config.channel);
Expand Down
35 changes: 34 additions & 1 deletion src/bootstrap/src/core/download.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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}");
Expand Down Expand Up @@ -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() {
Expand Down
Loading

0 comments on commit 4451777

Please sign in to comment.