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

feat: add cargo binstall support #3578

Merged
merged 3 commits into from
Jul 5, 2023
Merged

feat: add cargo binstall support #3578

merged 3 commits into from
Jul 5, 2023

Conversation

QEDK
Copy link
Contributor

@QEDK QEDK commented Jul 4, 2023

Adds binary installation support via binstall (https://github.com/cargo-bins/cargo-binstall).

@QEDK QEDK requested a review from gakonst as a code owner July 4, 2023 15:18
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

defer to @onbjerg here

but if we do that, we should also mention this option in the README

@mattsse mattsse added the C-enhancement New feature or request label Jul 4, 2023
@QEDK
Copy link
Contributor Author

QEDK commented Jul 4, 2023

defer to @onbjerg here

but if we do that, we should also mention this option in the README

Typically, it just works if the crate conforms to binstall expectations, but the current convention is a bit non-conformant (not necessarily wrong), will update the README as well.

@QEDK
Copy link
Contributor Author

QEDK commented Jul 4, 2023

Updated book with binstall instructions.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

thanks, lgtm

pending @onbjerg

@gakonst
Copy link
Member

gakonst commented Jul 5, 2023

Not sure that we want this? Seems like it adds more ways to install which seems redundant?

@QEDK
Copy link
Contributor Author

QEDK commented Jul 5, 2023

Not sure that we want this? Seems like it adds more ways to install which seems redundant?

binstall is the de-facto crate used for install Rust binaries, for e.g. see cargo watch (https://github.com/watchexec/cargo-watch). Imo, this would greatly improve the UX, because you just need to install binstall once and each upgrade you can just run cargo binstall reth and it would be upgraded and installed automatically.

@onbjerg
Copy link
Member

onbjerg commented Jul 5, 2023

As long as we don't have to do anything special for this to work, it's ok to add I guess. Imo cargo binstall is more for Rust utilities and Cargo plugins, so I don't see the big benefit here, but OTOH there isn't really a downside either (again, as long as we don't have to do anything special to make this work)

book/installation/binaries.md Outdated Show resolved Hide resolved
@onbjerg onbjerg merged commit 706fc41 into paradigmxyz:main Jul 5, 2023
onbjerg added a commit that referenced this pull request Jul 5, 2023
@onbjerg
Copy link
Member

onbjerg commented Jul 5, 2023

@QEDK I accidentally fast merged it because I thought CI was tripping but I had to revert it. Please open up the PR again; this is not working because of your change to Cargo.toml. You can't add the package.metadata section since it would require package.name as well, but this is not valid in a workspace context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants