Auto merge of #115514 - onur-ozkan:bootstrap-codebase-improvements, r=albertlarsan68

bors · bors · commit cdd182cbb2d1 · 2023-09-17T08:11:41.000Z
optimize and cleanup bootstrap source

I suggest reviewing this commit by commit.
diff --git a/src/bootstrap/README.md b/src/bootstrap/README.md
@@ -1,8 +1,7 @@
 # rustbuild - Bootstrapping Rust
 
-This is an in-progress README which is targeted at helping to explain how Rust
-is bootstrapped and in general, some of the technical details of the build
-system.
+This README is aimed at helping to explain how Rust is bootstrapped and in general,
+some of the technical details of the build system.
 
 Note that this README only covers internal information, not how to use the tool.
 Please check [bootstrapping dev guide][bootstrapping-dev-guide] for further information.
diff --git a/src/bootstrap/bin/_helper.rs b/src/bootstrap/bin/_helper.rs
@@ -0,0 +1,24 @@
+/// Parses the value of the "RUSTC_VERBOSE" environment variable and returns it as a `usize`.
+/// If it was not defined, returns 0 by default.
+///
+/// Panics if "RUSTC_VERBOSE" is defined with the value that is not an unsigned integer.
+fn parse_rustc_verbose() -> usize {
+    use std::str::FromStr;
+
+    match std::env::var("RUSTC_VERBOSE") {
+        Ok(s) => usize::from_str(&s).expect("RUSTC_VERBOSE should be an integer"),
+        Err(_) => 0,
+    }
+}
+
+/// Parses the value of the "RUSTC_STAGE" environment variable and returns it as a `String`.
+///
+/// If "RUSTC_STAGE" was not set, the program will be terminated with 101.
+fn parse_rustc_stage() -> String {
+    std::env::var("RUSTC_STAGE").unwrap_or_else(|_| {
+        // Don't panic here; it's reasonable to try and run these shims directly. Give a helpful error instead.
+        eprintln!("rustc shim: fatal: RUSTC_STAGE was not set");
+        eprintln!("rustc shim: note: use `x.py build -vvv` to see all environment variables set by bootstrap");
+        exit(101);
+    })
+}
diff --git a/src/bootstrap/bin/rustc.rs b/src/bootstrap/bin/rustc.rs
@@ -16,27 +16,25 @@
 //! never get replaced.
 
 include!("../dylib_util.rs");
+include!("./_helper.rs");
 
 use std::env;
 use std::path::PathBuf;
 use std::process::{exit, Child, Command};
-use std::str::FromStr;
 use std::time::Instant;
 
 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 stage = parse_rustc_stage();
+    let verbose = parse_rustc_verbose();
+
     // Detect whether or not we're a build script depending on whether --target
     // is passed (a bit janky...)
     let target = arg("--target");
     let version = args.iter().find(|w| &**w == "-vV");
 
-    let verbose = match env::var("RUSTC_VERBOSE") {
-        Ok(s) => usize::from_str(&s).expect("RUSTC_VERBOSE should be an integer"),
-        Err(_) => 0,
-    };
-
     // Use a different compiler for build scripts, since there may not yet be a
     // libstd for the real compiler to use. However, if Cargo is attempting to
     // determine the version of the compiler, the real compiler needs to be
@@ -47,12 +45,7 @@ fn main() {
     } else {
         ("RUSTC_REAL", "RUSTC_LIBDIR")
     };
-    let stage = env::var("RUSTC_STAGE").unwrap_or_else(|_| {
-        // Don't panic here; it's reasonable to try and run these shims directly. Give a helpful error instead.
-        eprintln!("rustc shim: fatal: RUSTC_STAGE was not set");
-        eprintln!("rustc shim: note: use `x.py build -vvv` to see all environment variables set by bootstrap");
-        exit(101);
-    });
+
     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);
 
diff --git a/src/bootstrap/bin/rustdoc.rs b/src/bootstrap/bin/rustdoc.rs
@@ -9,14 +9,14 @@ use std::process::{exit, Command};
 
 include!("../dylib_util.rs");
 
