Fix: --strategies on CLI do not seem to override disabled-strategies in the manifest #1857
Conversation
Forward `args.disabled_strategies` Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Make sure both --strategies and --disable-strategies is tested Signed-off-by: Jiahao XU <[email protected]>
|
cc @ilyagr does this look good to you? |
Signed-off-by: Jiahao XU <[email protected]>
| ## 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] | ||
|
|
||
| ## Test --disable-strategies overriding `disabled-strategies=["compile"]` in Cargo.toml |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, maybe we should add a new option for this...
There was a problem hiding this comment.
cc @passcod I don't have much idea on how we could fix/improve this, do you have any idea/suggestion for this?
There was a problem hiding this comment.
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.
Fixed #1851