Feature: SupportdDisable of strategies for crate using Cargo.toml (#1828)

* Refactor: Move `Strategy` to `binstalk-types`

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

* Add serialisation test for `Strategy`

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

* Add support to disable strategies via crate `Cargo.toml`

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

* Add e2e-test

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

* Fix `Cargo.toml` disabled strategy checking for compile strategy

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

* Optimize `resolve_inner`: Cache meta override

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

* Add compile-time length checking for `Strategy`

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

* More optimization

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

* Fix order of override: cli options alwayus takes precedence

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

* Add missing manifest for e2e-test

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

---------

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

Cargo.lock

@@ -425,6 +425,7 @@ dependencies = [
  "once_cell",
  "semver",
  "serde",
+ "serde_json",
  "strum",
  "strum_macros",
  "url",

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<Strategy>,
+    pub(crate) strategies: Vec<StrategyWrapped>,
 
     /// 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<Strategy>,
+    pub(crate) disable_strategies: Vec<StrategyWrapped>,
 
     /// 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<PossibleValue> {
+        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());
 }

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,
     };
 

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
     }

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;
 

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
     }

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"

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<PkgMeta>,
 }
 
+/// 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<PkgSigning>,
 
+    /// Stratgies to disable
+    pub disabled_strategies: Option<Box<[Strategy]>>,
+
     /// Target specific overrides
     pub overrides: BTreeMap<String, PkgOverride>,
 }
@@ -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<String>,
 
+    /// Stratgies to disable
+    pub disabled_strategies: Option<Box<[Strategy]>>,
+
     /// Package signing configuration
     pub signing: Option<PkgSigning>,
 }
@@ -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())
+            )
+        });
+    }
+}

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::<Result<Vec<_>, _>>()?;
+        .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::<Result<Vec<_>, 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 {
+    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,
     }))
-    } else {
-        Err(BinstallError::NoFallbackToCargoInstall)
-    }
 }
 
 ///  * `fetcher` - `fetcher.find()` must have returned `Ok(true)`.

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"]

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