From ee94b8b6396783e613662a1175b2469518b1180d Mon Sep 17 00:00:00 2001
From: Jiahao XU <30436523+NobodyXu@users.noreply.github.com>
Date: Sat, 3 Aug 2024 15:27:23 +1000
Subject: [PATCH] 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 | 27 ++++++++++++++-----
crates/bin/src/entry.rs | 10 +------
crates/bin/src/main_impl.rs | 5 ++--
.../strategies-test-override-Cargo.toml | 13 +++++++++
e2e-tests/strategies.sh | 10 ++++++-
5 files changed, 47 insertions(+), 18 deletions(-)
create mode 100644 e2e-tests/manifests/strategies-test-override-Cargo.toml
diff --git a/crates/bin/src/args.rs b/crates/bin/src/args.rs
index 195f25f1..cb0aa65a 100644
--- a/crates/bin/src/args.rs
+++ b/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)]
diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs
index e923d97f..4a0c446f 100644
--- a/crates/bin/src/entry.rs
+++ b/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;
diff --git a/crates/bin/src/main_impl.rs b/crates/bin/src/main_impl.rs
index 14a3dbd8..2c78960b 100644
--- a/crates/bin/src/main_impl.rs
+++ b/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:?}");
diff --git a/e2e-tests/manifests/strategies-test-override-Cargo.toml b/e2e-tests/manifests/strategies-test-override-Cargo.toml
new file mode 100644
index 00000000..97127671
--- /dev/null
+++ b/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"]
diff --git a/e2e-tests/strategies.sh b/e2e-tests/strategies.sh
index 0248d6c3..583d8d14 100755
--- a/e2e-tests/strategies.sh
+++ b/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