From 6bdb26930eba0a7c8566bb0741372c82a6424e53 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 28 Dec 2022 11:18:07 +1100 Subject: [PATCH] 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 --- Cargo.lock | 31 +++ crates/bin/src/entry.rs | 156 +++++++------- crates/binstalk/Cargo.toml | 1 + crates/binstalk/src/ops.rs | 1 - crates/binstalk/src/ops/install.rs | 149 ------------- crates/binstalk/src/ops/resolve.rs | 76 +------ crates/binstalk/src/ops/resolve/resolution.rs | 198 ++++++++++++++++++ 7 files changed, 323 insertions(+), 289 deletions(-) delete mode 100644 crates/binstalk/src/ops/install.rs create mode 100644 crates/binstalk/src/ops/resolve/resolution.rs diff --git a/Cargo.lock b/Cargo.lock index a16b8e91..27c89098 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -168,6 +168,7 @@ dependencies = [ "binstalk-downloader", "binstalk-types", "cargo_toml", + "command-group", "compact_str", "crates_io_api", "detect-targets", @@ -474,6 +475,18 @@ dependencies = [ "unicode-width", ] +[[package]] +name = "command-group" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e98782b6b757b633bd1da9d1a4ee7d323a6bd205a32b1c242d5e56e7092dfd3f" +dependencies = [ + "async-trait", + "nix", + "tokio", + "winapi", +] + [[package]] name = "compact_str" version = "0.6.1" @@ -1451,6 +1464,18 @@ dependencies = [ "tempfile", ] +[[package]] +name = "nix" +version = "0.26.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46a58d1d356c6597d08cde02c2f09d785b09e28711837b1ed667dc652c08a694" +dependencies = [ + "bitflags", + "cfg-if", + "libc", + "static_assertions", +] + [[package]] name = "normalize-path" version = "0.2.0" @@ -2169,6 +2194,12 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" +[[package]] +name = "static_assertions" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" + [[package]] name = "strsim" version = "0.10.0" diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index 6cb2216d..5431e4c9 100644 --- a/crates/bin/src/entry.rs +++ b/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::, 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); diff --git a/crates/binstalk/Cargo.toml b/crates/binstalk/Cargo.toml index 06219d06..7b13ebd0 100644 --- a/crates/binstalk/Cargo.toml +++ b/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" } diff --git a/crates/binstalk/src/ops.rs b/crates/binstalk/src/ops.rs index a0a14982..1b49f8a0 100644 --- a/crates/binstalk/src/ops.rs +++ b/crates/binstalk/src/ops.rs @@ -12,7 +12,6 @@ use crate::{ DesiredTargets, }; -pub mod install; pub mod resolve; pub type Resolver = fn(Client, Arc, Arc) -> Arc; diff --git a/crates/binstalk/src/ops/install.rs b/crates/binstalk/src/ops/install.rs deleted file mode 100644 index d541315f..00000000 --- a/crates/binstalk/src/ops/install.rs +++ /dev/null @@ -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, -) -> Result, 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, - bin_files: Vec, -) -> Result>, 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, - }) - } -} diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 306a562a..7fa725ac 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/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, - new_version: Version, - name: CompactString, - version_req: CompactString, - bin_files: Vec, - }, - 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, ) -> Result { 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, crate_name: CrateName, curr_version: Option, ) -> Result { @@ -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) } diff --git a/crates/binstalk/src/ops/resolve/resolution.rs b/crates/binstalk/src/ops/resolve/resolution.rs new file mode 100644 index 00000000..d313ac59 --- /dev/null +++ b/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, + pub new_version: Version, + pub name: CompactString, + pub version_req: CompactString, + pub bin_files: Vec, +} + +pub struct ResolutionSource { + pub name: CompactString, + pub version: CompactString, +} + +pub enum Resolution { + Fetch(Box), + 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 { + 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) -> 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(" ") +}