diff --git a/crates/bin/src/args.rs b/crates/bin/src/args.rs index 84c936ee..30c41fc5 100644 --- a/crates/bin/src/args.rs +++ b/crates/bin/src/args.rs @@ -7,14 +7,14 @@ use std::{ }; use binstalk::{ - errors::BinstallError, helpers::remote::tls::Version, manifests::cargo_toml_binstall::PkgFmt, ops::resolve::{CrateName, VersionReqExt}, }; -use clap::{Parser, ValueEnum}; +use clap::{error::ErrorKind, CommandFactory, Parser, ValueEnum}; use log::LevelFilter; use semver::VersionReq; +use strum::EnumCount; use strum_macros::EnumCount; #[derive(Debug, Parser)] @@ -39,10 +39,10 @@ pub struct Args { /// If duplicate names are provided, the last one (and their version requirement) /// is kept. #[clap( - help_heading = "Package selection", - value_name = "crate[@version]", - required_unless_present_any = ["version", "help"], -)] + help_heading = "Package selection", + value_name = "crate[@version]", + required_unless_present_any = ["version", "help"], + )] pub crate_names: Vec, /// Package version to install. @@ -309,7 +309,7 @@ pub enum Strategy { Compile, } -pub fn parse() -> Result { +pub fn parse() -> Args { // Filter extraneous arg when invoked by cargo // `cargo run -- --help` gives ["target/debug/cargo-binstall", "--help"] // `cargo binstall --help` gives ["/home/ryan/.cargo/bin/cargo-binstall", "binstall", "--help"] @@ -335,6 +335,9 @@ pub fn parse() -> Result { opts.log_level = LevelFilter::Off; } + // Ensure no conflict + let mut command = Args::command(); + if opts.crate_names.len() > 1 { let option = if opts.version_req.is_some() { "version" @@ -345,9 +348,95 @@ pub fn parse() -> Result { }; if !option.is_empty() { - return Err(BinstallError::OverrideOptionUsedWithMultiInstall { option }); + command + .error( + ErrorKind::ArgumentConflict, + format_args!( + r#"override option used with multi package syntax. +You cannot use --{option} and specify multiple packages at the same time. Do one or the other."# + ), + ) + .exit(); } } - Ok(opts) + // Check strategies for duplicates + let mut new_dup_strategy_err = || { + command.error( + ErrorKind::TooManyValues, + "--strategies should not contain duplicate strategy", + ) + }; + + if opts.strategies.len() > Strategy::COUNT { + // If len of strategies is larger than number of variants of Strategy, + // then there must be duplicates by pigeon hole principle. + new_dup_strategy_err().exit() + } + + // Whether specific variant of Strategy is present + let mut is_variant_present = [false; Strategy::COUNT]; + + for strategy in &opts.strategies { + let index = *strategy as u8 as usize; + if is_variant_present[index] { + new_dup_strategy_err().exit() + } else { + is_variant_present[index] = true; + } + } + + // Default strategies if empty + if opts.strategies.is_empty() { + opts.strategies = vec![ + Strategy::CrateMetaData, + Strategy::QuickInstall, + Strategy::Compile, + ]; + } + + // Filter out all disabled strategies + if !opts.disable_strategies.is_empty() { + // Since order doesn't matter, we can sort it and remove all duplicates + // to speedup checking. + opts.disable_strategies.sort_unstable(); + opts.disable_strategies.dedup(); + + // disable_strategies.len() <= Strategy::COUNT, of which is faster + // to just use [Strategy]::contains rather than + // [Strategy]::binary_search + opts.strategies + .retain(|strategy| !opts.disable_strategies.contains(strategy)); + + if opts.strategies.is_empty() { + command + .error(ErrorKind::TooFewValues, "You have disabled all strategies") + .exit() + } + + // Free disable_strategies as it will not be used again. + opts.disable_strategies = Vec::new(); + } + + // Ensure that Strategy::Compile is specified as the last strategy + if opts.strategies[..(opts.strategies.len() - 1)].contains(&Strategy::Compile) { + command + .error( + ErrorKind::InvalidValue, + "Compile strategy must be the last one", + ) + .exit() + } + + opts +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn verify_cli() { + Args::command().debug_assert() + } } diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index 5ae562d4..48a7161d 100644 --- a/crates/bin/src/entry.rs +++ b/crates/bin/src/entry.rs @@ -17,7 +17,6 @@ use binstalk_manifests::{ use crates_io_api::AsyncClient as CratesIoApiClient; use log::LevelFilter; use miette::{miette, Result, WrapErr}; -use strum::EnumCount; use tokio::task::block_in_place; use tracing::{debug, error, info, warn}; @@ -28,9 +27,21 @@ use crate::{ }; pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -> Result<()> { - // Compute strategies - let (resolvers, cargo_install_fallback) = - compute_resolvers(args.strategies, args.disable_strategies)?; + // Compute Resolvers + let mut cargo_install_fallback = false; + + let resolvers: Vec<_> = args + .strategies + .into_iter() + .filter_map(|strategy| match strategy { + Strategy::CrateMetaData => Some(GhCrateMeta::new as Resolver), + Strategy::QuickInstall => Some(QuickInstall::new as Resolver), + Strategy::Compile => { + cargo_install_fallback = true; + None + } + }) + .collect(); // Compute paths let (install_path, cargo_roots, metadata, temp_dir) = @@ -183,79 +194,6 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) - }) } -/// Return (resolvers, cargo_install_fallback) -fn compute_resolvers( - mut strategies: Vec, - mut disable_strategies: Vec, -) -> Result<(Vec, bool), BinstallError> { - let dup_strategy_err = - BinstallError::InvalidStrategies("--strategies should not contain duplicate strategy"); - - if strategies.len() > Strategy::COUNT { - // If len of strategies is larger than number of variants of Strategy, - // then there must be duplicates by pigeon hole principle. - return Err(dup_strategy_err); - } - - // Whether specific variant of Strategy is present - let mut is_variant_present = [false; Strategy::COUNT]; - - for strategy in &strategies { - let index = *strategy as u8 as usize; - if is_variant_present[index] { - return Err(dup_strategy_err); - } else { - is_variant_present[index] = true; - } - } - - // Default strategies if empty - if strategies.is_empty() { - strategies = vec![ - Strategy::CrateMetaData, - Strategy::QuickInstall, - Strategy::Compile, - ]; - } - - // Filter out all disabled strategies - if !disable_strategies.is_empty() { - // Since order doesn't matter, we can sort it and remove all duplicates - // to speedup checking. - disable_strategies.sort_unstable(); - disable_strategies.dedup(); - - // disable_strategies.len() <= Strategy::COUNT, of which is faster - // to just use [T]::contains rather than [T]::binary_search - strategies.retain(|strategy| !disable_strategies.contains(strategy)); - - if strategies.is_empty() { - return Err(BinstallError::InvalidStrategies( - "You have disabled all strategies", - )); - } - } - - let cargo_install_fallback = *strategies.last().unwrap() == Strategy::Compile; - - if cargo_install_fallback { - strategies.pop().unwrap(); - } - - let resolvers = strategies - .into_iter() - .map(|strategy| match strategy { - Strategy::CrateMetaData => Ok(GhCrateMeta::new as Resolver), - Strategy::QuickInstall => Ok(QuickInstall::new as Resolver), - Strategy::Compile => Err(BinstallError::InvalidStrategies( - "Compile strategy must be the last one", - )), - }) - .collect::, BinstallError>>()?; - - Ok((resolvers, cargo_install_fallback)) -} - /// Return (install_path, cargo_roots, metadata, temp_dir) fn compute_paths_and_load_manifests( roots: Option, diff --git a/crates/bin/src/main.rs b/crates/bin/src/main.rs index 9940a933..ecf0f41b 100644 --- a/crates/bin/src/main.rs +++ b/crates/bin/src/main.rs @@ -18,10 +18,7 @@ fn main() -> MainExit { // This must be the very first thing to happen let jobserver_client = LazyJobserverClient::new(); - let args = match args::parse() { - Ok(args) => args, - Err(err) => return MainExit::Error(err), - }; + let args = args::parse(); if args.version { println!("{}", env!("CARGO_PKG_VERSION")); diff --git a/crates/binstalk/src/errors.rs b/crates/binstalk/src/errors.rs index 23a02a88..5d799a8b 100644 --- a/crates/binstalk/src/errors.rs +++ b/crates/binstalk/src/errors.rs @@ -205,28 +205,6 @@ pub enum BinstallError { )] SuperfluousVersionOption, - /// An override option is used when multiple packages are to be installed. - /// - /// This is raised when more than one package name is provided and any of: - /// - /// - `--version` - /// - `--manifest-path` - /// - `--bin-dir` - /// - `--pkg-fmt` - /// - `--pkg-url` - /// - /// is provided. - /// - /// - Code: `binstall::conflict::overrides` - /// - Exit: 85 - #[error("override option used with multi package syntax")] - #[diagnostic( - severity(error), - code(binstall::conflict::overrides), - help("You cannot use --{option} and specify multiple packages at the same time. Do one or the other.") - )] - OverrideOptionUsedWithMultiInstall { option: &'static str }, - /// No binaries were found for the crate. /// /// When installing, either the binaries are specified in the crate's Cargo.toml, or they're @@ -304,14 +282,6 @@ pub enum BinstallError { #[diagnostic(severity(error), code(binstall::SourceFilePath))] EmptySourceFilePath, - /// Invalid strategies configured. - /// - /// - Code: `binstall::strategies` - /// - Exit: 93 - #[error("Invalid strategies configured: {0}")] - #[diagnostic(severity(error), code(binstall::strategies))] - InvalidStrategies(&'static str), - /// Fallback to `cargo-install` is disabled. /// /// - Code: `binstall::no_fallback_to_cargo_install` @@ -344,7 +314,6 @@ impl BinstallError { VersionParse { .. } => 80, VersionMismatch { .. } => 82, SuperfluousVersionOption => 84, - OverrideOptionUsedWithMultiInstall { .. } => 85, UnspecifiedBinaries => 86, NoViableTargets => 87, BinFileNotFound(_) => 88, @@ -352,7 +321,6 @@ impl BinstallError { DuplicateSourceFilePath { .. } => 90, InvalidSourceFilePath { .. } => 91, EmptySourceFilePath => 92, - InvalidStrategies(..) => 93, NoFallbackToCargoInstall => 94, CrateContext(context) => context.err.exit_number(), };