Fix: --strategies on CLI do not seem to override disabled-strategies in the manifest (#1857)

* Fix cli override in entry.rs

Forward `args.disabled_strategies`

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

* Fix `args::parse`: Do not free disabled_strategies

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

* Fix passing of cli_overrides

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

* Create strategies-test-override-Cargo.toml

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

* Add e2e-tests for cli-overrides

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 entry.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>

* Update strategies.sh

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

* Compute cli_overrides in args::parse

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

* fix use of args::parse main_impl.rs

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

* Update entry.rs

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

* Fix args::parse

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

* Fix typo in args.rs

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

* Fix args.rs

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

* Fix fmt in args.rs

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

* fix fmt in main_impl.rs

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

* update e2e-test-strategies

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

* Update e2e-tests/strategies.sh

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

* Update e2e-tests/strategies.sh

Make sure both --strategies and --disable-strategies is tested

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

* Update strategies.sh

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/args.rs

@@ -13,7 +13,7 @@ use binstalk::{
     ops::resolve::{CrateName, VersionReqExt},
     registry::Registry,
 };
-use binstalk_manifests::cargo_toml_binstall::Strategy;
+use binstalk_manifests::cargo_toml_binstall::{PkgOverride, Strategy};
 use clap::{builder::PossibleValue, error::ErrorKind, CommandFactory, Parser, ValueEnum};
 use compact_str::CompactString;
 use log::LevelFilter;
@@ -464,7 +464,7 @@ impl ValueEnum for StrategyWrapped {
     }
 }
 
-pub fn parse() -> Args {
+pub fn parse() -> (Args, PkgOverride) {
     // 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"]
@@ -561,6 +561,8 @@ You cannot use --{option} and specify multiple packages at the same time. Do one
         }
     }
 
+    let has_strategies_override = !opts.strategies.is_empty();
+
     // Default strategies if empty
     if opts.strategies.is_empty() {
         opts.strategies = vec![
@@ -588,9 +590,6 @@ You cannot use --{option} and specify multiple packages at the same time. Do one
                 .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
@@ -614,7 +613,23 @@ You cannot use --{option} and specify multiple packages at the same time. Do one
         _ => (),
     }
 
-    opts
+    let cli_overrides = PkgOverride {
+        pkg_url: opts.pkg_url.take(),
+        pkg_fmt: opts.pkg_fmt.take(),
+        bin_dir: opts.bin_dir.take(),
+        disabled_strategies: (!opts.disable_strategies.is_empty() || has_strategies_override).then(
+            || {
+                opts.disable_strategies
+                    .iter()
+                    .map(|strategy| strategy.0)
+                    .collect::<Vec<_>>()
+                    .into_boxed_slice()
+            },
+        ),
+        signing: None,
+    };
+
+    (opts, cli_overrides)
 }
 
 #[cfg(test)]

crates/bin/src/entry.rs

@@ -36,6 +36,7 @@ use crate::{args::Args, gh_token, git_credentials, install_path, ui::confirm};
 
 pub fn install_crates(
     args: Args,
+    cli_overrides: PkgOverride,
     jobserver_client: LazyJobserverClient,
 ) -> Result<Option<AutoAbortJoinHandle<Result<()>>>> {
     // Compute Resolvers
@@ -80,15 +81,6 @@ pub fn install_crates(
     // Launch target detection
     let desired_targets = get_desired_targets(args.targets);
 
-    // Computer cli_overrides
-    let cli_overrides = PkgOverride {
-        pkg_url: args.pkg_url,
-        pkg_fmt: args.pkg_fmt,
-        bin_dir: args.bin_dir,
-        disabled_strategies: None,
-        signing: None,
-    };
-
     // Initialize reqwest client
     let rate_limit = args.rate_limit;
 

crates/bin/src/main_impl.rs

@@ -15,7 +15,7 @@ pub fn do_main() -> impl Termination {
     // This must be the very first thing to happen
     let jobserver_client = LazyJobserverClient::new();
 
-    let args = args::parse();
+    let (args, cli_overrides) = args::parse();
 
     if args.version {
         let cargo_binstall_version = env!("CARGO_PKG_VERSION");
@@ -54,7 +54,8 @@ rustc-llvm-version: {rustc_llvm_version}"#
 
         let start = Instant::now();
 
-        let result = run_tokio_main(|| entry::install_crates(args, jobserver_client));
+        let result =
+            run_tokio_main(|| entry::install_crates(args, cli_overrides, jobserver_client));
 
         let done = start.elapsed();
         debug!("run time: {done:?}");

e2e-tests/manifests/strategies-test-override-Cargo.toml

@@ -0,0 +1,13 @@
+[package]
+name = "cargo-quickinstall"
+repository = "https://github.com/cargo-bins/cargo-quickinstall"
+version = "0.2.10"
+
+[[bin]]
+name = "cargo-quickinstall"
+path = "src/main.rs"
+test = false
+doc = false
+
+[package.metadata.binstall]
+disabled-strategies = ["crate-meta-data", "quick-install", "compile"]

e2e-tests/strategies.sh

@@ -37,7 +37,7 @@ fi
 ## Test compile-only strategy
 "./$1" binstall --no-confirm --strategies compile cargo-quickinstall@0.2.8
 
-## Test --disable-strategies
+## Test Cargo.toml disable-strategies
 set +e
 
 "./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-Cargo.toml" cargo-update@11.1.2
@@ -49,3 +49,11 @@ if [ "$exit_code" != 94 ]; then
     echo "Expected exit code 94, but actual exit code $exit_code"
     exit 1
 fi
+
+set -euxo pipefail
+
+## Test --strategies overriding `disabled-strategies=["compile"]` in Cargo.toml
+ "./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-override-Cargo.toml" --strategies compile cargo-quickinstall@0.2.10
+
+## Test --disable-strategies overriding `disabled-strategies=["compile"]` in Cargo.toml
+ "./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-override-Cargo.toml" --disable-strategies crate-meta-data,quick-install --force cargo-quickinstall@0.2.10