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

@@ -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 <crate_name>_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",

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<String>,
 }
 
@@ -182,16 +198,18 @@ fn parse_args() -> Result<Options, String> {
     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), 
             Some(crate_links), 
             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<Options, String> {
                 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
+        }
+    });
+}

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::<Vec<_>>();
         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::<Vec<_>>();
         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<Vec<BuildScriptOutput>, Option<i32>> {
-        let mut child = cmd
-            .stdout(Stdio::piped())
-            .spawn()
+    pub fn from_command(cmd: &mut Command) -> Result<(Vec<BuildScriptOutput>, 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)
         }
     }