+include!("./_helper.rs");
+
 fn main() {
     let args = env::args_os().skip(1).collect::<Vec<_>>();
-    let stage = env::var("RUSTC_STAGE").unwrap_or_else(|_| {
-        // Don't panic here; it's reasonable to try and run these shims directly. Give a helpful error instead.
-        eprintln!("rustc shim: fatal: RUSTC_STAGE was not set");
-        eprintln!("rustc shim: note: use `x.py build -vvv` to see all environment variables set by bootstrap");
-        exit(101);
-    });
+
+    let stage = parse_rustc_stage();
+    let verbose = parse_rustc_verbose();
+
     let rustdoc = env::var_os("RUSTDOC_REAL").expect("RUSTDOC_REAL was not set");
     let libdir = env::var_os("RUSTDOC_LIBDIR").expect("RUSTDOC_LIBDIR was not set");
     let sysroot = env::var_os("RUSTC_SYSROOT").expect("RUSTC_SYSROOT was not set");
@@ -25,13 +25,6 @@ fn main() {
     // is passed (a bit janky...)
     let target = args.windows(2).find(|w| &*w[0] == "--target").and_then(|w| w[1].to_str());
 
-    use std::str::FromStr;
-
-    let verbose = match env::var("RUSTC_VERBOSE") {
-        Ok(s) => usize::from_str(&s).expect("RUSTC_VERBOSE should be an integer"),
-        Err(_) => 0,
-    };
-
     let mut dylib_path = dylib_path();
     dylib_path.insert(0, PathBuf::from(libdir.clone()));
 
diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs
@@ -525,7 +525,7 @@ impl<'a> ShouldRun<'a> {
                 .iter()
                 .map(|p| {
                     // assert only if `p` isn't submodule
-                    if !submodules_paths.iter().find(|sm_p| p.contains(*sm_p)).is_some() {
+                    if submodules_paths.iter().find(|sm_p| p.contains(*sm_p)).is_none() {
                         assert!(
                             self.builder.src.join(p).exists(),
                             "`should_run.paths` should correspond to real on-disk paths - use `alias` if there is no relevant path: {}",
diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs
@@ -876,10 +876,8 @@ impl Step for Rustc {
                     cargo.rustflag("-Clto=off");
                 }
             }
-        } else {
-            if builder.config.rust_lto == RustcLto::Off {
-                cargo.rustflag("-Clto=off");
-            }
+        } else if builder.config.rust_lto == RustcLto::Off {
+            cargo.rustflag("-Clto=off");
         }
 
         for krate in &*self.crates {
diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs
@@ -322,33 +322,23 @@ pub struct RustfmtMetadata {
     pub version: String,
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
 pub enum RustfmtState {
     SystemToolchain(PathBuf),
     Downloaded(PathBuf),
     Unavailable,
+    #[default]
     LazyEvaluated,
 }
 
-impl Default for RustfmtState {
-    fn default() -> Self {
-        RustfmtState::LazyEvaluated
-    }
-}
-
-#[derive(Debug, Clone, Copy, PartialEq)]
+#[derive(Debug, Default, Clone, Copy, PartialEq)]
 pub enum LlvmLibunwind {
+    #[default]
     No,
     InTree,
     System,
 }
 
-impl Default for LlvmLibunwind {
-    fn default() -> Self {
-        Self::No
-    }
-}
-
 impl FromStr for LlvmLibunwind {
     type Err = String;
 
@@ -362,19 +352,14 @@ impl FromStr for LlvmLibunwind {
     }
 }
 
-#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Default, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum SplitDebuginfo {
     Packed,
     Unpacked,
+    #[default]
     Off,
 }
 
-impl Default for SplitDebuginfo {
-    fn default() -> Self {
-        SplitDebuginfo::Off
-    }
-}
-
 impl std::str::FromStr for SplitDebuginfo {
     type Err = ();
 
@@ -1529,7 +1514,7 @@ impl Config {
             let asserts = llvm_assertions.unwrap_or(false);
             config.llvm_from_ci = match llvm.download_ci_llvm {
                 Some(StringOrBool::String(s)) => {
-                    assert!(s == "if-available", "unknown option `{s}` for download-ci-llvm");
+                    assert_eq!(s, "if-available", "unknown option `{s}` for download-ci-llvm");
                     crate::llvm::is_ci_llvm_available(&config, asserts)
                 }
                 Some(StringOrBool::Bool(b)) => b,
diff --git a/src/bootstrap/config/tests.rs b/src/bootstrap/config/tests.rs
@@ -136,7 +136,7 @@ build-config = {}
         "setting string value without quotes"
     );
     assert_eq!(config.gdb, Some("bar".into()), "setting string value with quotes");
-    assert_eq!(config.deny_warnings, false, "setting boolean value");
+    assert!(!config.deny_warnings, "setting boolean value");
     assert_eq!(
         config.tools,
         Some(["cargo".to_string()].into_iter().collect()),
@@ -181,13 +181,13 @@ fn profile_user_dist() {
 
 #[test]
 fn rust_optimize() {
-    assert_eq!(parse("").rust_optimize.is_release(), true);
-    assert_eq!(parse("rust.optimize = false").rust_optimize.is_release(), false);
-    assert_eq!(parse("rust.optimize = true").rust_optimize.is_release(), true);
-    assert_eq!(parse("rust.optimize = 0").rust_optimize.is_release(), false);
-    assert_eq!(parse("rust.optimize = 1").rust_optimize.is_release(), true);
+    assert!(parse("").rust_optimize.is_release());
+    assert!(!parse("rust.optimize = false").rust_optimize.is_release());
+    assert!(parse("rust.optimize = true").rust_optimize.is_release());
+    assert!(!parse("rust.optimize = 0").rust_optimize.is_release());
+    assert!(parse("rust.optimize = 1").rust_optimize.is_release());
+    assert!(parse("rust.optimize = \"s\"").rust_optimize.is_release());
     assert_eq!(parse("rust.optimize = 1").rust_optimize.get_opt_level(), Some("1".to_string()));
-    assert_eq!(parse("rust.optimize = \"s\"").rust_optimize.is_release(), true);
     assert_eq!(parse("rust.optimize = \"s\"").rust_optimize.get_opt_level(), Some("s".to_string()));
 }
 
diff --git a/src/bootstrap/download.rs b/src/bootstrap/download.rs
@@ -441,7 +441,7 @@ impl Config {
     }
 
     pub(crate) fn download_beta_toolchain(&self) {
-        self.verbose(&format!("downloading stage0 beta artifacts"));
+        self.verbose("downloading stage0 beta artifacts");
 
         let date = &self.stage0_metadata.compiler.date;
         let version = &self.stage0_metadata.compiler.version;
diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs
@@ -116,7 +116,7 @@ pub const VERSION: usize = 2;
 
 /// Extra --check-cfg to add when building
 /// (Mode restriction, config name, config values (if any))
-const EXTRA_CHECK_CFGS: &[(Option<Mode>, &'static str, Option<&[&'static str]>)] = &[
+const EXTRA_CHECK_CFGS: &[(Option<Mode>, &str, Option<&[&'static str]>)] = &[
     (None, "bootstrap", None),
     (Some(Mode::Rustc), "parallel_compiler", None),
     (Some(Mode::ToolRustc), "parallel_compiler", None),
@@ -1757,10 +1757,11 @@ to download LLVM rather than building it.
         //
         // In these cases we automatically enable Ninja if we find it in the
         // environment.
-        if !self.config.ninja_in_file && self.config.build.contains("msvc") {
-            if cmd_finder.maybe_have("ninja").is_some() {
-                return true;
-            }
+        if !self.config.ninja_in_file
+            && self.config.build.contains("msvc")
+            && cmd_finder.maybe_have("ninja").is_some()
+        {
+            return true;
         }
 
         self.config.ninja_in_file
diff --git a/src/bootstrap/llvm.rs b/src/bootstrap/llvm.rs
@@ -155,7 +155,7 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
         "".to_owned()
     };
 
-    if &llvm_sha == "" {
+    if llvm_sha.is_empty() {
         eprintln!("error: could not find commit hash for downloading LLVM");
         eprintln!("help: maybe your repository history is too shallow?");
         eprintln!("help: consider disabling `download-ci-llvm`");
@@ -208,10 +208,10 @@ pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool {
         ("x86_64-unknown-netbsd", false),
     ];
 
-    if !supported_platforms.contains(&(&*config.build.triple, asserts)) {
-        if asserts == true || !supported_platforms.contains(&(&*config.build.triple, true)) {
-            return false;
-        }
+    if !supported_platforms.contains(&(&*config.build.triple, asserts))
+        && (asserts || !supported_platforms.contains(&(&*config.build.triple, true)))
+    {
+        return false;
     }
 
     if is_ci_llvm_modified(config) {
@@ -497,11 +497,11 @@ impl Step for Llvm {
             let mut cmd = Command::new(&res.llvm_config);
             let version = output(cmd.arg("--version"));
             let major = version.split('.').next().unwrap();
-            let lib_name = match &llvm_version_suffix {
+
+            match &llvm_version_suffix {
                 Some(version_suffix) => format!("libLLVM-{major}{version_suffix}.{extension}"),
                 None => format!("libLLVM-{major}.{extension}"),
-            };
-            lib_name
+            }
         };
 
         // When building LLVM with LLVM_LINK_LLVM_DYLIB for macOS, an unversioned
@@ -756,13 +756,15 @@ fn configure_cmake(
 
     // For distribution we want the LLVM tools to be *statically* linked to libstdc++.
     // We also do this if the user explicitly requested static libstdc++.
-    if builder.config.llvm_static_stdcpp {
-        if !target.contains("msvc") && !target.contains("netbsd") && !target.contains("solaris") {
-            if target.contains("apple") || target.contains("windows") {
-                ldflags.push_all("-static-libstdc++");
-            } else {
-                ldflags.push_all("-Wl,-Bsymbolic -static-libstdc++");
-            }
+    if builder.config.llvm_static_stdcpp
+        && !target.contains("msvc")
+        && !target.contains("netbsd")
+        && !target.contains("solaris")
+    {
+        if target.contains("apple") || target.contains("windows") {
+            ldflags.push_all("-static-libstdc++");
+        } else {
+            ldflags.push_all("-Wl,-Bsymbolic -static-libstdc++");
         }
     }
 
diff --git a/src/bootstrap/sanity.rs b/src/bootstrap/sanity.rs
@@ -95,20 +95,19 @@ pub fn check(build: &mut Build) {
                     .unwrap_or(true)
             })
             .any(|build_llvm_ourselves| build_llvm_ourselves);
+
     let need_cmake = building_llvm || build.config.any_sanitizers_enabled();
-    if need_cmake {
-        if cmd_finder.maybe_have("cmake").is_none() {
-            eprintln!(
-                "
+    if need_cmake && cmd_finder.maybe_have("cmake").is_none() {
+        eprintln!(
+            "
 Couldn't find required command: cmake
 
 You should install cmake, or set `download-ci-llvm = true` in the
 `[llvm]` section of `config.toml` to download LLVM rather
 than building it.
 "
-            );
-            crate::exit!(1);
-        }
+        );
+        crate::exit!(1);
     }
 
     build.config.python = build
@@ -202,10 +201,10 @@ than building it.
             .entry(*target)
             .or_insert_with(|| Target::from_triple(&target.triple));
 
-        if target.contains("-none-") || target.contains("nvptx") {
-            if build.no_std(*target) == Some(false) {
-                panic!("All the *-none-* and nvptx* targets are no-std targets")
-            }
+        if (target.contains("-none-") || target.contains("nvptx"))
+            && build.no_std(*target) == Some(false)
+        {
+            panic!("All the *-none-* and nvptx* targets are no-std targets")
         }
 
         // Some environments don't want or need these tools, such as when testing Miri.
diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs

Original file line number	Diff line number	Diff line change
`@@ -876,10 +876,8 @@ impl Step for Rustc {`
`876`	`876`	`cargo.rustflag("-Clto=off");`
`877`	`877`	`}`
`878`	`878`	`}`
`879`		`- } else {`
`880`		`- if builder.config.rust_lto == RustcLto::Off {`
`881`		`- cargo.rustflag("-Clto=off");`
`882`		`- }`
	`879`	`+ } else if builder.config.rust_lto == RustcLto::Off {`
	`880`	`+ cargo.rustflag("-Clto=off");`
`883`	`881`	`}`
`884`	`882`
`885`	`883`	`for krate in &*self.crates {`
Original file line number	Diff line number	Diff line change
`@@ -441,7 +441,7 @@ impl Config {`
`441`	`441`	`}`
`442`	`442`
`443`	`443`	`pub(crate) fn download_beta_toolchain(&self) {`
`444`		`- self.verbose(&format!("downloading stage0 beta artifacts"));`
	`444`	`+ self.verbose("downloading stage0 beta artifacts");`
`445`	`445`
`446`	`446`	`let date = &self.stage0_metadata.compiler.date;`
`447`	`447`	`let version = &self.stage0_metadata.compiler.version;`