From c7a408a8c58c6dde73a552d6f78f90c7ef96b513 Mon Sep 17 00:00:00 2001 From: UebelAndre Date: Tue, 22 Dec 2020 07:58:44 -0800 Subject: [PATCH] Slight cleanup of `cargo_build_script_runner` + ran rustfmt (#538) I was trying to read through this code and found it a bit hard to follow. This (to me) makes it a bit more readable. There should otherwise be no functional change here. --- cargo/cargo_build_script_runner/bin.rs | 257 +++++++++++++++---------- cargo/cargo_build_script_runner/lib.rs | 6 +- 2 files changed, 164 insertions(+), 99 deletions(-) diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index 8140d139d8..c48208e98f 100644 --- a/cargo/cargo_build_script_runner/bin.rs +++ b/cargo/cargo_build_script_runner/bin.rs @@ -31,121 +31,182 @@ fn main() -> Result<(), String> { // directory - resolving these may cause tools which inspect $0, or try to resolve files // relative to themselves, to fail. let exec_root = env::current_dir().expect("Failed to get current directory"); - - let mut args = env::args().skip(1); let manifest_dir_env = env::var("CARGO_MANIFEST_DIR").expect("CARGO_MANIFEST_DIR was not set"); let rustc_env = env::var("RUSTC").expect("RUSTC was not set"); let manifest_dir = exec_root.join(&manifest_dir_env); let rustc = exec_root.join(&rustc_env); + let Options { + progname, + crate_name, + crate_links, + out_dir, + envfile, + flagfile, + linkflags, + output_dep_env_path, + input_dep_env_paths, + } = parse_args()?; - // TODO: we should consider an alternative to positional arguments. - match (args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next()) { - (Some(progname), Some(crate_name), Some(crate_links), Some(out_dir), Some(envfile), Some(flagfile), Some(linkflags), Some(depenvfile)) => { - let out_dir_abs = exec_root.join(&out_dir); - // For some reason Google's RBE does not create the output directory, force create it. - create_dir_all(&out_dir_abs).expect(&format!("Failed to make output directory: {:?}", out_dir_abs)); - - let target_env_vars = get_target_env_vars(&rustc_env).expect("Error getting target env vars from rustc"); - - let mut command = Command::new(exec_root.join(&progname)); - command - .current_dir(&manifest_dir) - .envs(target_env_vars) - .env("OUT_DIR", out_dir_abs) - .env("CARGO_MANIFEST_DIR", manifest_dir) - .env("RUSTC", rustc) - .env("RUST_BACKTRACE", "full"); - - while let Some(dep_env_path) = args.next() { - if let Ok(contents) = read_to_string(dep_env_path) { - for line in contents.split('\n') { - // split on empty contents will still produce a single empty string in iterable. - if line.is_empty() { - continue; - } - let mut key_val = line.splitn(2, '='); - match (key_val.next(), key_val.next()) { - (Some(key), Some(value)) => { - command.env(key, value.replace("${pwd}", &exec_root.to_string_lossy())); - } - _ => { - return Err("error: Wrong environment file format, should not happen".to_owned()) - } - } + let out_dir_abs = exec_root.join(&out_dir); + // For some reason Google's RBE does not create the output directory, force create it. + create_dir_all(&out_dir_abs).expect(&format!( + "Failed to make output directory: {:?}", + out_dir_abs + )); + + let target_env_vars = + get_target_env_vars(&rustc_env).expect("Error getting target env vars from rustc"); + + let mut command = Command::new(exec_root.join(&progname)); + command + .current_dir(&manifest_dir) + .envs(target_env_vars) + .env("OUT_DIR", out_dir_abs) + .env("CARGO_MANIFEST_DIR", manifest_dir) + .env("RUSTC", rustc) + .env("RUST_BACKTRACE", "full"); + + for dep_env_path in input_dep_env_paths.iter() { + if let Ok(contents) = read_to_string(dep_env_path) { + for line in contents.split('\n') { + // split on empty contents will still produce a single empty string in iterable. + if line.is_empty() { + continue; + } + let mut key_val = line.splitn(2, '='); + match (key_val.next(), key_val.next()) { + (Some(key), Some(value)) => { + command.env(key, value.replace("${pwd}", &exec_root.to_string_lossy())); + } + _ => { + return Err("error: Wrong environment file format, should not happen".to_owned()) } - } else { - return Err("error: Dependency environment file unreadable".to_owned()) } } + } else { + return Err("error: Dependency environment file unreadable".to_owned()) + } + } - if let Some(cc_path) = env::var_os("CC") { - command.env("CC", absolutify(&exec_root, cc_path)); - } + if let Some(cc_path) = env::var_os("CC") { + command.env("CC", absolutify(&exec_root, cc_path)); + } - if let Some(ar_path) = env::var_os("AR") { - // The default OSX toolchain uses libtool as ar_executable not ar. - // This doesn't work when used as $AR, so simply don't set it - tools will probably fall back to - // /usr/bin/ar which is probably good enough. - if Path::new(&ar_path).file_name() == Some("libtool".as_ref()) { - command.env_remove("AR"); - } else { - command.env("AR", absolutify(&exec_root, ar_path)); - } + if let Some(ar_path) = env::var_os("AR") { + // The default OSX toolchain uses libtool as ar_executable not ar. + // This doesn't work when used as $AR, so simply don't set it - tools will probably fall back to + // /usr/bin/ar which is probably good enough. + if Path::new(&ar_path).file_name() == Some("libtool".as_ref()) { + command.env_remove("AR"); + } else { + command.env("AR", absolutify(&exec_root, ar_path)); + } + } + + // replace env vars with a ${pwd} prefix with the exec_root + for (key, value) in env::vars() { + let exec_root_str = exec_root.to_str().expect("exec_root not in utf8"); + if value.contains("${pwd}") { + env::set_var(key, value.replace("${pwd}", exec_root_str)); + } + } + + let output = BuildScriptOutput::from_command(&mut command).map_err(|exit_code| { + format!( + "Build script process failed{}", + if let Some(exit_code) = exit_code { + format!(" with exit code {}", exit_code) + } else { + String::new() } + ) + })?; - // replace env vars with a ${pwd} prefix with the exec_root - for (key, value) in env::vars() { - let exec_root_str = exec_root.to_str().expect("exec_root not in utf8"); - if value.contains("${pwd}") { - env::set_var(key, value.replace("${pwd}", exec_root_str)); - } + // The right way to set the dep env var is to use the links attribute from the + // Cargo.toml, but cargo_build_script didn't used to have a `links` attribute, so for + // backward-compatibility reasons, try to infer it from the name of the crate. + // TODO: remove this backward-compatibility fallback in next major version. + let crate_links = match crate_links.as_ref() { + "" => { + const SYS_CRATE_SUFFIX: &str = "-sys"; + if crate_name.ends_with(SYS_CRATE_SUFFIX) { + crate_name + .split_at(crate_name.rfind(SYS_CRATE_SUFFIX).unwrap()) + .0 + } else { + &crate_name } + } + crate_links => crate_links, + }; + write( + &envfile, + BuildScriptOutput::to_env(&output, &exec_root.to_string_lossy()).as_bytes(), + ) + .expect(&format!("Unable to write file {:?}", envfile)); + write( + &output_dep_env_path, + BuildScriptOutput::to_dep_env(&output, crate_links, &exec_root.to_string_lossy()) + .as_bytes(), + ) + .expect(&format!("Unable to write file {:?}", output_dep_env_path)); - let output = BuildScriptOutput::from_command(&mut command).map_err(|exit_code| { - format!( - "Build script process failed{}", - if let Some(exit_code) = exit_code { - format!(" with exit code {}", exit_code) - } else { - String::new() - } - ) - })?; - - // The right way to set the dep env var is to use the links attribute from the - // Cargo.toml, but cargo_build_script didn't used to have a `links` attribute, so for - // backward-compatibility reasons, try to infer it from the name of the crate. - // TODO: remove this backward-compatibility fallback in next major version. - let crate_links = match crate_links.as_ref() { - "" => { - const SYS_CRATE_SUFFIX: &str = "-sys"; - if crate_name.ends_with(SYS_CRATE_SUFFIX) { - crate_name - .split_at(crate_name.rfind(SYS_CRATE_SUFFIX).unwrap()) - .0 - } else { - &crate_name - } - }, - crate_links => crate_links, - }; - - write(&envfile, BuildScriptOutput::to_env(&output, &exec_root.to_string_lossy()).as_bytes()) - .expect(&format!("Unable to write file {:?}", envfile)); - write(&depenvfile, BuildScriptOutput::to_dep_env(&output, crate_links, &exec_root.to_string_lossy()).as_bytes()) - .expect(&format!("Unable to write file {:?}", depenvfile)); - - let CompileAndLinkFlags { compile_flags, link_flags } = BuildScriptOutput::to_flags(&output, &exec_root.to_string_lossy()); - - write(&flagfile, compile_flags.as_bytes()) - .expect(&format!("Unable to write file {:?}", flagfile)); - write(&linkflags, link_flags.as_bytes()) - .expect(&format!("Unable to write file {:?}", linkflags)); - Ok(()) + let CompileAndLinkFlags { + compile_flags, + link_flags, + } = BuildScriptOutput::to_flags(&output, &exec_root.to_string_lossy()); + + write(&flagfile, compile_flags.as_bytes()) + .expect(&format!("Unable to write file {:?}", flagfile)); + write(&linkflags, link_flags.as_bytes()) + .expect(&format!("Unable to write file {:?}", linkflags)); + Ok(()) +} + +/// A representation of expected command line arguments. +struct Options { + progname: String, + crate_name: String, + crate_links: String, + out_dir: String, + envfile: String, + flagfile: String, + linkflags: String, + output_dep_env_path: String, + input_dep_env_paths: Vec, +} + +/// Parses positional comamnd line arguments into a well defined struct +fn parse_args() -> Result { + let mut args = env::args().skip(1); + + // TODO: we should consider an alternative to positional arguments. + match (args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next()) { + ( + Some(progname), + Some(crate_name), + Some(crate_links), + Some(out_dir), + Some(envfile), + Some(flagfile), + Some(linkflags), + Some(output_dep_env_path) + ) => { + Ok(Options{ + progname, + crate_name, + crate_links, + out_dir, + envfile, + flagfile, + linkflags, + output_dep_env_path, + input_dep_env_paths: args.collect(), + }) } _ => { - Err("Usage: $0 progname crate_name out_dir envfile flagfile linkflagfile depenvfile [arg1...argn]".to_owned()) + Err("Usage: $0 progname crate_name out_dir envfile flagfile linkflagfile output_dep_env_path [arg1...argn]".to_owned()) } } } diff --git a/cargo/cargo_build_script_runner/lib.rs b/cargo/cargo_build_script_runner/lib.rs index c6b87fd15e..9bdf397e3f 100644 --- a/cargo/cargo_build_script_runner/lib.rs +++ b/cargo/cargo_build_script_runner/lib.rs @@ -144,7 +144,11 @@ impl BuildScriptOutput { v.iter() .filter_map(|x| { if let BuildScriptOutput::DepEnv(env) = x { - Some(format!("{}{}", prefix, Self::redact_exec_root(env, exec_root))) + Some(format!( + "{}{}", + prefix, + Self::redact_exec_root(env, exec_root) + )) } else { None }