Delete most of `src/bootstrap/bin/rustc.rs` #64316

alexcrichton · 2019-09-09T17:38:00Z

This commit is an attempt at deleting as much of the rustc.rs shim that we have in rustbuild as possible. This shim predates RUSTFLAGS and is as old as rustbuild itself. While useful for quick hacks, it subverts Cargo's knowledge of rustc, makes it more difficult to build crates out of rustbuild, and is generally a hazard/code smell due to its architecture.

Additionally since the inception of this script we've added a number of features to Cargo such as profile overrides and RUSTFLAGS. This commit attempts to use these features of Cargo as much as possible to delete almost all of src/bootstrap/bin/rustc.rs. It's hoped that all new configuration for the Rust compiler can be codified in rustbuild rather than in this shim, allowing Cargo to have more knowledge about what's going on and making it a bit easier to reproduce builds outside of Cargo itself.

This was primarily motivated by some recent work on std-aware Cargo, and is also generally a cleanup of the script itself. This internally resulted in a number of refactorings of rustbuild itself, and the commits should be readable one-at-a-time instead of having to digest them all at once.

rust-highfive · 2019-09-09T17:38:10Z

⚠️ Warning ⚠️

These commits modify submodules.

alexcrichton · 2019-09-09T17:38:19Z

r? @Mark-Simulacrum

Mark-Simulacrum

Overall looks excellent, left a few comments here and there though.

Edit: Looks like GH has deemed some of these outdated but I think not all of them are, I've resolved some of the comments below if they turned out to be replaced by a later one

src/bootstrap/builder.rs

src/bootstrap/bin/rustc.rs

src/bootstrap/builder.rs

Mark-Simulacrum · 2019-09-09T18:45:34Z

src/bootstrap/builder.rs

+            .arg("-Zconfig-profile");
+
+        let profile_var = |name: &str| {
+            let profile = if self.config.rust_optimize {


unrelated note: we should probably drop rust_optimize as a config flag entirely since there's no real point in a non-optimized build I imagine

src/bootstrap/compile.rs

src/bootstrap/builder.rs

Mark-Simulacrum · 2019-09-16T18:18:52Z

@alexcrichton I think this is ready to go now, unless I'm missing something? I've been waiting for a flip to S-waiting-on-review to do another pass if you think it's warranted.

alexcrichton · 2019-09-16T19:00:47Z

@Mark-Simulacrum sorry, yes, this should be good to go

Mark-Simulacrum · 2019-09-16T19:54:12Z

src/libserialize/tests/json.rs

@@ -1,3 +1,5 @@
+#![allow(rustc::default_hash_types)]


Could we switch these to rustc::internal like we do for libserialize/lib.rs?

Mark-Simulacrum · 2019-09-16T19:55:35Z

Okay, r=me with last nit fixed (and looks like a rebase)

Mark-Simulacrum · 2019-09-16T19:55:57Z

Let's also do

@bors rollup=never

alexcrichton · 2019-09-16T21:01:36Z

@bors: r=Mark-Simulacrum

alexcrichton · 2019-09-23T16:36:47Z

@bors: r=Mark-Simulacrum

bors · 2019-09-23T16:36:51Z

📌 Commit 1a8897f has been approved by Mark-Simulacrum

bors · 2019-09-24T06:41:23Z

⌛ Testing commit 1a8897f with merge 7fdea7a...

Delete most of `src/bootstrap/bin/rustc.rs` This commit is an attempt at deleting as much of the `rustc.rs` shim that we have in rustbuild as possible. This shim predates `RUSTFLAGS` and is as old as rustbuild itself. While useful for quick hacks, it subverts Cargo's knowledge of `rustc`, makes it more difficult to build crates out of rustbuild, and is generally a hazard/code smell due to its architecture. Additionally since the inception of this script we've added a number of features to Cargo such as profile overrides and `RUSTFLAGS`. This commit attempts to use these features of Cargo as much as possible to delete almost all of `src/bootstrap/bin/rustc.rs`. It's hoped that all new configuration for the Rust compiler can be codified in rustbuild rather than in this shim, allowing Cargo to have more knowledge about what's going on and making it a bit easier to reproduce builds outside of Cargo itself. This was primarily motivated by some recent work on std-aware Cargo, and is also generally a cleanup of the script itself. This internally resulted in a number of refactorings of rustbuild itself, and the commits should be readable one-at-a-time instead of having to digest them all at once.

bors · 2019-09-24T10:41:23Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 7fdea7a to master...

Support configurable deny-warnings for all in-tree crates. This removes the hard-coded `deny(warnings)` on all in-tree tools, and allows it to be configured from the config. This is just a personal preference, as I find `deny(warnings)` frustrating during development or doing small tests. This also fixes some regressions in terms of warning handling. Warnings used to be dependent on `SourceType`, but in rust-lang#64316 it was changed to be based on `Mode`. This means tools like rustdoc no longer used the same settings as the rest of the tree. It also made `SourceType` useless since the only thing it was used for was warnings. I think it would be better for everything in the tree to use the same settings. Fixes rust-lang#64523

…-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

…-Simulacrum Fix generating rustc docs with non-default lib directory. If `libdir` is set in `config.toml`, then the tool to generate the rustc docs was unable to run `rustc` because it could not find the shared libraries. The solution is to set the dylib search path to include the libdir. I changed the API of `add_rustc_lib_path` to take `Command` instead of `Cargo` to try to share the code in several places. This is how it worked before rust-lang#64316, and I think this still retains the spirit of that change. Fixes rust-lang#76702

…, r=albertlarsan68 fix out-of-date comment about rpath in bootstrap in rust-lang#64316 (rust-lang@1bec962), the `RUSTC_RPATH` enviroment variables had been removed , but the comments about the rpath still keep it this PR fix it to avoid misunstanding

rust-highfive assigned nikomatsakis Sep 9, 2019