Improve code

GuillaumeGomez · GuillaumeGomez · commit 0aa8bc09cd10 · 2023-10-04T23:20:31.000+02:00
diff --git a/build_system/src/build.rs b/build_system/src/build.rs
@@ -1,5 +1,7 @@
 use crate::config::set_config;
-use crate::utils::{get_gcc_path, run_command_with_env, run_command_with_output, walk_dir};
+use crate::utils::{
+    get_gcc_path, run_command, run_command_with_env, run_command_with_output_and_env, walk_dir,
+};
 use std::collections::HashMap;
 use std::ffi::OsStr;
 use std::fs;
@@ -9,7 +11,6 @@ use std::path::Path;
 struct BuildArg {
     codegen_release_channel: bool,
     sysroot_release_channel: bool,
-    no_default_features: bool,
     features: Vec<String>,
     gcc_path: String,
 }
@@ -21,13 +22,16 @@ impl BuildArg {
             gcc_path,
             ..Default::default()
         };
+        // We skip binary name and the `build` command.
         let mut args = std::env::args().skip(2);
 
         while let Some(arg) = args.next() {
             match arg.as_str() {
                 "--release" => build_arg.codegen_release_channel = true,
                 "--release-sysroot" => build_arg.sysroot_release_channel = true,
-                "--no-default-features" => build_arg.no_default_features = true,
+                "--no-default-features" => {
+                    build_arg.features.push("--no-default-features".to_string());
+                }
                 "--features" => {
                     if let Some(arg) = args.next() {
                         build_arg.features.push(arg.as_str().into());
@@ -41,7 +45,7 @@ impl BuildArg {
                     Self::usage();
                     return Ok(None);
                 }
-                a => return Err(format!("Unknown argument `{a}`")),
+                arg => return Err(format!("Unknown argument `{}`", arg)),
             }
         }
         Ok(Some(build_arg))
@@ -70,25 +74,25 @@ fn build_sysroot(
     std::env::set_current_dir("build_sysroot")
         .map_err(|e| format!("Failed to go to `build_sysroot` directory: {e:?}"))?;
     // Cleanup for previous run
-    //     v Clean target dir except for build scripts and incremental cache
-   let _e = walk_dir(
+    // Clean target dir except for build scripts and incremental cache
+    let _ = walk_dir(
         "target",
         |dir: &Path| {
             for top in &["debug", "release"] {
-                let _e = fs::remove_dir_all(dir.join(top).join("build"));
-                let _e = fs::remove_dir_all(dir.join(top).join("deps"));
-                let _e = fs::remove_dir_all(dir.join(top).join("examples"));
-                let _e = fs::remove_dir_all(dir.join(top).join("native"));
+                let _ = fs::remove_dir_all(dir.join(top).join("build"));
+                let _ = fs::remove_dir_all(dir.join(top).join("deps"));
+                let _ = fs::remove_dir_all(dir.join(top).join("examples"));
+                let _ = fs::remove_dir_all(dir.join(top).join("native"));
 
-                let _e = walk_dir(
+                let _ = walk_dir(
                     dir.join(top),
                     |sub_dir: &Path| {
                         if sub_dir
                             .file_name()
                             .map(|s| s.to_str().unwrap().starts_with("libsysroot"))
                             .unwrap_or(false)
                         {
-                            let _e = fs::remove_dir_all(sub_dir);
+                            let _ = fs::remove_dir_all(sub_dir);
                         }
                         Ok(())
                     },
@@ -98,7 +102,7 @@ fn build_sysroot(
                             .map(|s| s.to_str().unwrap().starts_with("libsysroot"))
                             .unwrap_or(false)
                         {
-                            let _e = fs::remove_file(file);
+                            let _ = fs::remove_file(file);
                         }
                         Ok(())
                     },
@@ -109,21 +113,21 @@ fn build_sysroot(
         |_| Ok(()),
     );
 
-    let _e = fs::remove_file("Cargo.lock");
-    let _e = fs::remove_file("test_target/Cargo.lock");
-    let _e = fs::remove_dir_all("sysroot");
+    let _ = fs::remove_file("Cargo.lock");
+    let _ = fs::remove_file("test_target/Cargo.lock");
+    let _ = fs::remove_dir_all("sysroot");
 
     // Builds libs
     let channel = if release_mode {
         let rustflags = env
-            .get(&"RUSTFLAGS".to_owned())
+            .get(&"RUSTFLAGS".to_string())
             .cloned()
             .unwrap_or_default();
         env.insert(
-            "RUSTFLAGS".to_owned(),
-            format!("{rustflags} -Zmir-opt-level=3"),
+            "RUSTFLAGS".to_string(),
+            format!("{} -Zmir-opt-level=3", rustflags),
         );
-        run_command_with_output(
+        run_command_with_output_and_env(
             &[
                 &"cargo",
                 &"build",
@@ -136,7 +140,7 @@ fn build_sysroot(
         )?;
         "release"
     } else {
-        run_command_with_output(
+        run_command_with_output_and_env(
             &[
                 &"cargo",
                 &"build",
@@ -152,12 +156,12 @@ fn build_sysroot(
     };
 
     // Copy files to sysroot
-    let sysroot_path = format!("sysroot/lib/rustlib/{target_triple}/lib/");
+    let sysroot_path = format!("sysroot/lib/rustlib/{}/lib/", target_triple);
     fs::create_dir_all(&sysroot_path)
-        .map_err(|e| format!("Failed to create directory `{sysroot_path}`: {e:?}"))?;
-    let copier = |d: &Path| run_command_with_output(&[&"cp", &"-r", &d, &sysroot_path], None, None);
+        .map_err(|e| format!("Failed to create directory `{}`: {:?}", sysroot_path, e))?;
+    let copier = |d: &Path| run_command(&[&"cp", &"-r", &d, &sysroot_path], None).map(|_| ());
     walk_dir(
-        &format!("target/{target_triple}/{channel}/deps"),
+        &format!("target/{}/{}/deps", target_triple, channel),
         copier,
         copier,
     )?;
@@ -169,21 +173,25 @@ fn build_codegen(args: &BuildArg) -> Result<(), String> {
     let mut env = HashMap::new();
 
     let current_dir =
-        std::env::current_dir().map_err(|e| format!("`current_dir` failed: {e:?}"))?;
-    env.insert(
-        "RUST_COMPILER_RT_ROOT".to_owned(),
-        format!("{}", current_dir.join("llvm/compiler-rt").display()),
-    );
-    env.insert("LD_LIBRARY_PATH".to_owned(), args.gcc_path.clone());
-    env.insert("LIBRARY_PATH".to_owned(), args.gcc_path.clone());
+        std::env::current_dir().map_err(|e| format!("`current_dir` failed: {:?}", e))?;
+    if let Ok(rt_root) = std::env::var("RUST_COMPILER_RT_ROOT") {
+        env.insert("RUST_COMPILER_RT_ROOT".to_string(), rt_root);
+    } else {
+        env.insert(
+            "RUST_COMPILER_RT_ROOT".to_string(),
+            format!("{}", current_dir.join("llvm/compiler-rt").display()),
+        );
+    }
+    env.insert("LD_LIBRARY_PATH".to_string(), args.gcc_path.clone());
+    env.insert("LIBRARY_PATH".to_string(), args.gcc_path.clone());
 
     let mut command: Vec<&dyn AsRef<OsStr>> = vec![&"cargo", &"rustc"];
     if args.codegen_release_channel {
         command.push(&"--release");
-        env.insert("CHANNEL".to_owned(), "release".to_owned());
-        env.insert("CARGO_INCREMENTAL".to_owned(), "1".to_owned());
+        env.insert("CHANNEL".to_string(), "release".to_string());
+        env.insert("CARGO_INCREMENTAL".to_string(), "1".to_string());
     } else {
-        env.insert("CHANNEL".to_owned(), "debug".to_owned());
+        env.insert("CHANNEL".to_string(), "debug".to_string());
     }
     let ref_features = args.features.iter().map(|s| s.as_str()).collect::<Vec<_>>();
     for feature in &ref_features {
@@ -194,10 +202,10 @@ fn build_codegen(args: &BuildArg) -> Result<(), String> {
     let config = set_config(&mut env, &[], Some(&args.gcc_path))?;
 
     // We voluntarily ignore the error.
-    let _e = fs::remove_dir_all("target/out");
+    let _ = fs::remove_dir_all("target/out");
     let gccjit_target = "target/out/gccjit";
     fs::create_dir_all(gccjit_target)
-        .map_err(|e| format!("Failed to create directory `{gccjit_target}`: {e:?}"))?;
+        .map_err(|e| format!("Failed to create directory `{}`: {:?}", gccjit_target, e))?;
 
     println!("[BUILD] sysroot");
     build_sysroot(
@@ -210,7 +218,7 @@ fn build_codegen(args: &BuildArg) -> Result<(), String> {
 
 pub fn run() -> Result<(), String> {
     let args = match BuildArg::new()? {
-        Some(a) => a,
+        Some(args) => args,
         None => return Ok(()),
     };
     build_codegen(&args)?;
diff --git a/build_system/src/config.rs b/build_system/src/config.rs
@@ -14,13 +14,13 @@ pub fn set_config(
     test_flags: &[String],
     gcc_path: Option<&str>,
 ) -> Result<ConfigInfo, String> {
-    env.insert("CARGO_INCREMENTAL".to_owned(), "0".to_owned());
+    env.insert("CARGO_INCREMENTAL".to_string(), "0".to_string());
 
     let gcc_path = match gcc_path {
-        Some(g) => g.to_owned(),
+        Some(g) => g.to_string(),
         None => get_gcc_path()?,
     };
-    env.insert("GCC_PATH".to_owned(), gcc_path.clone());
+    env.insert("GCC_PATH".to_string(), gcc_path.clone());
 
     let os_name = get_os_name()?;
     let dylib_ext = match os_name.as_str() {
@@ -49,72 +49,78 @@ pub fn set_config(
     }
     // Since we don't support ThinLTO, disable LTO completely when not trying to do LTO.
     // TODO(antoyo): remove when we can handle ThinLTO.
-    let disable_lto_lfags = "-Clto=off";
+    let disable_lto_lfags = if env.get(&"CHANNEL".to_string()).is_some() {
+        ""
+    } else {
+        "-Clto=off"
+    }
+    .to_string();
     let current_dir = std_env::current_dir().map_err(|e| format!("`current_dir` failed: {e:?}"))?;
+    let channel = if let Some(channel) = env.get(&"CHANNEL".to_string()) {
+        channel.as_str()
+    } else {
+        "debug"
+    };
     let cg_backend_path = current_dir
         .join("target")
-        .join(if let Some(channel) = env.get(&"CHANNEL".to_owned()) {
-            channel.as_str()
-        } else {
-            "debug"
-        })
-        .join(&format!("librustc_codegen_gcc.{dylib_ext}"));
+        .join(channel)
+        .join(&format!("librustc_codegen_gcc.{}", dylib_ext));
     let sysroot_path = current_dir.join("build_sysroot/sysroot");
     let mut rustflags = Vec::new();
-    if let Some(cg_rustflags) = env.get(&"CG_RUSTFLAGS".to_owned()) {
+    if let Some(cg_rustflags) = env.get(&"CG_RUSTFLAGS".to_string()) {
         rustflags.push(cg_rustflags.clone());
     }
     if let Some(linker) = linker {
-        rustflags.push(linker.to_owned());
+        rustflags.push(linker.to_string());
     }
     rustflags.extend_from_slice(&[
-        "-Csymbol-mangling-version=v0".to_owned(),
-        "-Cdebuginfo=2".to_owned(),
-        disable_lto_lfags.to_owned(),
+        "-Csymbol-mangling-version=v0".to_string(),
+        "-Cdebuginfo=2".to_string(),
+        disable_lto_lfags,
         format!("-Zcodegen-backend={}", cg_backend_path.display()),
-        "--sysroot".to_owned(),
+        "--sysroot".to_string(),
         format!("{}", sysroot_path.display()),
     ]);
     rustflags.extend_from_slice(test_flags);
     // FIXME(antoyo): remove once the atomic shim is gone
     if os_name == "Darwin" {
         rustflags.extend_from_slice(&[
-            "-Clink-arg=-undefined".to_owned(),
-            "-Clink-arg=dynamic_lookup".to_owned(),
+            "-Clink-arg=-undefined".to_string(),
+            "-Clink-arg=dynamic_lookup".to_string(),
         ]);
     }
-    env.insert("RUSTFLAGS".to_owned(), rustflags.join(" "));
+    env.insert("RUSTFLAGS".to_string(), rustflags.join(" "));
     // display metadata load errors
-    env.insert("RUSTC_LOG".to_owned(), "warn".to_owned());
+    env.insert("RUSTC_LOG".to_string(), "warn".to_string());
 
+    let sysroot = current_dir.join(&format!(
+        "build_sysroot/sysroot/lib/rustlib/{}/lib",
+        target_triple
+    ));
     let ld_library_path = format!(
         "{target}:{sysroot}:{gcc_path}",
         target = current_dir.join("target/out").display(),
-        sysroot = current_dir
-            .join(&format!(
-                "build_sysroot/sysroot/lib/rustlib/{target_triple}/lib"
-            ),)
-            .display(),
+        sysroot = sysroot.display(),
     );
-    env.insert("LD_LIBRARY_PATH".to_owned(), ld_library_path.clone());
-    env.insert("DYLD_LIBRARY_PATH".to_owned(), ld_library_path);
+    env.insert("LD_LIBRARY_PATH".to_string(), ld_library_path.clone());
+    env.insert("DYLD_LIBRARY_PATH".to_string(), ld_library_path);
 
     // NOTE: To avoid the -fno-inline errors, use /opt/gcc/bin/gcc instead of cc.
     // To do so, add a symlink for cc to /opt/gcc/bin/gcc in our PATH.
     // Another option would be to add the following Rust flag: -Clinker=/opt/gcc/bin/gcc
     let path = std::env::var("PATH").unwrap_or_default();
-    env.insert("PATH".to_owned(), format!("/opt/gcc/bin:{path}"));
+    env.insert("PATH".to_string(), format!("/opt/gcc/bin:{path}"));
 
-    let mut rustc_command = vec!["rustc".to_owned()];
+    let mut rustc_command = vec!["rustc".to_string()];
     rustc_command.extend_from_slice(&rustflags);
     rustc_command.extend_from_slice(&[
-        "-L".to_owned(),
-        "crate=target/out".to_owned(),
-        "--out-dir".to_owned(),
-        "target/out".to_owned(),
+        "-L".to_string(),
+        "crate=target/out".to_string(),
+        "--out-dir".to_string(),
+        "target/out".to_string(),
     ]);
     Ok(ConfigInfo {
-        target_triple: target_triple.to_owned(),
+        target_triple: target_triple.to_string(),
         rustc_command,
         run_wrapper,
     })
diff --git a/build_system/src/prepare.rs b/build_system/src/prepare.rs
@@ -7,7 +7,7 @@ use std::path::Path;
 fn prepare_libcore(sysroot_path: &Path) -> Result<(), String> {
     let rustc_path = match get_rustc_path() {
         Some(path) => path,
-        None => return Err("`rustc` path not found".to_owned()),
+        None => return Err("`rustc` path not found".to_string()),
     };
 
     let parent = match rustc_path.parent() {
@@ -20,7 +20,7 @@ fn prepare_libcore(sysroot_path: &Path) -> Result<(), String> {
         .canonicalize()
         .map_err(|e| format!("Failed to canonicalize path: {e:?}"))?;
     if !rustlib_dir.is_dir() {
-        return Err("Please install `rust-src` component".to_owned());
+        return Err("Please install `rust-src` component".to_string());
     }
 
     let sysroot_dir = sysroot_path.join("sysroot_src");
@@ -90,8 +90,8 @@ fn prepare_libcore(sysroot_path: &Path) -> Result<(), String> {
     for file_path in patches {
         println!("[GIT] apply `{}`", file_path.display());
         let path = Path::new("../..").join(file_path);
-        run_command_with_output(&[&"git", &"apply", &path], Some(&sysroot_dir), None)?;
-        run_command_with_output(&[&"git", &"add", &"-A"], Some(&sysroot_dir), None)?;
+        run_command_with_output(&[&"git", &"apply", &path], Some(&sysroot_dir))?;
+        run_command_with_output(&[&"git", &"add", &"-A"], Some(&sysroot_dir))?;
         run_command_with_output(
             &[
                 &"git",
@@ -101,7 +101,6 @@ fn prepare_libcore(sysroot_path: &Path) -> Result<(), String> {
                 &format!("Patch {}", path.display()),
             ],
             Some(&sysroot_dir),
-            None,
         )?;
     }
     println!("Successfully prepared libcore for building");
@@ -139,12 +138,11 @@ where
         "crate_patches",
         |_| Ok(()),
         |file_path| {
-            let s = file_path.as_os_str().to_str().unwrap();
-            if s.contains(&filter) && s.ends_with(".patch") {
+            let patch = file_path.as_os_str().to_str().unwrap();
+            if patch.contains(&filter) && patch.ends_with(".patch") {
                 run_command_with_output(
                     &[&"git", &"am", &file_path.canonicalize().unwrap()],
                     Some(&repo_path),
-                    None,
                 )?;
             }
             Ok(())
diff --git a/build_system/src/rustc_info.rs b/build_system/src/rustc_info.rs
@@ -8,5 +8,5 @@ pub fn get_rustc_path() -> Option<PathBuf> {
     }
     run_command(&[&"rustup", &"which", &"rustc"], None)
         .ok()
-        .map(|out| Path::new(String::from_utf8(out.stdout).unwrap().trim()).to_owned())
+        .map(|out| Path::new(String::from_utf8(out.stdout).unwrap().trim()).to_path_buf())
 }
diff --git a/build_system/src/utils.rs b/build_system/src/utils.rs

Original file line number	Diff line number	Diff line change
`@@ -8,5 +8,5 @@ pub fn get_rustc_path() -> Option<PathBuf> {`
`8`	`8`	`}`
`9`	`9`	`run_command(&[&"rustup", &"which", &"rustc"], None)`
`10`	`10`	`.ok()`
`11`		`- .map(\|out\| Path::new(String::from_utf8(out.stdout).unwrap().trim()).to_owned())`
	`11`	`+ .map(\|out\| Path::new(String::from_utf8(out.stdout).unwrap().trim()).to_path_buf())`
`12`	`12`	`}`