From 7b8aed4dee9b5bbff33c2b5709c25a489bbaec7e Mon Sep 17 00:00:00 2001 From: Andre Brisco Date: Wed, 30 Dec 2020 16:13:28 -0800 Subject: [PATCH 1/3] cargo_build_script now only prints warnings to stderr. All output is otherwise written to log files --- cargo/cargo_build_script.bzl | 21 ++++++++++++-- cargo/cargo_build_script_runner/bin.rs | 40 +++++++++++++++++++------- cargo/cargo_build_script_runner/lib.rs | 26 +++++++---------- 3 files changed, 60 insertions(+), 27 deletions(-) diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index 1aa6f5857f..2c4dc02892 100644 --- a/cargo/cargo_build_script.bzl +++ b/cargo/cargo_build_script.bzl @@ -45,6 +45,11 @@ def _build_script_impl(ctx): manifest_dir = "%s.runfiles/%s/%s" % (script.path, ctx.label.workspace_name or ctx.workspace_name, ctx.label.package) compilation_mode_opt_level = get_compilation_mode_opts(ctx, toolchain).opt_level + streams = struct( + stdout = ctx.actions.declare_file(ctx.label.name + ".stdout.log"), + stderr = ctx.actions.declare_file(ctx.label.name + ".stderr.log"), + ) + crate_name = ctx.attr.crate_name # Derive crate name from the rule label which is _build_script if not provided. @@ -134,7 +139,18 @@ def _build_script_impl(ctx): # See https://doc.rust-lang.org/cargo/reference/build-scripts.html#-sys-packages # for details. args = ctx.actions.args() - args.add_all([script.path, crate_name, links, out_dir.path, env_out.path, flags_out.path, link_flags.path, dep_env_out.path]) + args.add_all([ + script.path, + crate_name, + links, + out_dir.path, + env_out.path, + flags_out.path, + link_flags.path, + dep_env_out.path, + streams.stdout.path, + streams.stderr.path, + ]) build_script_inputs = [] for dep in ctx.attr.deps: if DepInfo in dep and dep[DepInfo].dep_env: @@ -147,7 +163,7 @@ def _build_script_impl(ctx): ctx.actions.run( executable = ctx.executable._cargo_build_script_runner, arguments = [args], - outputs = [out_dir, env_out, flags_out, link_flags, dep_env_out], + outputs = [out_dir, env_out, flags_out, link_flags, dep_env_out, streams.stdout, streams.stderr], tools = tools, inputs = build_script_inputs, mnemonic = "CargoBuildScriptRun", @@ -162,6 +178,7 @@ def _build_script_impl(ctx): flags = flags_out, link_flags = link_flags, ), + OutputGroupInfo(streams = depset([streams.stdout, streams.stderr])), ] _build_script_run = rule( diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index c48208e98f..69772c9767 100644 --- a/cargo/cargo_build_script_runner/bin.rs +++ b/cargo/cargo_build_script_runner/bin.rs @@ -44,6 +44,8 @@ fn main() -> Result<(), String> { flagfile, linkflags, output_dep_env_path, + stdout_path, + stderr_path, input_dep_env_paths, } = parse_args()?; @@ -111,14 +113,16 @@ fn main() -> Result<(), String> { } } - let output = BuildScriptOutput::from_command(&mut command).map_err(|exit_code| { + let (buildrs_outputs, process_output) = BuildScriptOutput::from_command(&mut command).map_err(|process_output| { format!( - "Build script process failed{}", - if let Some(exit_code) = exit_code { + "Build script process failed{}\n--stdout:\n{}\n--stderr:\n{}", + if let Some(exit_code) = process_output.status.code() { format!(" with exit code {}", exit_code) } else { String::new() - } + }, + String::from_utf8(process_output.stdout).expect("Failed to parse stdout of child process"), + String::from_utf8(process_output.stderr).expect("Failed to parse stdout of child process"), ) })?; @@ -142,20 +146,30 @@ fn main() -> Result<(), String> { write( &envfile, - BuildScriptOutput::to_env(&output, &exec_root.to_string_lossy()).as_bytes(), + BuildScriptOutput::to_env(&buildrs_outputs, &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()) + BuildScriptOutput::to_dep_env(&buildrs_outputs, crate_links, &exec_root.to_string_lossy()) .as_bytes(), ) .expect(&format!("Unable to write file {:?}", output_dep_env_path)); + write( + &stdout_path, + process_output.stdout, + ) + .expect(&format!("Unable to write file {:?}", stdout_path)); + write( + &stderr_path, + process_output.stderr, + ) + .expect(&format!("Unable to write file {:?}", stderr_path)); let CompileAndLinkFlags { compile_flags, link_flags, - } = BuildScriptOutput::to_flags(&output, &exec_root.to_string_lossy()); + } = BuildScriptOutput::to_flags(&buildrs_outputs, &exec_root.to_string_lossy()); write(&flagfile, compile_flags.as_bytes()) .expect(&format!("Unable to write file {:?}", flagfile)); @@ -174,6 +188,8 @@ struct Options { flagfile: String, linkflags: String, output_dep_env_path: String, + stdout_path: String, + stderr_path: String, input_dep_env_paths: Vec, } @@ -182,7 +198,7 @@ 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()) { + match (args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next(), args.next()) { ( Some(progname), Some(crate_name), @@ -190,8 +206,10 @@ fn parse_args() -> Result { Some(out_dir), Some(envfile), Some(flagfile), - Some(linkflags), - Some(output_dep_env_path) + Some(linkflags), + Some(output_dep_env_path), + Some(stdout_path), + Some(stderr_path), ) => { Ok(Options{ progname, @@ -202,6 +220,8 @@ fn parse_args() -> Result { flagfile, linkflags, output_dep_env_path, + stdout_path, + stderr_path, input_dep_env_paths: args.collect(), }) } diff --git a/cargo/cargo_build_script_runner/lib.rs b/cargo/cargo_build_script_runner/lib.rs index 9bdf397e3f..e6ef266b21 100644 --- a/cargo/cargo_build_script_runner/lib.rs +++ b/cargo/cargo_build_script_runner/lib.rs @@ -15,7 +15,7 @@ //! Parse the output of a cargo build.rs script and generate a list of flags and //! environment variable for the build. use std::io::{BufRead, BufReader, Read}; -use std::process::{Command, Stdio}; +use std::process::{Command, Output}; #[derive(Debug, PartialEq, Eq)] pub struct CompileAndLinkFlags { @@ -51,14 +51,12 @@ impl BuildScriptOutput { let split = line.splitn(2, '=').collect::>(); if split.len() <= 1 { // Not a cargo directive. - print!("{}", line); return None; } let param = split[1].trim().to_owned(); let key_split = split[0].splitn(2, ':').collect::>(); if key_split.len() <= 1 || key_split[0] != "cargo" { // Not a cargo directive. - print!("{}", line); return None; } match key_split[1] { @@ -73,12 +71,12 @@ impl BuildScriptOutput { None } "warning" => { - eprintln!("Build Script Warning: {}", split[1]); + eprint!("Build Script Warning: {}", split[1]); None } "rustc-cdylib-link-arg" => { // cargo:rustc-cdylib-link-arg=FLAG — Passes custom flags to a linker for cdylib crates. - eprintln!( + eprint!( "Warning: build script returned unsupported directive `{}`", split[0] ); @@ -109,18 +107,16 @@ impl BuildScriptOutput { } /// Take a [Command], execute it and converts its input into a vector of [BuildScriptOutput] - pub fn from_command(cmd: &mut Command) -> Result, Option> { - let mut child = cmd - .stdout(Stdio::piped()) - .spawn() + pub fn from_command(cmd: &mut Command) -> Result<(Vec, Output), Output> { + let child_output = cmd + .output() .expect("Unable to start binary"); - let reader = BufReader::new(child.stdout.as_mut().expect("Failed to open stdout")); - let output = Self::from_reader(reader); - let ecode = child.wait().expect("failed to wait on child"); - if ecode.success() { - Ok(output) + if child_output.status.success() { + let reader = BufReader::new(child_output.stdout.as_slice()); + let output = Self::from_reader(reader); + Ok((output, child_output)) } else { - Err(ecode.code()) + Err(child_output) } } From 527158bfaabd46f6991839c3c034b6ae2748712b Mon Sep 17 00:00:00 2001 From: Andre Brisco Date: Sat, 9 Jan 2021 06:53:54 -0800 Subject: [PATCH 2/3] Build script errors now print neatly --- cargo/cargo_build_script_runner/bin.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index 69772c9767..ca8aad8120 100644 --- a/cargo/cargo_build_script_runner/bin.rs +++ b/cargo/cargo_build_script_runner/bin.rs @@ -24,7 +24,7 @@ use std::fs::{create_dir_all, read_to_string, write}; use std::path::Path; use std::process::Command; -fn main() -> Result<(), String> { +fn run_buildrs() -> Result<(), String> { // We use exec_root.join rather than std::fs::canonicalize, to avoid resolving symlinks, as // some execution strategies and remote execution environments may use symlinks in ways which // canonicalizing them may break them, e.g. by having input files be symlinks into a /cas @@ -281,3 +281,14 @@ fn absolutify(root: &Path, maybe_relative: OsString) -> OsString { maybe_relative } } + +fn main() { + std::process::exit(match run_buildrs() { + Ok(_) => 0, + Err(err) => { + // Neatly print errors + eprintln!("{}", err); + 1 + } + }); +} From fe93a45eed24b79a4a7572ae7aa4d69d4bade6be Mon Sep 17 00:00:00 2001 From: Andre Brisco Date: Sat, 9 Jan 2021 14:25:04 -0800 Subject: [PATCH 3/3] Updated requirements of `deps` attribute for `cargo_build_script` --- cargo/cargo_build_script.bzl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index 2c4dc02892..7df940fd6b 100644 --- a/cargo/cargo_build_script.bzl +++ b/cargo/cargo_build_script.bzl @@ -191,7 +191,7 @@ _build_script_run = rule( # The source of truth will be the `cargo_build_script` macro until stardoc # implements documentation inheritence. See https://github.com/bazelbuild/stardoc/issues/27 "script": attr.label( - doc = "The binary script to run, generally a rust_binary target.", + doc = "The binary script to run, generally a `rust_binary` target.", executable = True, allow_files = True, mandatory = True, @@ -204,7 +204,8 @@ _build_script_run = rule( doc = "The name of the native library this crate links against.", ), "deps": attr.label_list( - doc = "The dependencies of the crate defined by `crate_name`", + doc = "The Rust dependencies of the crate defined by `crate_name`", + providers = [DepInfo], ), "version": attr.string( doc = "The semantic version (semver) of the crate",