From 8c388e1b816d0a7e5a7d3cc5d213be7f35299cf5 Mon Sep 17 00:00:00 2001 From: UebelAndre Date: Mon, 18 Jan 2021 03:53:12 -0800 Subject: [PATCH] cargo_build_script now only prints warnings to stderr. (#548) ## Overview This prevents noisy output from `cargo_build_script` from being printed to the terminal, instead two log files are generated (`stdout`, `stderr`) which contain the raw output of the underlying `build.rs` script ## Details Currently users see something similar to the following output when they use `cargo_build_script`: (note, uplaoded as a file because there's a lot of output) [big_output.log](https://github.com/bazelbuild/rules_rust/files/5777780/big_output.log) In general, this masks important information in a sea of arbitrary log lines. The changes in this PR update the output to: ```output INFO: From CargoBuildScriptRun external/cargo_raze__libssh2_sys__0_2_20/libssh2_sys_build_script.out_dir: Build Script Warning: libssh2/src/bcrypt_pbkdf.c:84:46: warning: expression does not compute the number of elements in this array; element type is 'uint32_t' (aka 'unsigned int'), not 'uint64_t' (aka 'unsigned long long') [-Wsizeof-array-div] Build Script Warning: blf_enc(&state, cdata, sizeof(cdata) / sizeof(uint64_t)); Build Script Warning: ~~~~~ ^ Build Script Warning: libssh2/src/bcrypt_pbkdf.c:65:14: note: array 'cdata' declared here Build Script Warning: uint32_t cdata[BCRYPT_BLOCKS]; Build Script Warning: ^ Build Script Warning: libssh2/src/bcrypt_pbkdf.c:84:46: note: place parentheses around the 'sizeof(uint64_t)' expression to silence this warning Build Script Warning: blf_enc(&state, cdata, sizeof(cdata) / sizeof(uint64_t)); Build Script Warning: ^ Build Script Warning: 1 warning generated. ``` And log files are generated for the raw stderr and stdout from the build script, ie: ```output bazel-out/darwin-fastbuild/bin/external/cargo_raze__libssh2_sys__0_2_20/libssh2_sys_build_script.stderr.log bazel-out/darwin-fastbuild/bin/external/cargo_raze__libssh2_sys__0_2_20/libssh2_sys_build_script.stdout.log ``` (note, uplaoded as a file because there's a lot of output) [libssh2_sys_build_script.stderr.log](https://github.com/bazelbuild/rules_rust/files/5777782/libssh2_sys_build_script.stderr.log) Additionally, the `deps` attribute of `cargo_build_script` now only accepts targets that return the `DepInfo` provider. All other dependencies should be passed via `data`. --- cargo/cargo_build_script.bzl | 26 +++++++++++-- cargo/cargo_build_script_runner/bin.rs | 53 ++++++++++++++++++++------ cargo/cargo_build_script_runner/lib.rs | 26 ++++++------- 3 files changed, 75 insertions(+), 30 deletions(-) diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl index 1aa6f5857f..7df940fd6b 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( @@ -174,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, @@ -187,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", diff --git a/cargo/cargo_build_script_runner/bin.rs b/cargo/cargo_build_script_runner/bin.rs index c48208e98f..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 @@ -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(), }) } @@ -261,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 + } + }); +} 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) } }