-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add fallback input option to ensure that fallback is not used #517
Merged
+33
−1
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest
INPUT_STRATEGIES
and pass it directly tocargo-binstall
.User can specify
crate-meta-data
for trying to download it from official maintainer,quick-install
from third-party quickinstall,compile
for from source.If empty, then disable cargo-binstall
https://github.com/cargo-bins/cargo-binstall/blob/48ee0b0e3e646f7f0cfb2428f46dbcd32afe979b/crates/bin/src/args.rs#L418
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting approval for cargo-binstall would be worth attempting IMO if its cargo-quickinstall was disabled.
i.e. trying to explain to my cyber-security team that we're using install-action which falls back to cargo-binstall which falls back to cargo-quickinstall is too hard, and it is that cargo-quickinstall stage which becomes "oh its just grabbing some binary from somewhere on the internet, but they are good people running it, trust them". The fall back to cargo-binstall is explainable if it is "it fetches from github releases or it builds from source 100%"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good to hear, disabling quickinstall is pretty easy, you just need to pass
--strategies crate-meta-data,compile
and it will only use the prebuilt from the official repository specified inCargo.toml
on https://crates.io , or fallback tocargo-install
.--strategies compile
would then be equivalent to launchingcargo-install
, thoughcargo-binstall
would launch multiplecargo-install
for each crate to compile them in parallel, with a jobserver to limit parallelism.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, for a reviewer who is not familiar with the details of cargo-binstall's API but needs to review code that uses this action, something like
strategies: ''
/strategies: crate-meta-data,compile
is not at all clear at first glance. (compared to something likefallback: none
/fallback: binstall-first-party
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, maybe we can pass
--disable-strategies
instead.--disable-strategies quick-install
is equivalent to--strategies crate-meta-data,compile
, and it's much more straight forward for people just wanting to disable quickinstall.