Minor refactor and optimization (#543)

* Avoid potential panicking in `args::parse` by using `Vec::get` instead of indexing
* Refactor: Simplify `opts::{resolve, install}` API
   Many parameters can be shared and put into `opts::Options` intead and
   that would also avoid a few `Arc<Path>`.
* Optimize `get_install_path`: Avoid cloning `install_path`
* Optimize `LazyJobserverClient`: Un`Arc` & remove `Clone` impl
   to avoid additional boxing
* Optimize `find_version`: Avoid cloning `semver::Version`
* Optimize `GhCrateMeta::launch_baseline_find_tasks`
   return `impl Iterator<Item = impl Future<Output = ...>>`
   instead of `impl Iterator<Item = AutoAbortJoinHandle<...>>`
   to avoid unnecessary spawning.
   
   Each task spawned has to be boxed and then polled by tokio runtime.
   They might also be moved.
   
   While they increase parallelism, spawning these futures does not justify
   the costs because:
    - Each `Future` only calls `remote_exists`
    - Each `remote_exists` call send requests to the same domain, which is
      likely to share the same http2 connection.
      Since the conn is shared anyway, spawning does not speedup anything
      but merely add communication overhead.
    - Plus the tokio runtime spawning cost
* Optimize `install_crates`: Destruct `Args` before any `.await` point
   to reduce size of the future
* Refactor `logging`: Replace param `arg` with `log_level` & `json_output`
   to avoid dep on `Args`
* Add dep strum & strum_macros to crates/bin
* Derive `strum_macros::EnumCount` for `Strategy`
* Optimize strategies parsing in `install_crates`
* Fix panic in `install_crates` when `Compile` is not the last strategy specified
* Optimize: Take `Vec<Self>` instead of slice in `CrateName::dedup`
* Refactor: Extract new fn `compute_resolvers`
* Refactor: Extract new fn `compute_paths_and_load_manifests`
* Refactor: Extract new fn `filter_out_installed_crates`
* Reorder `install_crates`: Only run target detection if args are valid
   and there are some crates to be installed.
* Optimize `filter_out_installed_crates`: Avoid allocation
   by returning an `Iterator`
* Fix user_agent of `remote::Client`: Let user specify it
* Refactor: Replace `UIThread` with `ui::confirm`
   which is much simpler.

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

crates/binstalk/src/drivers/version.rs

@@ -45,7 +45,7 @@ pub(super) fn find_version<Item: Version, VersionIter: Iterator<Item = Item>>(
             }
         })
         // Return highest version
-        .max_by_key(|(_item, ver)| ver.clone())
+        .max_by(|(_item_x, ver_x), (_item_y, ver_y)| ver_x.cmp(ver_y))
         .ok_or(BinstallError::VersionMismatch {
             req: version_req.clone(),
         })

crates/binstalk/src/fetchers/gh_crate_meta.rs

@@ -1,4 +1,4 @@
-use std::{path::Path, sync::Arc};
+use std::{future::Future, path::Path, sync::Arc};
 
 use compact_str::{CompactString, ToCompactString};
 use futures_util::stream::{FuturesUnordered, StreamExt};
@@ -15,7 +15,6 @@ use crate::{
         download::Download,
         remote::{Client, Method},
         signal::wait_on_cancellation_signal,
-        tasks::AutoAbortJoinHandle,
     },
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
@@ -31,7 +30,7 @@ pub struct GhCrateMeta {
     resolution: OnceCell<(Url, PkgFmt)>,
 }
 
