Refactor, improvements and bugfix for resolution and installation process (#619)

* Refactor: Extract new mod `ops::resolve::resolution`
* Refactor: Extract new type `ResolutionFetch`
* Refactor: Extract new type `ResolutionSource`
* Improve `Resolution::print`: Provides more details on which crate
   is the resolution for.
* Make `Resolution::print` a pub fn
* Refactor: Extract new fn `ResolutionFetch::install`
* Refactor: Extract new async fn `ResolutionSource::install`
* Optimize `ResolutionSource::install` avoiding `OsStr::to_string_lossy`.
* Add new dep command-group v2.0.0 to binstalk with feat with-tokio
* Fix `ResolutionSource::install`: Use `AsyncCommandGroup::group_spawn`
   instead of `tokio::process::Command::spawn` to ensure all the processes
   spanwed by the `cargo` process can be terminated with one signal sent to
   the `cargo` process.
* Fix printing resolution: Make sure they are printed without interleaving
* Refactor `entry::install_crates`
* Improve dry-run output for `ResolutionSource::install`
* Refactor: Extract new fn `ResolutionFetch::print`
* Refactor: Extract new fn `ResolutionSource::print`
* Optimize `Resolution`: Box unit variant `Fetch`
* Improve formatting of `tokio::process::Command`
   Prints out sth like `cargo install ...` instead of the `Debug::fmt`.
* Improve dry-run output

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

crates/bin/src/entry.rs

@@ -111,90 +111,98 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
     let no_confirm = args.no_confirm;
     let no_cleanup = args.no_cleanup;
 
-    let tasks: Vec<_> = if !dry_run && !no_confirm {
-        // Resolve crates
-        let tasks: Vec<_> = crate_names
-            .map(|(crate_name, current_version)| {
-                AutoAbortJoinHandle::spawn(ops::resolve::resolve(
-                    binstall_opts.clone(),
-                    crate_name,
-                    current_version,
-                ))
-            })
-            .collect();
+    // Resolve crates
+    let tasks: Vec<_> = crate_names
+        .map(|(crate_name, current_version)| {
+            AutoAbortJoinHandle::spawn(ops::resolve::resolve(
+                binstall_opts.clone(),
+                crate_name,
+                current_version,
+            ))
+        })
+        .collect();
 
-        // Confirm
-        let mut resolutions = Vec::with_capacity(tasks.len());
-        for task in tasks {
-            match task.await?? {
-                Resolution::AlreadyUpToDate => {}
-                res => resolutions.push(res),
-            }
-        }
+    // Collect results
+    let mut resolution_fetchs = Vec::new();
+    let mut resolution_sources = Vec::new();
 
-        if resolutions.is_empty() {
-            debug!("Nothing to do");
-            return Ok(());
-        }
-
-        confirm().await?;
-
-        // Install
-        resolutions
-            .into_iter()
-            .map(|resolution| {
-                AutoAbortJoinHandle::spawn(ops::install::install(resolution, binstall_opts.clone()))
-            })
-            .collect()
-    } else {
-        // Resolve crates and install without confirmation
-        crate_names
-            .map(|(crate_name, current_version)| {
-                let opts = binstall_opts.clone();
-
-                AutoAbortJoinHandle::spawn(async move {
-                    let resolution =
-                        ops::resolve::resolve(opts.clone(), crate_name, current_version).await?;
-
-                    ops::install::install(resolution, opts).await
-                })
-            })
-            .collect()
-    };
-
-    let mut metadata_vec = Vec::with_capacity(tasks.len());
     for task in tasks {
-        if let Some(metadata) = task.await?? {
-            metadata_vec.push(metadata);
+        match task.await?? {
+            Resolution::AlreadyUpToDate => {}
+            Resolution::Fetch(fetch) => {
+                fetch.print(&binstall_opts);
+                resolution_fetchs.push(fetch)
+            }
+            Resolution::InstallFromSource(source) => {
+                source.print();
+                resolution_sources.push(source)
+            }
         }
     }
 
-    block_in_place(|| {
-        if let Some((mut cargo_binstall_metadata, _)) = metadata {
-            // The cargo manifest path is already created when loading
-            // metadata.
+    if resolution_fetchs.is_empty() && resolution_sources.is_empty() {
+        debug!("Nothing to do");
+        return Ok(());
+    }
 
-            debug!("Writing .crates.toml");
-            CratesToml::append_to_path(cargo_roots.join(".crates.toml"), metadata_vec.iter())?;
+    // Confirm
+    if !dry_run && !no_confirm {
+        confirm().await?;
+    }
 
-            debug!("Writing binstall/crates-v1.json");
-            for metadata in metadata_vec {
-                cargo_binstall_metadata.replace(metadata);
-            }
-            cargo_binstall_metadata.overwrite()?;
-        }
-
-        if no_cleanup {
-            // Consume temp_dir without removing it from fs.
-            temp_dir.into_path();
+    if !resolution_fetchs.is_empty() {
+        if dry_run {
+            info!("Dry-run: Not proceeding to install fetched binaries");
         } else {
-            temp_dir.close().unwrap_or_else(|err| {
-                warn!("Failed to clean up some resources: {err}");
-            });
-        }
+            let f = || -> Result<()> {
+                let metadata_vec = resolution_fetchs
+                    .into_iter()
+                    .map(|fetch| fetch.install(&binstall_opts))
+                    .collect::<Result<Vec<_>, BinstallError>>()?;
 
-        Ok(())
-    })
+                if let Some((mut cargo_binstall_metadata, _)) = metadata {
+                    // The cargo manifest path is already created when loading
+                    // metadata.
+
+                    debug!("Writing .crates.toml");
+                    CratesToml::append_to_path(
+                        cargo_roots.join(".crates.toml"),
+                        metadata_vec.iter(),
+                    )?;
+
+                    debug!("Writing binstall/crates-v1.json");
+                    for metadata in metadata_vec {
+                        cargo_binstall_metadata.replace(metadata);
+                    }
+                    cargo_binstall_metadata.overwrite()?;
+                }
+
+                if no_cleanup {
+                    // Consume temp_dir without removing it from fs.
+                    temp_dir.into_path();
+                } else {
+                    temp_dir.close().unwrap_or_else(|err| {
+                        warn!("Failed to clean up some resources: {err}");
+                    });
+                }
+
+                Ok(())
+            };
+
+            block_in_place(f)?;
+        }
+    }
+
+    let tasks: Vec<_> = resolution_sources
+        .into_iter()
+        .map(|source| AutoAbortJoinHandle::spawn(source.install(binstall_opts.clone())))
+        .collect();
+
+    for task in tasks {
+        task.await??;
+    }
+
+    Ok(())
 }
 
 type Metadata = (BinstallCratesV1Records, BTreeMap<CompactString, Version>);

crates/binstalk/Cargo.toml

@@ -14,6 +14,7 @@ async-trait = "0.1.60"
 binstalk-downloader = { version = "0.2.0", path = "../binstalk-downloader" }
 binstalk-types = { version = "0.1.0", path = "../binstalk-types" }
 cargo_toml = "0.13.0"
+command-group = { version = "2.0.0", features = ["with-tokio"] }
 compact_str = { version = "0.6.1", features = ["serde"] }
 crates_io_api = { version = "0.8.1", default-features = false }
 detect-targets = { version = "0.1.3", path = "../detect-targets" }

crates/binstalk/src/ops.rs

@@ -12,7 +12,6 @@ use crate::{
     DesiredTargets,
 };
 
-pub mod install;
 pub mod resolve;
 
 pub type Resolver = fn(Client, Arc<Data>, Arc<TargetData>) -> Arc<dyn Fetcher>;

crates/binstalk/src/ops/install.rs

@@ -1,149 +0,0 @@
-use std::{borrow::Cow, env, ffi::OsStr, sync::Arc};
-
-use compact_str::CompactString;
-use tokio::{process::Command, task::block_in_place};
-use tracing::{debug, error, info, instrument};
-
-use super::{resolve::Resolution, Options};
-use crate::{
-    bins,
-    errors::BinstallError,
-    helpers::jobserver_client::LazyJobserverClient,
-    manifests::crate_info::{CrateInfo, CrateSource},
-};
-
-#[instrument(skip_all)]
-pub async fn install(
-    resolution: Resolution,
-    opts: Arc<Options>,
-) -> Result<Option<CrateInfo>, BinstallError> {
-    match resolution {
-        Resolution::AlreadyUpToDate => Ok(None),
-        Resolution::Fetch {
-            fetcher,
-            new_version,
-            name,
-            version_req,
-            bin_files,
-        } => {
-            let target = fetcher.target().into();
-
-            install_from_package(opts, bin_files).map(|option| {
-                option.map(|bins| CrateInfo {
-                    name,
-                    version_req,
-                    current_version: new_version,
-                    source: CrateSource::cratesio_registry(),
-                    target,
-                    bins,
-                })
-            })
-        }
-        Resolution::InstallFromSource { name, version } => {
-            let desired_targets = opts.desired_targets.get().await;
-            let target = desired_targets
-                .first()
-                .ok_or(BinstallError::NoViableTargets)?;
-
-            if !opts.dry_run {
-                install_from_source(
-                    &name,
-                    &version,
-                    target,
-                    &opts.jobserver_client,
-                    opts.quiet,
-                    opts.force,
-                )
-                .await
-                .map(|_| None)
-            } else {
-                info!(
-                    "Dry-run: running `cargo install {name} --version {version} --target {target}`",
-                );
-                Ok(None)
-            }
-        }
-    }
-}
-
-fn install_from_package(
-    opts: Arc<Options>,
-    bin_files: Vec<bins::BinFile>,
-) -> Result<Option<Vec<CompactString>>, BinstallError> {
-    if opts.dry_run {
-        info!("Dry run, not proceeding");
-        return Ok(None);
-    }
-
-    info!("Installing binaries...");
-    block_in_place(|| {
-        for file in &bin_files {
-            file.install_bin()?;
-        }
-
-        // Generate symlinks
-        if !opts.no_symlinks {
-            for file in &bin_files {
-                file.install_link()?;
-            }
-        }
-
-        Ok(Some(
-            bin_files.into_iter().map(|bin| bin.base_name).collect(),
-        ))
-    })
-}
-
-async fn install_from_source(
-    name: &str,
-    version: &str,
-    target: &str,
-    lazy_jobserver_client: &LazyJobserverClient,
-    quiet: bool,
-    force: bool,
-) -> Result<(), BinstallError> {
-    let jobserver_client = lazy_jobserver_client.get().await?;
-
-    let cargo = env::var_os("CARGO")
-        .map(Cow::Owned)
-        .unwrap_or_else(|| Cow::Borrowed(OsStr::new("cargo")));
-
-    debug!(
-        "Running `{} install {name} --version {version} --target {target}`",
-        cargo.to_string_lossy(),
-    );
-
-    let mut cmd = Command::new(cargo);
-
-    cmd.arg("install")
-        .arg(name)
-        .arg("--version")
-        .arg(version)
-        .arg("--target")
-        .arg(target)
-        .kill_on_drop(true);
-
-    if quiet {
-        cmd.arg("--quiet");
-    }
-
-    if force {
-        cmd.arg("--force");
-    }
-
-    let mut child = jobserver_client.configure_and_run(&mut cmd, |cmd| cmd.spawn())?;
-
-    debug!("Spawned command pid={:?}", child.id());
-
-    let status = child.wait().await?;
-    if status.success() {
-        info!("Cargo finished successfully");
-        Ok(())
-    } else {
-        error!("Cargo errored! {status:?}");
-        Err(BinstallError::SubProcess {
-            command: format!("{cmd:?}").into_boxed_str(),
-            status,
-        })
-    }
-}

crates/binstalk/src/ops/resolve.rs

@@ -27,66 +27,14 @@ use crate::{
 mod crate_name;
 #[doc(inline)]
 pub use crate_name::CrateName;
+
 mod version_ext;
 #[doc(inline)]
 pub use version_ext::VersionReqExt;
 
-pub enum Resolution {
-    Fetch {
-        fetcher: Arc<dyn Fetcher>,
-        new_version: Version,
-        name: CompactString,
-        version_req: CompactString,
-        bin_files: Vec<bins::BinFile>,
-    },
-    InstallFromSource {
-        name: CompactString,
-        version: CompactString,
-    },
-    AlreadyUpToDate,
-}
-impl Resolution {
-    fn print(&self, opts: &Options) {
-        match self {
-            Resolution::Fetch {
-                fetcher, bin_files, ..
-            } => {
-                let fetcher_target = fetcher.target();
-                // Prompt user for confirmation
-                debug!(
-                    "Found a binary install source: {} ({fetcher_target})",
-                    fetcher.source_name()
-                );
-
-                warn!(
-                    "The package will be downloaded from {}{}",
-                    if fetcher.is_third_party() {
-                        "third-party source "
-                    } else {
-                        ""
-                    },
-                    fetcher.source_name()
-                );
-
-                info!("This will install the following binaries:");
-                for file in bin_files {
-                    info!("  - {}", file.preview_bin());
-                }
-
-                if !opts.no_symlinks {
-                    info!("And create (or update) the following symlinks:");
-                    for file in bin_files {
-                        info!("  - {}", file.preview_link());
-                    }
-                }
-            }
-            Resolution::InstallFromSource { .. } => {
-                warn!("The package will be installed from source (with cargo)",)
-            }
-            Resolution::AlreadyUpToDate => (),
-        }
-    }
-}
+mod resolution;
+#[doc(inline)]
+pub use resolution::{Resolution, ResolutionFetch, ResolutionSource};
 
 #[instrument(skip_all)]
 pub async fn resolve(
@@ -95,17 +43,15 @@ pub async fn resolve(
     curr_version: Option<Version>,
 ) -> Result<Resolution, BinstallError> {
     let crate_name_name = crate_name.name.clone();
-    let resolution = resolve_inner(&opts, crate_name, curr_version)
+    let resolution = resolve_inner(opts, crate_name, curr_version)
         .await
         .map_err(|err| err.crate_context(crate_name_name))?;
 
-    resolution.print(&opts);
-
     Ok(resolution)
 }
 
 async fn resolve_inner(
-    opts: &Options,
+    opts: Arc<Options>,
     crate_name: CrateName,
     curr_version: Option<Version>,
 ) -> Result<Resolution, BinstallError> {
@@ -120,7 +66,7 @@ async fn resolve_inner(
 
     let version_req_str = version_req.to_compact_string();
 
-    let Some(package_info) = PackageInfo::resolve(opts,
+    let Some(package_info) = PackageInfo::resolve(&opts,
         crate_name.name,
         curr_version,
         version_req,
@@ -188,13 +134,13 @@ async fn resolve_inner(
                 {
                     Ok(bin_files) => {
                         if !bin_files.is_empty() {
-                            return Ok(Resolution::Fetch {
+                            return Ok(Resolution::Fetch(Box::new(ResolutionFetch {
                                 fetcher,
                                 new_version: package_info.version,
                                 name: package_info.name,
                                 version_req: version_req_str,
                                 bin_files,
-                            });
+                            })));
                         } else {
                             warn!(
                                 "Error when checking binaries provided by fetcher {}: \
@@ -227,10 +173,10 @@ async fn resolve_inner(
     }
 
     if opts.cargo_install_fallback {
-        Ok(Resolution::InstallFromSource {
+        Ok(Resolution::InstallFromSource(ResolutionSource {
             name: package_info.name,
             version: package_info.version_str,
-        })
+        }))
     } else {
         Err(BinstallError::NoFallbackToCargoInstall)
     }

crates/binstalk/src/ops/resolve/resolution.rs

@@ -0,0 +1,198 @@
+use std::{borrow::Cow, env, ffi::OsStr, fmt, iter, path::Path, sync::Arc};
+
+use command_group::AsyncCommandGroup;
+use compact_str::{CompactString, ToCompactString};
+use either::Either;
+use itertools::Itertools;
+use semver::Version;
+use tokio::process::Command;
+use tracing::{debug, error, info, warn};
+
+use crate::{
+    bins,
+    errors::BinstallError,
+    fetchers::Fetcher,
+    manifests::crate_info::{CrateInfo, CrateSource},
+    ops::Options,
+};
+
+pub struct ResolutionFetch {
+    pub fetcher: Arc<dyn Fetcher>,
+    pub new_version: Version,
+    pub name: CompactString,
+    pub version_req: CompactString,
+    pub bin_files: Vec<bins::BinFile>,
+}
+
+pub struct ResolutionSource {
+    pub name: CompactString,
+    pub version: CompactString,
+}
+
+pub enum Resolution {
+    Fetch(Box<ResolutionFetch>),
+    InstallFromSource(ResolutionSource),
+    AlreadyUpToDate,
+}
+
+impl Resolution {
+    pub fn print(&self, opts: &Options) {
+        match self {
+            Resolution::Fetch(fetch) => {
+                fetch.print(opts);
+            }
+            Resolution::InstallFromSource(source) => {
+                source.print();
+            }
+            Resolution::AlreadyUpToDate => (),
+        }
+    }
+}
+
+impl ResolutionFetch {
+    pub fn install(self, opts: &Options) -> Result<CrateInfo, BinstallError> {
+        info!("Installing binaries...");
+        for file in &self.bin_files {
+            file.install_bin()?;
+        }
+
+        // Generate symlinks
+        if !opts.no_symlinks {
+            for file in &self.bin_files {
+                file.install_link()?;
+            }
+        }
+
+        Ok(CrateInfo {
+            name: self.name,
+            version_req: self.version_req,
+            current_version: self.new_version,
+            source: CrateSource::cratesio_registry(),
+            target: self.fetcher.target().to_compact_string(),
+            bins: self
+                .bin_files
+                .into_iter()
+                .map(|bin| bin.base_name)
+                .collect(),
+        })
+    }
+
+    pub fn print(&self, opts: &Options) {
+        let fetcher = &self.fetcher;
+        let bin_files = &self.bin_files;
+        let name = &self.name;
+        let new_version = &self.new_version;
+
+        debug!(
+            "Found a binary install source: {} ({})",
+            fetcher.source_name(),
+            fetcher.target()
+        );
+
+        warn!(
+            "The package {name} v{new_version} will be downloaded from {}{}",
+            if fetcher.is_third_party() {
+                "third-party source "
+            } else {
+                ""
+            },
+            fetcher.source_name()
+        );
+
+        info!("This will install the following binaries:");
+        for file in bin_files {
+            info!("  - {}", file.preview_bin());
+        }
+
+        if !opts.no_symlinks {
+            info!("And create (or update) the following symlinks:");
+            for file in bin_files {
+                info!("  - {}", file.preview_link());
+            }
+        }
+    }
+}
+
+impl ResolutionSource {
+    pub async fn install(self, opts: Arc<Options>) -> Result<(), BinstallError> {
+        let desired_targets = opts.desired_targets.get().await;
+        let target = desired_targets
+            .first()
+            .ok_or(BinstallError::NoViableTargets)?;
+
+        let name = &self.name;
+        let version = &self.version;
+
+        let cargo = env::var_os("CARGO")
+            .map(Cow::Owned)
+            .unwrap_or_else(|| Cow::Borrowed(OsStr::new("cargo")));
+
+        debug!(
+            "Running `{} install {name} --version {version} --target {target}`",
+            Path::new(&cargo).display(),
+        );
+
+        let mut cmd = Command::new(cargo);
+
+        cmd.arg("install")
+            .arg(name)
+            .arg("--version")
+            .arg(version)
+            .arg("--target")
+            .arg(target)
+            .kill_on_drop(true);
+
+        if opts.quiet {
+            cmd.arg("--quiet");
+        }
+
+        if opts.force {
+            cmd.arg("--force");
+        }
+
+        if !opts.dry_run {
+            let mut child = opts
+                .jobserver_client
+                .get()
+                .await?
+                .configure_and_run(&mut cmd, |cmd| cmd.group_spawn())?;
+
+            debug!("Spawned command pid={:?}", child.id());
+
+            let status = child.wait().await?;
+            if status.success() {
+                info!("Cargo finished successfully");
+                Ok(())
+            } else {
+                error!("Cargo errored! {status:?}");
+                Err(BinstallError::SubProcess {
+                    command: format_cmd(&cmd).to_string().into_boxed_str(),
+                    status,
+                })
+            }
+        } else {
+            info!("Dry-run: running `{}`", format_cmd(&cmd));
+            Ok(())
+        }
+    }
+
+    pub fn print(&self) {
+        warn!(
+            "The package {} v{} will be installed from source (with cargo)",
+            self.name, self.version
+        )
+    }
+}
+
+fn format_cmd(cmd: &Command) -> impl fmt::Display + '_ {
+    let cmd = cmd.as_std();
+
+    let program = Either::Left(Path::new(cmd.get_program()).display());
+
+    let program_args = cmd
+        .get_args()
+        .map(OsStr::to_string_lossy)
+        .map(Either::Right);
+
+    iter::once(program).chain(program_args).format(" ")
+}