Skip to content

rustbuild: Cleanup global lint settings #62438

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 7, 2019
Merged

rustbuild: Cleanup global lint settings #62438

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b11757e
rustbuild: Cleanup global lint settings
petrochenkov Jul 6, 2019
36a5aa8
Address review comments
petrochenkov Jul 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 29 additions & 37 deletions src/bootstrap/bin/rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,16 @@ fn main() {
cmd.args(&args)
.env(bootstrap::util::dylib_path_var(),
env::join_paths(&dylib_path).unwrap());
let mut maybe_crate = None;

// Get the name of the crate we're compiling, if any.
let maybe_crate_name = args.windows(2)
.find(|a| &*a[0] == "--crate-name")
.map(|crate_name| &*crate_name[1]);
let crate_name = args.windows(2)
.find(|args| args[0] == "--crate-name")
.and_then(|args| args[1].to_str());

if let Some(current_crate) = maybe_crate_name {
if let Some(crate_name) = crate_name {
if let Some(target) = env::var_os("RUSTC_TIME") {
if target == "all" ||
target.into_string().unwrap().split(",").any(|c| c.trim() == current_crate)
target.into_string().unwrap().split(",").any(|c| c.trim() == crate_name)
{
cmd.arg("-Ztime");
}
Expand All @@ -125,6 +124,17 @@ fn main() {
cmd.arg(format!("-Cdebuginfo={}", debuginfo_level));
}

if env::var_os("RUSTC_DENY_WARNINGS").is_some() &&
env::var_os("RUSTC_EXTERNAL_TOOL").is_none() {
cmd.arg("-Dwarnings");
cmd.arg("-Drust_2018_idioms");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we maybe lost bare_trait_objects here? Or is that included in rust_2018_idioms (it should be, I'd guess)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a part of the edition idiom group.
(And it's a part of -Dwarnings too now because it was turned into warn-by-default on both editions.)

if stage != "0" && crate_name != Some("rustc_version") && // cfg(not(bootstrap))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the cfg comment ontop of the if and expand it a bit, something like "Internal lints may change, so don't enable them on the bootstrap compiler. Also, only enable them for internal crates."

I think rustc_version is just excluded because it's actually external but happens to fall into the use_internal_crates check, could it be moved into that function and a comment added saying "this is actually an external crate"?

Copy link
Contributor Author

@petrochenkov petrochenkov Jul 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal lints may change, so don't enable them on the bootstrap compiler

The reason is simpler - stage 0 compiler doesn't understand -Drustc::internal since it's too new for it and gives an error.

rustc_version for some reason reports stage == "1" when it's actually built by a stage 0 compiler that errors on -Drustc::internal (this happens during x.py doc, IIRC), it doesn't have issues with the lints themselves.
So it's also a part of the bootstrap problem and we'll be able to remove this exception during the next bootstrap compiler update, that's why it's not in use_internal_crates.
(There are other "false-positive" rustc_* crates which happen to pass the internal lint checks and therefore not listed as exceptions.)

I'll add a comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect the stage 0 compiler to understand -Drustc::internal now, since we just bumped beta -- should we remove that check then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think I tried it and it didn't work.
Does stage 0 include #61545 (merged yesterday)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, that's ~one day too recent :)

use_internal_lints(crate_name) {
cmd.arg("-Zunstable-options");
cmd.arg("-Drustc::internal");
}
}

if let Some(target) = target {
// The stage0 compiler has a special sysroot distinct from what we
// actually downloaded, so we just always pass the `--sysroot` option.
Expand Down Expand Up @@ -167,9 +177,6 @@ fn main() {
cmd.arg(format!("-Clinker={}", target_linker));
}

let crate_name = maybe_crate_name.unwrap();
maybe_crate = Some(crate_name);

// If we're compiling specifically the `panic_abort` crate then we pass
// the `-C panic=abort` option. Note that we do not do this for any
// other crate intentionally as this is the only crate for now that we
Expand All @@ -182,8 +189,8 @@ fn main() {
// `compiler_builtins` are unconditionally compiled with panic=abort to
// workaround undefined references to `rust_eh_unwind_resume` generated
// otherwise, see issue https://github.com/rust-lang/rust/issues/43095.
if crate_name == "panic_abort" ||
crate_name == "compiler_builtins" && stage != "0" {
if crate_name == Some("panic_abort") ||
crate_name == Some("compiler_builtins") && stage != "0" {
cmd.arg("-C").arg("panic=abort");
}

Expand All @@ -196,7 +203,7 @@ fn main() {

// The compiler builtins are pretty sensitive to symbols referenced in
// libcore and such, so we never compile them with debug assertions.
if crate_name == "compiler_builtins" {
if crate_name == Some("compiler_builtins") {
cmd.arg("-C").arg("debug-assertions=no");
} else {
cmd.arg("-C").arg(format!("debug-assertions={}", debug_assertions));
Expand Down Expand Up @@ -305,22 +312,6 @@ fn main() {
}
}

// This is required for internal lints.
if let Some(crate_name) = args.windows(2).find(|a| &*a[0] == "--crate-name") {
let crate_name = crate_name[1].to_string_lossy();
if crate_name != "rustc_version"
&& (crate_name.starts_with("rustc")
|| crate_name.starts_with("syntax")
|| crate_name == "arena"
|| crate_name == "fmt_macros")
{
cmd.arg("-Zunstable-options");
if stage != "0" {
cmd.arg("-Wrustc::internal");
}
}
}

// Force all crates compiled by this compiler to (a) be unstable and (b)
// allow the `rustc_private` feature to link to other unstable crates
// also in the sysroot. We also do this for host crates, since those
Expand All @@ -333,13 +324,6 @@ fn main() {
cmd.arg("--cfg").arg("parallel_compiler");
}

if env::var_os("RUSTC_DENY_WARNINGS").is_some() && env::var_os("RUSTC_EXTERNAL_TOOL").is_none()
{
cmd.arg("-Dwarnings");
cmd.arg("-Dbare_trait_objects");
cmd.arg("-Drust_2018_idioms");
}

if verbose > 1 {
eprintln!(
"rustc command: {:?}={:?} {:?}",
Expand All @@ -362,7 +346,7 @@ fn main() {
}

if env::var_os("RUSTC_PRINT_STEP_TIMINGS").is_some() {
if let Some(krate) = maybe_crate {
if let Some(crate_name) = crate_name {
let start = Instant::now();
let status = cmd
.status()
Expand All @@ -371,7 +355,7 @@ fn main() {

let is_test = args.iter().any(|a| a == "--test");
eprintln!("[RUSTC-TIMING] {} test:{} {}.{:03}",
krate.to_string_lossy(),
crate_name,
is_test,
dur.as_secs(),
dur.subsec_nanos() / 1_000_000);
Expand All @@ -390,6 +374,14 @@ fn main() {
std::process::exit(code);
}

// Rustc crates for which internal lints are in effect.
fn use_internal_lints(crate_name: Option<&str>) -> bool {
crate_name.map_or(false, |crate_name| {
crate_name.starts_with("rustc") || crate_name.starts_with("syntax") ||
["arena", "fmt_macros"].contains(&crate_name)
})
}

#[cfg(unix)]
fn exec_cmd(cmd: &mut Command) -> io::Result<i32> {
use std::os::unix::process::CommandExt;
Expand Down