-type BaselineFindTask = AutoAbortJoinHandle<Result<Option<(Url, PkgFmt)>, BinstallError>>;
+type FindTaskRes = Result<Option<(Url, PkgFmt)>, BinstallError>;
 
 impl GhCrateMeta {
     fn launch_baseline_find_tasks<'a>(
@@ -39,7 +38,7 @@ impl GhCrateMeta {
         pkg_fmt: PkgFmt,
         pkg_url: &'a str,
         repo: Option<&'a str>,
-    ) -> impl Iterator<Item = BaselineFindTask> + 'a {
+    ) -> impl Iterator<Item = impl Future<Output = FindTaskRes> + 'a> + 'a {
         // build up list of potential URLs
         let urls = pkg_fmt.extensions().iter().filter_map(move |ext| {
             let ctx = Context::from_data_with_repo(&self.data, ext, repo);
@@ -56,13 +55,13 @@ impl GhCrateMeta {
         urls.map(move |url| {
             let client = self.client.clone();
 
-            AutoAbortJoinHandle::spawn(async move {
+            async move {
                 debug!("Checking for package at: '{url}'");
 
                 Ok((client.remote_exists(url.clone(), Method::HEAD).await?
                     || client.remote_exists(url.clone(), Method::GET).await?)
                     .then_some((url, pkg_fmt)))
-            })
+            }
         })
     }
 }
@@ -134,7 +133,7 @@ impl super::Fetcher for GhCrateMeta {
         };
 
         while let Some(res) = handles.next().await {
-            if let Some((url, pkg_fmt)) = res?? {
+            if let Some((url, pkg_fmt)) = res? {
                 debug!("Winning URL is {url}, with pkg_fmt {pkg_fmt}");
                 self.resolution.set((url, pkg_fmt)).unwrap(); // find() is called first
                 return Ok(true);

crates/binstalk/src/helpers/jobserver_client.rs

@@ -1,12 +1,11 @@
-use std::{num::NonZeroUsize, sync::Arc, thread::available_parallelism};
+use std::{num::NonZeroUsize, thread::available_parallelism};
 
 use jobslot::Client;
 use tokio::sync::OnceCell;
 
 use crate::errors::BinstallError;
 
-#[derive(Clone)]
-pub struct LazyJobserverClient(Arc<OnceCell<Client>>);
+pub struct LazyJobserverClient(OnceCell<Client>);
 
 impl LazyJobserverClient {
     /// This must be called at the start of the program since
@@ -19,7 +18,7 @@ impl LazyJobserverClient {
         // It doesn't do anything that is actually unsafe, like
         // dereferencing pointer.
         let opt = unsafe { Client::from_env() };
-        Self(Arc::new(OnceCell::new_with(opt)))
+        Self(OnceCell::new_with(opt))
     }
 
     pub async fn get(&self) -> Result<&Client, BinstallError> {

crates/binstalk/src/ops.rs

@@ -2,11 +2,12 @@
 
 use std::{path::PathBuf, sync::Arc};
 
+use crates_io_api::AsyncClient as CratesIoApiClient;
 use semver::VersionReq;
 
 use crate::{
     fetchers::{Data, Fetcher},
-    helpers::remote::Client,
+    helpers::{jobserver_client::LazyJobserverClient, remote::Client},
     manifests::cargo_toml_binstall::PkgOverride,
     DesiredTargets,
 };
@@ -20,11 +21,19 @@ pub struct Options {
     pub no_symlinks: bool,
     pub dry_run: bool,
     pub force: bool,
+    pub quiet: bool,
+
     pub version_req: Option<VersionReq>,
     pub manifest_path: Option<PathBuf>,
     pub cli_overrides: PkgOverride,
+
     pub desired_targets: DesiredTargets,
-    pub quiet: bool,
     pub resolvers: Vec<Resolver>,
     pub cargo_install_fallback: bool,
+
+    pub temp_dir: PathBuf,
+    pub install_path: PathBuf,
+    pub client: Client,
+    pub crates_io_api_client: CratesIoApiClient,
+    pub jobserver_client: LazyJobserverClient,
 }

crates/binstalk/src/ops/install.rs

@@ -16,7 +16,6 @@ use crate::{
 pub async fn install(
     resolution: Resolution,
     opts: Arc<Options>,
-    jobserver_client: LazyJobserverClient,
 ) -> Result<Option<CrateInfo>, BinstallError> {
     match resolution {
         Resolution::AlreadyUpToDate => Ok(None),
@@ -51,7 +50,7 @@ pub async fn install(
                     &name,
                     &version,
                     target,
-                    jobserver_client,
+                    &opts.jobserver_client,
                     opts.quiet,
                     opts.force,
                 )
@@ -99,7 +98,7 @@ async fn install_from_source(
     name: &str,
     version: &str,
     target: &str,
-    lazy_jobserver_client: LazyJobserverClient,
+    lazy_jobserver_client: &LazyJobserverClient,
     quiet: bool,
     force: bool,
 ) -> Result<(), BinstallError> {

crates/binstalk/src/ops/resolve.rs

@@ -8,6 +8,7 @@ use std::{
 
 use cargo_toml::Manifest;
 use compact_str::{CompactString, ToCompactString};
+use crates_io_api::AsyncClient as CratesIoApiClient;
 use itertools::Itertools;
 use semver::{Version, VersionReq};
 use tokio::task::block_in_place;
@@ -94,23 +95,11 @@ pub async fn resolve(
     opts: Arc<Options>,
     crate_name: CrateName,
     curr_version: Option<Version>,
-    temp_dir: Arc<Path>,
-    install_path: Arc<Path>,
-    client: Client,
-    crates_io_api_client: crates_io_api::AsyncClient,
 ) -> Result<Resolution, BinstallError> {
     let crate_name_name = crate_name.name.clone();
-    let resolution = resolve_inner(
-        &opts,
-        crate_name,
-        curr_version,
-        temp_dir,
-        install_path,
-        client,
-        crates_io_api_client,
-    )
-    .await
-    .map_err(|err| err.crate_context(crate_name_name))?;
+    let resolution = resolve_inner(&opts, crate_name, curr_version)
+        .await
+        .map_err(|err| err.crate_context(crate_name_name))?;
 
     resolution.print(&opts);
 
@@ -121,10 +110,6 @@ async fn resolve_inner(
     opts: &Options,
     crate_name: CrateName,
     curr_version: Option<Version>,
-    temp_dir: Arc<Path>,
-    install_path: Arc<Path>,
-    client: Client,
-    crates_io_api_client: crates_io_api::AsyncClient,
 ) -> Result<Resolution, BinstallError> {
     info!("Resolving package: '{}'", crate_name);
 
@@ -141,8 +126,8 @@ async fn resolve_inner(
         crate_name.name,
         curr_version,
         version_req,
-        client.clone(),
-        crates_io_api_client).await?
+        opts.client.clone(),
+        &opts.crates_io_api_client).await?
     else {
         return Ok(Resolution::AlreadyUpToDate)
     };
@@ -175,7 +160,7 @@ async fn resolve_inner(
             })
             .cartesian_product(resolvers)
             .map(|(fetcher_data, f)| {
-                let fetcher = f(&client, &fetcher_data);
+                let fetcher = f(&opts.client, &fetcher_data);
                 (
                     fetcher.clone(),
                     AutoAbortJoinHandle::spawn(async move { fetcher.find().await }),
@@ -187,7 +172,7 @@ async fn resolve_inner(
         match handle.flattened_join().await {
             Ok(true) => {
                 // Generate temporary binary path
-                let bin_path = temp_dir.join(format!(
+                let bin_path = opts.temp_dir.join(format!(
                     "bin-{}-{}-{}",
                     package_info.name,
                     fetcher.target(),
@@ -198,7 +183,7 @@ async fn resolve_inner(
                     fetcher.as_ref(),
                     &bin_path,
                     &package_info,
-                    &install_path,
+                    &opts.install_path,
                     opts.no_symlinks,
                 )
                 .await
@@ -407,14 +392,12 @@ impl PackageInfo {
         curr_version: Option<Version>,
         version_req: VersionReq,
         client: Client,
-        crates_io_api_client: crates_io_api::AsyncClient,
+        crates_io_api_client: &CratesIoApiClient,
     ) -> Result<Option<Self>, BinstallError> {
         // Fetch crate via crates.io, git, or use a local manifest path
         let manifest = match opts.manifest_path.as_ref() {
             Some(manifest_path) => load_manifest_path(manifest_path)?,
-            None => {
-                fetch_crate_cratesio(client, &crates_io_api_client, &name, &version_req).await?
-            }
+            None => fetch_crate_cratesio(client, crates_io_api_client, &name, &version_req).await?,
         };
 
         let Some(mut package) = manifest.package else {

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

@@ -43,8 +43,7 @@ impl FromStr for CrateName {
 }
 
 impl CrateName {
-    pub fn dedup(crate_names: &[Self]) -> impl Iterator<Item = Self> {
-        let mut crate_names = crate_names.to_vec();
+    pub fn dedup(mut crate_names: Vec<Self>) -> impl Iterator<Item = Self> {
         crate_names.sort_by(|x, y| x.name.cmp(&y.name));
         crate_names.into_iter().coalesce(|previous, current| {
             if previous.name == current.name {
@@ -62,7 +61,7 @@ mod tests {
 
     macro_rules! assert_dedup {
         ([ $( ( $input_name:expr, $input_version:expr ) ),*  ], [ $( ( $output_name:expr, $output_version:expr ) ),*  ]) => {
-            let input_crate_names = [$( CrateName {
+            let input_crate_names = vec![$( CrateName {
                 name: $input_name.into(),
                 version_req: Some($input_version.parse().unwrap())
             }, )*];
@@ -72,7 +71,7 @@ mod tests {
             }, )*];
             output_crate_names.sort_by(|x, y| x.name.cmp(&y.name));
 
-            let crate_names: Vec<_> = CrateName::dedup(&input_crate_names).collect();
+            let crate_names: Vec<_> = CrateName::dedup(input_crate_names).collect();
             assert_eq!(crate_names, output_crate_names);
         };
     }