Refactor: check --version and $crate@$version conflict in cargo-binstall (#2201)

* Rm Options::version_req

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Refactor filter_out_installed_crates

Detect conflict between --version and crate@version in it, instead of in resolve

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix entry.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix resolve.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix entry.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix fmt in entry.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Rm unused import in ops.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

* Fix fmt in entry.rs

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

---------

Signed-off-by: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>

crates/bin/src/entry.rs

@@ -34,7 +34,7 @@ use file_format::FileFormat;
 use home::cargo_home;
 use log::LevelFilter;
 use miette::{miette, Report, Result, WrapErr};
-use semver::Version;
+use semver::{Version, VersionReq};
 use tokio::task::block_in_place;
 use tracing::{debug, error, info, warn};
 
@@ -76,8 +76,13 @@ pub fn install_crates(
     )?;
 
     // Remove installed crates
-    let mut crate_names =
+    let mut crate_names = filter_out_installed_crates(
-        filter_out_installed_crates(args.crate_names, args.force, manifests.as_ref()).peekable();
+        args.crate_names,
+        args.force,
+        manifests.as_ref(),
+        args.version_req,
+    )
+    .peekable();
 
     if crate_names.peek().is_none() {
         debug!("Nothing to do");
@@ -141,7 +146,6 @@ pub fn install_crates(
         locked: args.locked,
         no_track: args.no_track,
 
-        version_req: args.version_req,
         #[cfg(feature = "git")]
         cargo_toml_fetch_override: match (args.manifest_path, args.git) {
             (Some(manifest_path), None) => Some(CargoTomlFetchOverride::Path(manifest_path)),
@@ -222,15 +226,17 @@ pub fn install_crates(
     let no_cleanup = args.no_cleanup;
 
     // Resolve crates
-    let tasks: Vec<_> = crate_names
+    let tasks = crate_names
-        .map(|(crate_name, current_version)| {
+        .map(|res| {
-            AutoAbortJoinHandle::spawn(ops::resolve::resolve(
+            res.map(|(crate_name, current_version)| {
-                binstall_opts.clone(),
+                AutoAbortJoinHandle::spawn(ops::resolve::resolve(
-                crate_name,
+                    binstall_opts.clone(),
-                current_version,
+                    crate_name,
-            ))
+                    current_version,
+                ))
+            })
         })
-        .collect();
+        .collect::<Result<Vec<_>, BinstallError>>()?;
 
     Ok(Some(if args.continue_on_failure {
         AutoAbortJoinHandle::spawn(async move {
@@ -460,11 +466,12 @@ fn filter_out_installed_crates<'a>(
     crate_names: Vec<CrateName>,
     force: bool,
     manifests: Option<&'a Manifests>,
-) -> impl Iterator<Item = (CrateName, Option<semver::Version>)> + 'a {
+    version_req: Option<VersionReq>,
+) -> impl Iterator<Item = Result<(CrateName, Option<semver::Version>), BinstallError>> + 'a {
     let installed_crates = manifests.map(|m| m.installed_crates());
 
     CrateName::dedup(crate_names)
-    .filter_map(move |crate_name| {
+    .filter_map(move |mut crate_name| {
         let name = &crate_name.name;
 
         let curr_version = installed_crates
@@ -474,6 +481,12 @@ fn filter_out_installed_crates<'a>(
             // So here we take ownership of the version stored to avoid cloning.
             .and_then(|crates| crates.get(name));
 
+        match (crate_name.version_req.is_some(), version_req.is_some()) {
+            (false, true) => crate_name.version_req = version_req.clone(),
+            (true, true) => return Some(Err(BinstallError::SuperfluousVersionOption)),
+            _ => (),
+        };
+
         match (
             force,
             curr_version,
@@ -489,10 +502,10 @@ fn filter_out_installed_crates<'a>(
 
             // The version req is "*" thus a remote upgraded version could exist
             (false, Some(curr_version), None) => {
-                Some((crate_name, Some(curr_version.clone())))
+                Some(Ok((crate_name, Some(curr_version.clone()))))
             }
 
-            _ => Some((crate_name, None)),
+            _ => Some(Ok((crate_name, None))),
         }
     })
 }

crates/binstalk/src/ops.rs

@@ -3,7 +3,6 @@
 use std::{path::PathBuf, sync::Arc, time::Duration};
 
 use compact_str::CompactString;
-use semver::VersionReq;
 
 use crate::{
     fetchers::{Data, Fetcher, SignaturePolicy, TargetDataErased},
@@ -38,7 +37,6 @@ pub struct Options {
     pub locked: bool,
     pub no_track: bool,
 
-    pub version_req: Option<VersionReq>,
     pub cargo_toml_fetch_override: Option<CargoTomlFetchOverride>,
     pub cli_overrides: PkgOverride,
 

crates/binstalk/src/ops/resolve.rs

@@ -67,12 +67,7 @@ async fn resolve_inner(
 ) -> Result<Resolution, BinstallError> {
     info!("Resolving package: '{}'", crate_name);
 
-    let version_req = match (&crate_name.version_req, &opts.version_req) {
+    let version_req = crate_name.version_req.unwrap_or(VersionReq::STAR);
-        (Some(version), None) => MaybeOwned::Borrowed(version),
-        (None, Some(version)) => MaybeOwned::Borrowed(version),
-        (Some(_), Some(_)) => Err(BinstallError::SuperfluousVersionOption)?,
-        (None, None) => MaybeOwned::Owned(VersionReq::STAR),
-    };
 
     let version_req_str = version_req.to_compact_string();