Skip to content

Commit

Permalink
Rollup merge of rust-lang#74046 - ehuss:deny-warnings-caching, r=Mark…
Browse files Browse the repository at this point in the history
…-Simulacrum

Fix caching issue when building tools.

This fixes a problem with tool builds not being cached properly.

rust-lang#73297 changed it so that Clippy will participate in the "deny warnings" setting. Unfortunately this causes a problem because Clippy shares the build directory with other tools which do not participate in "deny warnings".  Because Cargo does not independently cache artifacts based on different RUSTFLAGS settings, it causes all the shared dependencies to get rebuilt if Clippy ever gets built.

The solution here is to stop using RUSTFLAGS, and just sneak the settings in through the rustc wrapper. Cargo won't know about the different settings, so it will not bust the cache. This should be safe since lint settings on dependencies are ignored. This is how things used to work in the past before rust-lang#64316.

Alternate solutions:
* Treat Clippy as a "submodule" and don't enforce warnings on it. This was the behavior before rust-lang#73297. The consequence is that if a warning sneaks into clippy, that the clippy maintainers will need to fix it when they sync clippy back to the clippy repo.
* Just deny warnings on all tools (removing the in-tree/submodule distinction). This is tempting, but with some issues (cc rust-lang#52336):
  * Adding or changing warnings in rustc can be difficult to land because tools have to be updated if they trip the warning. In practice, this isn't too bad.  Cargo (and rustfmt) already runs with `deny(warnings)`, so this has been the de-facto standard already (although they do not use the extra lints like `unused_lifetimes`).
* Teach Cargo to add flags to the workspace members, but not dependencies.
* Teach Cargo to add flags without fingerprinting them?
* Teach Cargo to independently cache different RUSTFLAGS artifacts (this was [reverted](rust-lang/cargo#7417) due to complications). This would also unnecessarily rebuild dependencies, but would avoid cache thrashing.
* Teach Cargo about lint settings.

Closes rust-lang#74016
  • Loading branch information
Manishearth authored Jul 14, 2020
2 parents d9614db + 310c97b commit e553243
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 4 deletions.
4 changes: 4 additions & 0 deletions src/bootstrap/bin/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ fn main() {
cmd.env("RUST_BACKTRACE", "1");
}

if let Ok(lint_flags) = env::var("RUSTC_LINT_FLAGS") {
cmd.args(lint_flags.split_whitespace());
}

if target.is_some() {
// The stage0 compiler has a special sysroot distinct from what we
// actually downloaded, so we just always pass the `--sysroot` option,
Expand Down
18 changes: 14 additions & 4 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1130,22 +1130,32 @@ impl<'a> Builder<'a> {
cargo.env("RUSTC_VERBOSE", self.verbosity.to_string());

if source_type == SourceType::InTree {
let mut lint_flags = Vec::new();
// When extending this list, add the new lints to the RUSTFLAGS of the
// build_bootstrap function of src/bootstrap/bootstrap.py as well as
// some code doesn't go through this `rustc` wrapper.
rustflags.arg("-Wrust_2018_idioms");
rustflags.arg("-Wunused_lifetimes");
lint_flags.push("-Wrust_2018_idioms");
lint_flags.push("-Wunused_lifetimes");

if self.config.deny_warnings {
rustflags.arg("-Dwarnings");
lint_flags.push("-Dwarnings");
}

// FIXME(#58633) hide "unused attribute" errors in incremental
// builds of the standard library, as the underlying checks are
// not yet properly integrated with incremental recompilation.
if mode == Mode::Std && compiler.stage == 0 && self.config.incremental {
rustflags.arg("-Aunused-attributes");
lint_flags.push("-Aunused-attributes");
}
// This does not use RUSTFLAGS due to caching issues with Cargo.
// Clippy is treated as an "in tree" tool, but shares the same
// cache as other "submodule" tools. With these options set in
// RUSTFLAGS, that causes *every* shared dependency to be rebuilt.
// By injecting this into the rustc wrapper, this circumvents
// Cargo's fingerprint detection. This is fine because lint flags
// are always ignored in dependencies. Eventually this should be
// fixed via better support from Cargo.
cargo.env("RUSTC_LINT_FLAGS", lint_flags.join(" "));
}

if let Mode::Rustc | Mode::Codegen = mode {
Expand Down

0 comments on commit e553243

Please sign in to comment.