From 3f29fbe83a15cd56afbf4726841b280550f507f5 Mon Sep 17 00:00:00 2001 From: Jiahao XU <30436523+NobodyXu@users.noreply.github.com> Date: Wed, 24 Jul 2024 00:05:22 +1000 Subject: [PATCH] Feature: SupportdDisable of strategies for crate using `Cargo.toml` (#1828) * Refactor: Move `Strategy` to `binstalk-types` Signed-off-by: Jiahao XU * Add serialisation test for `Strategy` Signed-off-by: Jiahao XU * Add support to disable strategies via crate `Cargo.toml` Signed-off-by: Jiahao XU * Add e2e-test Signed-off-by: Jiahao XU * Fix `Cargo.toml` disabled strategy checking for compile strategy Signed-off-by: Jiahao XU * Optimize `resolve_inner`: Cache meta override Signed-off-by: Jiahao XU * Add compile-time length checking for `Strategy` Signed-off-by: Jiahao XU * More optimization Signed-off-by: Jiahao XU * Fix order of override: cli options alwayus takes precedence Signed-off-by: Jiahao XU * Add missing manifest for e2e-test Signed-off-by: Jiahao XU --------- Signed-off-by: Jiahao XU --- Cargo.lock | 1 + crates/bin/src/args.rs | 51 +++++++----- crates/bin/src/entry.rs | 13 ++- crates/binstalk-fetchers/src/gh_crate_meta.rs | 5 ++ crates/binstalk-fetchers/src/lib.rs | 5 +- crates/binstalk-fetchers/src/quickinstall.rs | 6 +- crates/binstalk-types/Cargo.toml | 3 + .../binstalk-types/src/cargo_toml_binstall.rs | 65 +++++++++++++++ crates/binstalk/src/ops/resolve.rs | 82 ++++++++++++------- .../manifests/strategies-test-Cargo.toml | 19 +++++ e2e-tests/strategies.sh | 13 +++ 11 files changed, 206 insertions(+), 57 deletions(-) create mode 100644 e2e-tests/manifests/strategies-test-Cargo.toml diff --git a/Cargo.lock b/Cargo.lock index 3ad0d864..040048c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -425,6 +425,7 @@ dependencies = [ "once_cell", "semver", "serde", + "serde_json", "strum", "strum_macros", "url", diff --git a/crates/bin/src/args.rs b/crates/bin/src/args.rs index e81f1970..061fbdab 100644 --- a/crates/bin/src/args.rs +++ b/crates/bin/src/args.rs @@ -13,12 +13,12 @@ use binstalk::{ ops::resolve::{CrateName, VersionReqExt}, registry::Registry, }; -use clap::{error::ErrorKind, CommandFactory, Parser, ValueEnum}; +use binstalk_manifests::cargo_toml_binstall::Strategy; +use clap::{builder::PossibleValue, error::ErrorKind, CommandFactory, Parser, ValueEnum}; use compact_str::CompactString; use log::LevelFilter; use semver::VersionReq; use strum::EnumCount; -use strum_macros::EnumCount; use zeroize::Zeroizing; #[derive(Debug, Parser)] @@ -162,13 +162,13 @@ pub struct Args { value_delimiter(','), env = "BINSTALL_STRATEGIES" )] - pub(crate) strategies: Vec, + pub(crate) strategies: Vec, /// Disable the strategies specified. /// If a strategy is specified in `--strategies` and `--disable-strategies`, /// then it will be removed. #[clap(help_heading = "Overrides", long, value_delimiter(','))] - pub(crate) disable_strategies: Vec, + pub(crate) disable_strategies: Vec, /// If `--github-token` or environment variable `GITHUB_TOKEN`/`GH_TOKEN` /// is not specified, then cargo-binstall will try to extract github token from @@ -431,16 +431,24 @@ impl Default for RateLimit { } /// Strategy for installing the package -#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, ValueEnum, EnumCount)] -#[repr(u8)] -pub(crate) enum Strategy { - /// Attempt to download official pre-built artifacts using - /// information provided in `Cargo.toml`. - CrateMetaData, - /// Query third-party QuickInstall for the crates. - QuickInstall, - /// Build the crates from source using `cargo-build`. - Compile, +#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] +pub(crate) struct StrategyWrapped(pub(crate) Strategy); + +impl StrategyWrapped { + const VARIANTS: &'static [Self; 3] = &[ + Self(Strategy::CrateMetaData), + Self(Strategy::QuickInstall), + Self(Strategy::Compile), + ]; +} + +impl ValueEnum for StrategyWrapped { + fn value_variants<'a>() -> &'a [Self] { + Self::VARIANTS + } + fn to_possible_value(&self) -> Option { + Some(PossibleValue::new(self.0.to_str())) + } } pub fn parse() -> Args { @@ -532,7 +540,7 @@ You cannot use --{option} and specify multiple packages at the same time. Do one let mut is_variant_present = [false; Strategy::COUNT]; for strategy in &opts.strategies { - let index = *strategy as u8 as usize; + let index = strategy.0 as u8 as usize; if is_variant_present[index] { new_dup_strategy_err().exit() } else { @@ -543,9 +551,9 @@ You cannot use --{option} and specify multiple packages at the same time. Do one // Default strategies if empty if opts.strategies.is_empty() { opts.strategies = vec![ - Strategy::CrateMetaData, - Strategy::QuickInstall, - Strategy::Compile, + StrategyWrapped(Strategy::CrateMetaData), + StrategyWrapped(Strategy::QuickInstall), + StrategyWrapped(Strategy::Compile), ]; } @@ -573,7 +581,8 @@ You cannot use --{option} and specify multiple packages at the same time. Do one } // Ensure that Strategy::Compile is specified as the last strategy - if opts.strategies[..(opts.strategies.len() - 1)].contains(&Strategy::Compile) { + if opts.strategies[..(opts.strategies.len() - 1)].contains(&StrategyWrapped(Strategy::Compile)) + { command .error( ErrorKind::InvalidValue, @@ -593,10 +602,14 @@ You cannot use --{option} and specify multiple packages at the same time. Do one #[cfg(test)] mod test { + use strum::VariantArray; + use super::*; #[test] fn verify_cli() { Args::command().debug_assert() } + + const _: () = assert!(Strategy::VARIANTS.len() == StrategyWrapped::VARIANTS.len()); } diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index 48288aa2..770c72fb 100644 --- a/crates/bin/src/entry.rs +++ b/crates/bin/src/entry.rs @@ -21,7 +21,9 @@ use binstalk::{ }, }; use binstalk_manifests::{ - cargo_config::Config, cargo_toml_binstall::PkgOverride, crates_manifests::Manifests, + cargo_config::Config, + cargo_toml_binstall::{PkgOverride, Strategy}, + crates_manifests::Manifests, }; use file_format::FileFormat; use home::cargo_home; @@ -30,11 +32,7 @@ use miette::{miette, Report, Result, WrapErr}; use tokio::task::block_in_place; use tracing::{debug, error, info, warn}; -use crate::{ - args::{Args, Strategy}, - gh_token, git_credentials, install_path, - ui::confirm, -}; +use crate::{args::Args, gh_token, git_credentials, install_path, ui::confirm}; pub fn install_crates( args: Args, @@ -46,7 +44,7 @@ pub fn install_crates( let resolvers: Vec<_> = args .strategies .into_iter() - .filter_map(|strategy| match strategy { + .filter_map(|strategy| match strategy.0 { Strategy::CrateMetaData => Some(GhCrateMeta::new as Resolver), Strategy::QuickInstall => Some(QuickInstall::new as Resolver), Strategy::Compile => { @@ -87,6 +85,7 @@ pub fn install_crates( pkg_url: args.pkg_url, pkg_fmt: args.pkg_fmt, bin_dir: args.bin_dir, + disabled_strategies: None, signing: None, }; diff --git a/crates/binstalk-fetchers/src/gh_crate_meta.rs b/crates/binstalk-fetchers/src/gh_crate_meta.rs index 51cd012c..d1df0637 100644 --- a/crates/binstalk-fetchers/src/gh_crate_meta.rs +++ b/crates/binstalk-fetchers/src/gh_crate_meta.rs @@ -1,6 +1,7 @@ use std::{borrow::Cow, fmt, iter, path::Path, sync::Arc}; use binstalk_git_repo_api::gh_api_client::{GhApiError, GhReleaseArtifact, GhReleaseArtifactUrl}; +use binstalk_types::cargo_toml_binstall::Strategy; use compact_str::{CompactString, ToCompactString}; use either::Either; use leon::Template; @@ -396,6 +397,10 @@ impl super::Fetcher for GhCrateMeta { FETCHER_GH_CRATE_META } + fn strategy(&self) -> Strategy { + Strategy::CrateMetaData + } + fn is_third_party(&self) -> bool { false } diff --git a/crates/binstalk-fetchers/src/lib.rs b/crates/binstalk-fetchers/src/lib.rs index 5d4f692a..49a86894 100644 --- a/crates/binstalk-fetchers/src/lib.rs +++ b/crates/binstalk-fetchers/src/lib.rs @@ -4,7 +4,7 @@ use std::{path::Path, sync::Arc, time::Duration}; use binstalk_downloader::{download::DownloadError, remote::Error as RemoteError}; use binstalk_git_repo_api::gh_api_client::{GhApiError, GhRepo, RepoInfo as GhRepoInfo}; -use binstalk_types::cargo_toml_binstall::SigningAlgorithm; +use binstalk_types::cargo_toml_binstall::{SigningAlgorithm, Strategy}; use thiserror::Error as ThisError; use tokio::{sync::OnceCell, task::JoinError, time::sleep}; pub use url::ParseError as UrlParseError; @@ -134,6 +134,9 @@ pub trait Fetcher: Send + Sync { /// [`Fetcher::fetch_and_extract`]. fn fetcher_name(&self) -> &'static str; + /// The strategy used by this fetcher + fn strategy(&self) -> Strategy; + /// Should return true if the remote is from a third-party source fn is_third_party(&self) -> bool; diff --git a/crates/binstalk-fetchers/src/quickinstall.rs b/crates/binstalk-fetchers/src/quickinstall.rs index 2862fb89..ad5d2f7d 100644 --- a/crates/binstalk-fetchers/src/quickinstall.rs +++ b/crates/binstalk-fetchers/src/quickinstall.rs @@ -5,7 +5,7 @@ use std::{ }; use binstalk_downloader::remote::Method; -use binstalk_types::cargo_toml_binstall::{PkgFmt, PkgMeta, PkgSigning}; +use binstalk_types::cargo_toml_binstall::{PkgFmt, PkgMeta, PkgSigning, Strategy}; use tokio::sync::OnceCell; use tracing::{error, info, trace}; use url::Url; @@ -252,6 +252,10 @@ by rust officially."#, "QuickInstall" } + fn strategy(&self) -> Strategy { + Strategy::QuickInstall + } + fn is_third_party(&self) -> bool { true } diff --git a/crates/binstalk-types/Cargo.toml b/crates/binstalk-types/Cargo.toml index 8560eb7c..54e5d0ed 100644 --- a/crates/binstalk-types/Cargo.toml +++ b/crates/binstalk-types/Cargo.toml @@ -18,3 +18,6 @@ serde = { version = "1.0.163", features = ["derive"] } strum = "0.26.1" strum_macros = "0.26.1" url = { version = "2.3.1", features = ["serde"] } + +[dev-dependencies] +serde_json = "1" diff --git a/crates/binstalk-types/src/cargo_toml_binstall.rs b/crates/binstalk-types/src/cargo_toml_binstall.rs index 2c9af131..e1ab03c6 100644 --- a/crates/binstalk-types/src/cargo_toml_binstall.rs +++ b/crates/binstalk-types/src/cargo_toml_binstall.rs @@ -5,6 +5,7 @@ use std::{borrow::Cow, collections::BTreeMap}; use serde::{Deserialize, Serialize}; +use strum_macros::{EnumCount, VariantArray}; mod package_formats; #[doc(inline)] @@ -19,6 +20,41 @@ pub struct Meta { pub binstall: Option, } +/// Strategies to use for binary discovery +#[derive( + Debug, + Copy, + Clone, + Eq, + PartialEq, + Ord, + PartialOrd, + EnumCount, + VariantArray, + Deserialize, + Serialize, +)] +#[serde(rename_all = "kebab-case")] +pub enum Strategy { + /// Attempt to download official pre-built artifacts using + /// information provided in `Cargo.toml`. + CrateMetaData, + /// Query third-party QuickInstall for the crates. + QuickInstall, + /// Build the crates from source using `cargo-build`. + Compile, +} + +impl Strategy { + pub const fn to_str(self) -> &'static str { + match self { + Strategy::CrateMetaData => "crate-meta-data", + Strategy::QuickInstall => "quick-install", + Strategy::Compile => "compile", + } + } +} + /// Metadata for binary installation use. /// /// Exposed via `[package.metadata]` in `Cargo.toml` @@ -37,6 +73,9 @@ pub struct PkgMeta { /// Package signing configuration pub signing: Option, + /// Stratgies to disable + pub disabled_strategies: Option>, + /// Target specific overrides pub overrides: BTreeMap, } @@ -82,10 +121,16 @@ impl PkgMeta { .or_else(|| self.bin_dir.clone()), signing: pkg_overrides + .clone() .into_iter() .find_map(|pkg_override| pkg_override.signing.clone()) .or_else(|| self.signing.clone()), + disabled_strategies: pkg_overrides + .into_iter() + .find_map(|pkg_override| pkg_override.disabled_strategies.clone()) + .or_else(|| self.disabled_strategies.clone()), + overrides: Default::default(), } } @@ -106,6 +151,9 @@ pub struct PkgOverride { /// Path template override for binary files in packages pub bin_dir: Option, + /// Stratgies to disable + pub disabled_strategies: Option>, + /// Package signing configuration pub signing: Option, } @@ -141,3 +189,20 @@ pub enum SigningAlgorithm { /// [minisign](https://jedisct1.github.io/minisign/) Minisign, } + +#[cfg(test)] +mod tests { + use strum::VariantArray; + + use super::*; + + #[test] + fn test_strategy_ser() { + Strategy::VARIANTS.iter().for_each(|strategy| { + assert_eq!( + serde_json::to_string(&strategy).unwrap(), + format!(r#""{}""#, strategy.to_str()) + ) + }); + } +} diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs index 5c6379d9..426e478b 100644 --- a/crates/binstalk/src/ops/resolve.rs +++ b/crates/binstalk/src/ops/resolve.rs @@ -8,7 +8,10 @@ use std::{ }; use binstalk_fetchers::FETCHER_GH_CRATE_META; -use binstalk_types::crate_info::{CrateSource, SourceType}; +use binstalk_types::{ + cargo_toml_binstall::Strategy, + crate_info::{CrateSource, SourceType}, +}; use compact_str::{CompactString, ToCompactString}; use itertools::Itertools; use leon::Template; @@ -90,8 +93,22 @@ async fn resolve_inner( .get() .await .iter() - .map(|target| TargetTriple::from_str(target).map(|triple| (triple, target))) - .collect::, _>>()?; + .map(|target| { + debug!("Building metadata for target: {target}"); + + let meta = package_info.meta.merge_overrides( + iter::once(&opts.cli_overrides).chain(package_info.overrides.get(target)), + ); + + debug!("Found metadata: {meta:?}"); + + Ok(Arc::new(TargetData { + target: target.clone(), + meta, + target_related_info: TargetTriple::from_str(target)?, + })) + }) + .collect::, BinstallError>>()?; let resolvers = &opts.resolvers; let binary_name = match package_info.binaries.as_slice() { @@ -115,32 +132,24 @@ async fn resolve_inner( handles.extend( resolvers .iter() - .cartesian_product(desired_targets.clone().into_iter().map( - |(triple, target)| { - debug!("Building metadata for target: {target}"); - - let target_meta = package_info.meta.merge_overrides( - iter::once(&opts.cli_overrides) - .chain(package_info.overrides.get(target)), - ); - - debug!("Found metadata: {target_meta:?}"); - - Arc::new(TargetData { - target: target.clone(), - meta: target_meta, - target_related_info: triple, - }) - }, - )) + .cartesian_product(&desired_targets) .filter_map(|(f, target_data)| { let fetcher = f( opts.client.clone(), gh_api_client.clone(), data.clone(), - target_data, + target_data.clone(), opts.signature_policy, ); + + if let Some(disabled_strategies) = + target_data.meta.disabled_strategies.as_deref() + { + if disabled_strategies.contains(&fetcher.strategy()) { + return None; + } + } + filter_fetcher_by_name_predicate(fetcher.fetcher_name()).then_some(fetcher) }), ) @@ -231,14 +240,29 @@ async fn resolve_inner( } } - if opts.cargo_install_fallback { - Ok(Resolution::InstallFromSource(ResolutionSource { - name: package_info.name, - version: package_info.version_str, - })) - } else { - Err(BinstallError::NoFallbackToCargoInstall) + if !opts.cargo_install_fallback { + return Err(BinstallError::NoFallbackToCargoInstall); } + + let meta = package_info + .meta + .merge_overrides(iter::once(&opts.cli_overrides)); + + let target_meta = desired_targets + .first() + .map(|target_data| &target_data.meta) + .unwrap_or(&meta); + + if let Some(disabled_strategies) = target_meta.disabled_strategies.as_deref() { + if disabled_strategies.contains(&Strategy::Compile) { + return Err(BinstallError::NoFallbackToCargoInstall); + } + } + + Ok(Resolution::InstallFromSource(ResolutionSource { + name: package_info.name, + version: package_info.version_str, + })) } /// * `fetcher` - `fetcher.find()` must have returned `Ok(true)`. diff --git a/e2e-tests/manifests/strategies-test-Cargo.toml b/e2e-tests/manifests/strategies-test-Cargo.toml new file mode 100644 index 00000000..f4235a84 --- /dev/null +++ b/e2e-tests/manifests/strategies-test-Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "cargo-update" +repository = "https://github.com/nabijaczleweli/cargo-update" +version = "11.1.2" + +[[bin]] +name = "cargo-install-update" +path = "src/main.rs" +test = false +doc = false + +[[bin]] +name = "cargo-install-update-config" +path = "src/main-config.rs" +test = false +doc = false + +[package.metadata.binstall] +disabled-strategies = ["quick-install", "compile"] diff --git a/e2e-tests/strategies.sh b/e2e-tests/strategies.sh index 1a91c894..0248d6c3 100755 --- a/e2e-tests/strategies.sh +++ b/e2e-tests/strategies.sh @@ -36,3 +36,16 @@ fi ## Test compile-only strategy "./$1" binstall --no-confirm --strategies compile cargo-quickinstall@0.2.8 + +## Test --disable-strategies +set +e + +"./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-Cargo.toml" cargo-update@11.1.2 +exit_code="$?" + +set -e + +if [ "$exit_code" != 94 ]; then + echo "Expected exit code 94, but actual exit code $exit_code" + exit 1 +fi