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.

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

--strategies on CLI do not seem to override disabled-strategies in the manifest #1851

Closed
ilyagr opened this issue Jul 28, 2024 · 2 comments · Fixed by #1857
Closed

--strategies on CLI do not seem to override disabled-strategies in the manifest #1851

ilyagr opened this issue Jul 28, 2024 · 2 comments · Fixed by #1857

Comments

@ilyagr
Copy link

ilyagr commented Jul 28, 2024

We discussed in #1830 (comment) that they are supposed to.

I might be missing something, but the code didn't seem to do that. I then added the following to strategies.sh, and the test seems to fail.

(The diff also fixes a comment) (Update: I made a draft PR for this diff, because why not. #1852)

diff --git a/e2e-tests/strategies.sh b/e2e-tests/strategies.sh
index 0248d6c378..7708afc852 100755
--- a/e2e-tests/strategies.sh
+++ b/e2e-tests/strategies.sh
@@ -37,7 +37,7 @@
 ## Test compile-only strategy
 "./$1" binstall --no-confirm --strategies compile [email protected]
 
-## Test --disable-strategies
+## Test `disabled-strategies=["compile", "quick-install"]` in Cargo.toml
 set +e
 
 "./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-Cargo.toml" [email protected]
@@ -49,3 +49,6 @@
     echo "Expected exit code 94, but actual exit code $exit_code"
     exit 1
 fi
+
+## Test --strategies overriding `disabled-strategies=["compile"]` in Cargo.toml
+"./$1" binstall --no-confirm --manifest-path "manifests/strategies-test-Cargo.toml" --strategies compile [email protected]

The last lines of the test were:

+ ./cargo-binstall binstall --no-confirm --manifest-path manifests/strategies-test-Cargo.toml --strategies compile [email protected]
 INFO resolve: Resolving package: 'cargo-update@=11.1.2'
ERROR Fatal error:
  × For crate cargo-update: Fallback to cargo-install is disabled
  ╰─▶ Fallback to cargo-install is disabled

error: Recipe `e2e-test` failed on line 234 with exit code 94

(I came up upon this as I was thinking about adding some more user-facing messaging about the strategy being disabled by the maintainer)


Another aside: how is one supposed to run tests? I had to cargo build and then symlink target/debug to target/aarch64-apple-darwin/debug before just e2e-test-strategies worked. I'm guessing there's a better way?

@NobodyXu
Copy link
Member

how is one supposed to run tests?

just build
just e2e-tests

In justfile, we also provide a target for each target, so you can run it after just build

@NobodyXu
Copy link
Member

disabled_strategies: None,

It turns out I forgot to forward it to resolve, which passes an iterator of cli_overrides, target_meta overrides to

pub fn merge_overrides<'a, It>(&self, pkg_overrides: It) -> Self

Since I set it to None it fallbacks to target_meta (e.g. package.binstall.x86_64_...) then the one found in package.binstall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants