From 390406d63525fb173bf1092ffa41218bdae01a41 Mon Sep 17 00:00:00 2001 From: Jiahao XU <30436523+NobodyXu@users.noreply.github.com> Date: Thu, 19 Jun 2025 21:36:45 +1000 Subject: [PATCH] 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 | 45 +++++++++++++++++++----------- crates/binstalk/src/ops.rs | 2 -- crates/binstalk/src/ops/resolve.rs | 7 +---- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index 87893c3b..16eaab2b 100644 --- a/crates/bin/src/entry.rs +++ b/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 = - filter_out_installed_crates(args.crate_names, args.force, manifests.as_ref()).peekable(); + let mut crate_names = filter_out_installed_crates( + 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 - .map(|(crate_name, current_version)| { - AutoAbortJoinHandle::spawn(ops::resolve::resolve( - binstall_opts.clone(), - crate_name, - current_version, - )) + let tasks = crate_names + .map(|res| { + res.map(|(crate_name, current_version)| { + AutoAbortJoinHandle::spawn(ops::resolve::resolve( + binstall_opts.clone(), + crate_name, + current_version, + )) + }) }) - .collect(); + .collect::, 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, force: bool, manifests: Option<&'a Manifests>, -) -> impl Iterator)> + 'a { + version_req: Option, +) -> impl Iterator), 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))), } }) } diff --git a/crates/binstalk/src/ops.rs b/crates/binstalk/src/ops.rs index 88d38fa2..5cfe8a17 100644 --- a/crates/binstalk/src/ops.rs +++ b/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, pub cargo_toml_fetch_override: Option, pub cli_overrides: PkgOverride, diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 74266340..dd5d0bc1 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/crates/binstalk/src/ops/resolve.rs @@ -67,12 +67,7 @@ async fn resolve_inner( ) -> Result { info!("Resolving package: '{}'", crate_name); - let version_req = match (&crate_name.version_req, &opts.version_req) { - (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 = crate_name.version_req.unwrap_or(VersionReq::STAR); let version_req_str = version_req.to_compact_string();