Merge "Pass cargo output around as string rather than writing to file." into main am: b0147248bd

Original change: https://android-review.googlesource.com/c/platform/development/+/2843614

Change-Id: I9f9caae120b4d757c89f278fbc59baaf26dee395
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

tools/cargo_embargo/Android.bp

@@ -27,6 +27,7 @@ rust_defaults {
         "libenv_logger",
         "libglob",
         "liblog_rust",
+        "libnix",
         "libonce_cell",
         "libregex",
         "libserde",

tools/cargo_embargo/src/cargo/cargo_out.rs

@@ -14,6 +14,7 @@
 
 use super::metadata::WorkspaceMetadata;
 use super::{Crate, CrateType, Extern, ExternType};
+use crate::CargoOutput;
 use anyhow::anyhow;
 use anyhow::bail;
 use anyhow::Context;
@@ -23,7 +24,6 @@ use once_cell::sync::Lazy;
 use regex::Regex;
 use std::collections::BTreeMap;
 use std::env;
-use std::fs::{read_to_string, File};
 use std::path::Path;
 use std::path::PathBuf;
 
@@ -31,16 +31,14 @@ use std::path::PathBuf;
 /// the rustc invocations.
 ///
 /// Ignores crates outside the current directory and build script crates.
-pub fn parse_cargo_out(
+pub fn parse_cargo_out(cargo_output: &CargoOutput) -> Result<Vec<Crate>> {
-    cargo_out_path: impl AsRef<Path>,
+    let metadata = serde_json::from_str(&cargo_output.cargo_metadata)
-    cargo_metadata_path: impl AsRef<Path>,
+        .context("failed to parse cargo metadata")?;
-) -> Result<Vec<Crate>> {
+    parse_cargo_out_str(
-    let cargo_out = read_to_string(cargo_out_path).context("failed to read cargo.out")?;
+        &cargo_output.cargo_out,
-    let metadata = serde_json::from_reader(
+        &metadata,
-        File::open(cargo_metadata_path).context("failed to open cargo.metadata")?,
+        env::current_dir().unwrap().canonicalize().unwrap(),
     )
-    .context("failed to parse cargo.metadata")?;
-    parse_cargo_out_str(&cargo_out, &metadata, env::current_dir().unwrap().canonicalize().unwrap())
 }
 
 /// Parses the given `cargo.out` and `cargo.metadata` file contents and generates a list of crates

tools/cargo_embargo/src/cargo/metadata.rs

@@ -19,7 +19,6 @@ use crate::config::VariantConfig;
 use anyhow::{anyhow, bail, Context, Result};
 use serde::Deserialize;
 use std::collections::BTreeMap;
-use std::fs::File;
 use std::ops::Deref;
 use std::path::{Path, PathBuf};
 
@@ -100,14 +99,9 @@ pub enum TargetKind {
     Test,
 }
 
-pub fn parse_cargo_metadata_file(
+pub fn parse_cargo_metadata_str(cargo_metadata: &str, cfg: &VariantConfig) -> Result<Vec<Crate>> {
-    cargo_metadata_path: impl AsRef<Path>,
+    let metadata =
-    cfg: &VariantConfig,
+        serde_json::from_str(cargo_metadata).context("failed to parse cargo metadata")?;
-) -> Result<Vec<Crate>> {
-    let metadata: WorkspaceMetadata = serde_json::from_reader(
-        File::open(cargo_metadata_path).context("failed to open cargo.metadata")?,
-    )
-    .context("failed to parse cargo.metadata")?;
     parse_cargo_metadata(&metadata, &cfg.features, cfg.tests)
 }
 
@@ -439,7 +433,13 @@ mod tests {
                 .variants
                 .iter()
                 .map(|variant_cfg| {
-                    parse_cargo_metadata_file(&cargo_metadata_path, variant_cfg).unwrap()
+                    parse_cargo_metadata_str(
+                        &read_to_string(&cargo_metadata_path)
+                            .with_context(|| format!("Failed to open {:?}", cargo_metadata_path))
+                            .unwrap(),
+                        variant_cfg,
+                    )
+                    .unwrap()
                 })
                 .collect::<Vec<_>>();
             assert_eq!(crates, expected_crates);

tools/cargo_embargo/src/main.rs

@@ -40,20 +40,23 @@ use anyhow::Context;
 use anyhow::Result;
 use bp::*;
 use cargo::{
-    cargo_out::parse_cargo_out, metadata::parse_cargo_metadata_file, Crate, CrateType, ExternType,
+    cargo_out::parse_cargo_out, metadata::parse_cargo_metadata_str, Crate, CrateType, ExternType,
 };
 use clap::Parser;
 use clap::Subcommand;
 use log::debug;
+use nix::fcntl::OFlag;
+use nix::unistd::pipe2;
 use once_cell::sync::Lazy;
 use std::collections::BTreeMap;
 use std::collections::VecDeque;
 use std::env;
-use std::fs::{write, File};
+use std::fs::{read_to_string, write, File};
-use std::io::Write;
+use std::io::{Read, Write};
+use std::os::fd::{FromRawFd, OwnedFd};
 use std::path::Path;
 use std::path::PathBuf;
-use std::process::Command;
+use std::process::{Command, Stdio};
 
 // Major TODOs
 //  * handle errors, esp. in cargo.out parsing. they should fail the program with an error code
@@ -286,15 +289,22 @@ fn make_crates(args: &Args, cfg: &VariantConfig) -> Result<Vec<Crate>> {
 
     let cargo_out_path = "cargo.out";
     let cargo_metadata_path = "cargo.metadata";
-    if !args.reuse_cargo_out || !Path::new(cargo_out_path).exists() {
+    let cargo_output = if args.reuse_cargo_out && Path::new(cargo_out_path).exists() {
-        generate_cargo_out(cfg, cargo_out_path, cargo_metadata_path)
+        CargoOutput {
-            .context("generate_cargo_out failed")?;
+            cargo_out: read_to_string(cargo_out_path)?,
+            cargo_metadata: read_to_string(cargo_metadata_path)?,
         }
+    } else {
+        let cargo_output = generate_cargo_out(cfg).context("generate_cargo_out failed")?;
+        write(cargo_out_path, &cargo_output.cargo_out)?;
+        write(cargo_metadata_path, &cargo_output.cargo_metadata)?;
+        cargo_output
+    };
 
     if cfg.run_cargo {
-        parse_cargo_out(cargo_out_path, cargo_metadata_path).context("parse_cargo_out failed")
+        parse_cargo_out(&cargo_output).context("parse_cargo_out failed")
     } else {
-        parse_cargo_metadata_file(cargo_metadata_path, cfg)
+        parse_cargo_metadata_str(&cargo_output.cargo_metadata, cfg)
     }
 }
 
@@ -375,32 +385,54 @@ fn write_all_bp(
     Ok(())
 }
 
-fn run_cargo(cargo_out: &mut File, cmd: &mut Command) -> Result<()> {
+/// Runs the given command, and returns its standard output and standard error as a string.
-    use std::os::unix::io::OwnedFd;
+fn run_cargo(cmd: &mut Command) -> Result<String> {
-    use std::process::Stdio;
+    let (pipe_read, pipe_write) = pipe()?;
-    let fd: OwnedFd = cargo_out.try_clone()?.into();
+    cmd.stdout(pipe_write.try_clone()?).stderr(pipe_write).stdin(Stdio::null());
     debug!("Running: {:?}\n", cmd);
-    let output = cmd.stdout(Stdio::from(fd.try_clone()?)).stderr(Stdio::from(fd)).output()?;
+    let mut child = cmd.spawn()?;
-    if !output.status.success() {
+
-        bail!("cargo command failed with exit status: {:?}", output.status);
+    // Unset the stdout and stderr for the command so that they are dropped in this process.
+    // Otherwise the `read_to_string` below will block forever as there is still an open write file
+    // descriptor for the pipe even after the child finishes.
+    cmd.stderr(Stdio::null()).stdout(Stdio::null());
+
+    let mut output = String::new();
+    File::from(pipe_read).read_to_string(&mut output)?;
+    let status = child.wait()?;
+    if !status.success() {
+        bail!(
+            "cargo command `{:?}` failed with exit status: {:?}.\nOutput: \n------\n{}\n------",
+            cmd,
+            status,
+            output
+        );
     }
-    Ok(())
+
+    Ok(output)
 }
 
-/// Run various cargo commands and save the output to `cargo_out_path`.
+/// Creates a new pipe and returns the file descriptors for each end of it.
-fn generate_cargo_out(
+fn pipe() -> Result<(OwnedFd, OwnedFd), nix::Error> {
-    cfg: &VariantConfig,
+    let (a, b) = pipe2(OFlag::O_CLOEXEC)?;
-    cargo_out_path: &str,
+    // SAFETY: We just created the file descriptors, so we own them.
-    cargo_metadata_path: &str,
+    unsafe { Ok((OwnedFd::from_raw_fd(a), OwnedFd::from_raw_fd(b))) }
-) -> Result<()> {
+}
-    let mut cargo_out_file = std::fs::File::create(cargo_out_path)?;
-    let mut cargo_metadata_file = std::fs::File::create(cargo_metadata_path)?;
 
+/// The raw output from running `cargo metadata`, `cargo build` and other commands.
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct CargoOutput {
+    cargo_metadata: String,
+    cargo_out: String,
+}
+
+/// Run various cargo commands and returns the output.
+fn generate_cargo_out(cfg: &VariantConfig) -> Result<CargoOutput> {
     let verbose_args = ["-v"];
     let target_dir_args = ["--target-dir", "target.tmp"];
 
     // cargo clean
-    run_cargo(&mut cargo_out_file, Command::new("cargo").arg("clean").args(target_dir_args))
+    run_cargo(Command::new("cargo").arg("clean").args(target_dir_args))
         .context("Running cargo clean")?;
 
     let default_target = "x86_64-unknown-linux-gnu";
@@ -428,8 +460,7 @@ fn generate_cargo_out(
     };
 
     // cargo metadata
-    run_cargo(
+    let cargo_metadata = run_cargo(
-        &mut cargo_metadata_file,
         Command::new("cargo")
             .arg("metadata")
             .arg("-q") // don't output warnings to stderr
@@ -439,10 +470,10 @@ fn generate_cargo_out(
     )
     .context("Running cargo metadata")?;
 
+    let mut cargo_out = String::new();
     if cfg.run_cargo {
         // cargo build
-        run_cargo(
+        cargo_out += &run_cargo(
-            &mut cargo_out_file,
             Command::new("cargo")
                 .args(["build", "--target", default_target])
                 .args(verbose_args)
@@ -453,8 +484,7 @@ fn generate_cargo_out(
 
         if cfg.tests {
             // cargo build --tests
-            run_cargo(
+            cargo_out += &run_cargo(
-                &mut cargo_out_file,
                 Command::new("cargo")
                     .args(["build", "--target", default_target, "--tests"])
                     .args(verbose_args)
@@ -463,8 +493,7 @@ fn generate_cargo_out(
                     .args(&feature_args),
             )?;
             // cargo test -- --list
-            run_cargo(
+            cargo_out += &run_cargo(
-                &mut cargo_out_file,
                 Command::new("cargo")
                     .args(["test", "--target", default_target])
                     .args(target_dir_args)
@@ -475,7 +504,7 @@ fn generate_cargo_out(
         }
     }
 
-    Ok(())
+    Ok(CargoOutput { cargo_metadata, cargo_out })
 }
 
 /// Create the Android.bp file for `package_dir`.