From 850d440b3cad194bf4259d4addcf72bfc624cf8c Mon Sep 17 00:00:00 2001 From: Andre Brisco Date: Sun, 20 Dec 2020 12:44:52 -0800 Subject: [PATCH 1/3] Slight cleanup of `cargo_build_script_runner` + ran rustfmt on it's src files. --- cargo/cargo_build_script_runner/bin.rs | 260 ++++++++++++++++--------- cargo/cargo_build_script_runner/lib.rs | 6 +- 2 files changed, 168 insertions(+), 98 deletions(-) diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index 8140d139d8..a8b42dc81a 100644 --- a/cargo/cargo_build_script_runner/bin.rs +++ b/cargo/cargo_build_script_runner/bin.rs @@ -31,118 +31,184 @@ 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, + depenvfile, + remaining_args, + } = 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 remaining_args.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( + &depenvfile, + BuildScriptOutput::to_dep_env(&output, crate_links, &exec_root.to_string_lossy()) + .as_bytes(), + ) + .expect(&format!("Unable to write file {:?}", depenvfile)); - 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, + depenvfile: String, + remaining_args: 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(depenvfile) + ) => { + let mut remaining_args = Vec::new(); + while let Some(arg) = args.next() { + remaining_args.push(arg); + } + + Ok(Options{ + progname, + crate_name, + crate_links, + out_dir, + envfile, + flagfile, + linkflags, + depenvfile, + remaining_args, + }) } _ => { Err("Usage: $0 progname crate_name out_dir envfile flagfile linkflagfile depenvfile [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 } From aefdbbd070d8cf682a64c779e1c241c63e8738b0 Mon Sep 17 00:00:00 2001 From: Andre Brisco Date: Tue, 22 Dec 2020 06:58:12 -0800 Subject: [PATCH 2/3] Addressed PR feedback --- cargo/cargo_build_script_runner/bin.rs | 27 +++++++++++--------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index a8b42dc81a..13274b2a8d 100644 --- a/cargo/cargo_build_script_runner/bin.rs +++ b/cargo/cargo_build_script_runner/bin.rs @@ -43,8 +43,8 @@ fn main() -> Result<(), String> { envfile, flagfile, linkflags, - depenvfile, - remaining_args, + output_dep_env_path, + input_dep_env_paths, } = parse_args()?; let out_dir_abs = exec_root.join(&out_dir); @@ -66,7 +66,7 @@ fn main() -> Result<(), String> { .env("RUSTC", rustc) .env("RUST_BACKTRACE", "full"); - for dep_env_path in remaining_args.iter() { + 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. @@ -146,11 +146,11 @@ fn main() -> Result<(), String> { ) .expect(&format!("Unable to write file {:?}", envfile)); write( - &depenvfile, + &output_dep_env_path, BuildScriptOutput::to_dep_env(&output, crate_links, &exec_root.to_string_lossy()) .as_bytes(), ) - .expect(&format!("Unable to write file {:?}", depenvfile)); + .expect(&format!("Unable to write file {:?}", output_dep_env_path)); let CompileAndLinkFlags { compile_flags, @@ -173,8 +173,8 @@ struct Options { envfile: String, flagfile: String, linkflags: String, - depenvfile: String, - remaining_args: Vec, + output_dep_env_path: String, + input_dep_env_paths: Vec, } /// Parses positional comamnd line arguments into a well defined struct @@ -191,13 +191,8 @@ fn parse_args() -> Result { Some(envfile), Some(flagfile), Some(linkflags), - Some(depenvfile) + Some(output_dep_env_path) ) => { - let mut remaining_args = Vec::new(); - while let Some(arg) = args.next() { - remaining_args.push(arg); - } - Ok(Options{ progname, crate_name, @@ -206,12 +201,12 @@ fn parse_args() -> Result { envfile, flagfile, linkflags, - depenvfile, - remaining_args, + 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()) } } } From 135251095ba506b95e7a79312270fa555385502c Mon Sep 17 00:00:00 2001 From: Andre Brisco Date: Tue, 22 Dec 2020 07:43:13 -0800 Subject: [PATCH 3/3] Fixed typo --- cargo/cargo_build_script_runner/bin.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index 13274b2a8d..c48208e98f 100644 --- a/cargo/cargo_build_script_runner/bin.rs +++ b/cargo/cargo_build_script_runner/bin.rs @@ -202,7 +202,7 @@ fn parse_args() -> Result { flagfile, linkflags, output_dep_env_path, - input_dep_env_paths: args.collect(); + input_dep_env_paths: args.collect(), }) } _ => {