Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 21 commits into from
Aug 3, 2024
Merged

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

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1801932
Fix cli override in entry.rs
NobodyXu Aug 3, 2024
aacc9e3
Fix `args::parse`: Do not free disabled_strategies
NobodyXu Aug 3, 2024
1cc7c7a
Fix passing of cli_overrides
NobodyXu Aug 3, 2024
7c810c2
Create strategies-test-override-Cargo.toml
NobodyXu Aug 3, 2024
6528b8c
Add e2e-tests for cli-overrides
NobodyXu Aug 3, 2024
2481a74
Fix entry.rs
NobodyXu Aug 3, 2024
75a8f99
fix entry.rs
NobodyXu Aug 3, 2024
6c74c1c
Fix entry.rs
NobodyXu Aug 3, 2024
85f244f
Update strategies.sh
NobodyXu Aug 3, 2024
8f21c16
Compute cli_overrides in args::parse
NobodyXu Aug 3, 2024
91ee049
fix use of args::parse main_impl.rs
NobodyXu Aug 3, 2024
a9b1163
Update entry.rs
NobodyXu Aug 3, 2024
3a560cc
Fix args::parse
NobodyXu Aug 3, 2024
f2f7ddc
Fix typo in args.rs
NobodyXu Aug 3, 2024
73303aa
Fix args.rs
NobodyXu Aug 3, 2024
0437171
Fix fmt in args.rs
NobodyXu Aug 3, 2024
730e469
fix fmt in main_impl.rs
NobodyXu Aug 3, 2024
77e90ec
update e2e-test-strategies
NobodyXu Aug 3, 2024
d1e9380
Update e2e-tests/strategies.sh
NobodyXu Aug 3, 2024
88266db
Update e2e-tests/strategies.sh
NobodyXu Aug 3, 2024
25024bb
Update strategies.sh
NobodyXu Aug 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions crates/bin/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"]
Expand Down Expand Up @@ -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![
Expand Down Expand Up @@ -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
Expand All @@ -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)]
Expand Down
10 changes: 1 addition & 9 deletions crates/bin/src/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
5 changes: 3 additions & 2 deletions crates/bin/src/main_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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:?}");
Expand Down
13 changes: 13 additions & 0 deletions e2e-tests/manifests/strategies-test-override-Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"]
10 changes: 9 additions & 1 deletion e2e-tests/strategies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fi
## Test compile-only strategy
"./$1" binstall --no-confirm --strategies compile [email protected]

## Test --disable-strategies
## Test Cargo.toml disable-strategies
set +e

"./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-Cargo.toml" [email protected]
Expand All @@ -49,3 +49,11 @@ if [ "$exit_code" != 94 ]; then
echo "Expected exit code 94, but actual exit code $exit_code"
exit 1
fi

NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
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 [email protected]
NobodyXu marked this conversation as resolved.
Show resolved Hide resolved

## Test --disable-strategies overriding `disabled-strategies=["compile"]` in Cargo.toml
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only glanced at this so far, but my first instinct would be to expect the CLI --disable-strategies to only ever decrease the set of allowed strategies. So, I would make it so the situation of this test disallow all strategies.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current implementation might makes sense?

After all cli options can overrides anything from the maintainer, previously we have pkg-url, pkg-bin and pkg-fmt which overrides the maintainers' configurations.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think disabled-strategies should be treated more like specifying both on the same command-line.

Here's a scenario: let's say the maintainer disabled quick-install, but the user doesn't know about it. Let's say Github downloads are temporarily down for whatever reason. The user runs cargo binstall package, it uses compile, and they don't like it. So, one natural thing to try would be to interrupt that and try cargo binstall --disable-strategies compile.

I think that it wouldn't be great if this installed the package from quickinstall without the user realizing that he's getting a different thing than the GitHub download they usually gets. I think the "good" outcome here would be if binstall told the user that it can't download the package from GitHub, and thus can't install anything. (It could also tell the user that quick-install was disabled by maintainer, and to use --strategies quick-install if the user wants to override that).

I don't think this scenario would be very common, but it's enough to form my opinion of what behavior is a bit better. Either way, we should make sure that what happens is documented.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe we should add a new option for this...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @passcod I don't have much idea on how we could fix/improve this, do you have any idea/suggestion for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good enough to do this:

[merge disable strategies so that] binstall tells the user that it can't download the package from GitHub, and thus can't install anything.

and make sure that a positive (eg. enable quickinstall) overrides a negative (eg disable quickinstall) regardless of source (cli or maintainer).

I don't think it's worth having a message for the user until we get feedback to that effect, given this should be pretty rare.

"./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-override-Cargo.toml" --strategies compile --disable-strategies crate-meta-data,quick-install [email protected]
NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
Loading