-
Notifications
You must be signed in to change notification settings - Fork 489
Add cargo-binstall support and adopt Rust target triple naming #1390
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
base: master
Are you sure you want to change the base?
Add cargo-binstall support and adopt Rust target triple naming #1390
Conversation
eladyn
left a comment
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.
Thanks for the contribution! I added some minor comments. Once everything looks good, I'll give this a try on my fork. :)
On another note: Some of your changes seem a little AI-like, so I wanted to kindly ask you to disclose this and the extent to which this is true, if this is indeed the case. I realize that we currently don't have any contribution guidelines regarding AI usage, but I - just speaking for myself - would like to give this some thought before just accepting AI-generated contributions. If this is just false accusations, I'm very sorry in advance, and please don't take that as an offense!
| elif [ "${{ matrix.os }}" = "linux" ] && [ "${{ matrix.arch }}" = "x86_64" ]; then | ||
| TARGET_TRIPLE="x86_64-unknown-linux-gnu" | ||
| elif [ "${{ matrix.os }}" = "macos" ] && [ "${{ matrix.arch }}" = "x86_64" ]; then | ||
| TARGET_TRIPLE="x86_64-apple-darwin" | ||
| elif [ "${{ matrix.os }}" = "macos" ] && [ "${{ matrix.arch }}" = "aarch64" ]; then | ||
| TARGET_TRIPLE="aarch64-apple-darwin" |
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.
Wouldn't it be possible to just use rustc -vV to automatically determine the host target triple?
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.
You're absolutely right, using rustc -vV is cleaner and more reliable. I'll make that change. Thanks for the suggestion.
| Getting `spotifyd` on your system should be as easy as downloading a binary in most cases. | ||
| If you'd like to learn how to compile `spotifyd` yourself, head over to [building from source](./source.md). | ||
|
|
||
| ## Using cargo-binstall (Recommended) |
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 think, I'd rather not have cargo-binstall be the recommended installation method. Official packages by distributions will probably have better compatibility with the user's system and don't require a working rust / cargo installation...
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.
Yeah that makes sense, I’ll remove the “Recommended” wording and position cargo-binstall as an optional convenience method for users who already have Rust/Cargo installed
| - Example: `spotifyd-x86_64-unknown-linux-gnu-full.tar.gz` | ||
| **Legacy format** (deprecated, will be removed in next release): `spotifyd-{os}-{arch}-{variant}.tar.gz` | ||
| - Example: `spotifyd-linux-x86_64-full-legacy.tar.gz` |
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'm not entirely sure, but I think the code above will remove -legacy before publishing the release. And probably, if we want to keep a legacy version, this should keep the identical naming as previously.
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.
Yes you’re right, the packaging step removes the -legacy suffix, so legacy artifacts would not retain their original naming and this could lead to confusion or potential naming conflicts. I can remove the artifact_name=${artifact_name%-legacy line so legacy releases keep the -legacy suffix and preserve the previous naming format. Does that approach work for you?
| ["target/release/spotifyd", "usr/bin/", "755"], | ||
| ["README.md", "usr/share/doc/spotifyd/README", "644"], | ||
| ["contrib/spotifyd.service", "etc/systemd/user/", "644"], | ||
| [ |
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.
This is rather picky, but maybe we can keep the previous formatting of the Cargo.toml file. :)
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.
No problem at all, I'll revert to the original formatting. Consistency is important.
| [package.metadata.binstall.overrides.x86_64-pc-windows-msvc] | ||
| pkg-fmt = "zip" |
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 don't think we provide Windows binaries currently, what does this do?
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.
You're right, you don't currently provide Windows binaries, this is premature optimisation. I added it based on cargo-binstall's support matrix (https://github.com/cargo-bins/cargo-binstall/blob/main/SUPPORT.md), but I'm happy to remove it for now. We can add it back when/if Windows builds are introduced.
| Starting with version 0.4.2, release assets follow Rust's standard target triple naming format: | ||
|
|
||
| Format: `spotifyd-{target-triple}-{variant}.tar.gz` | ||
|
|
||
| Examples: | ||
| `spotifyd-x86_64-unknown-linux-gnu-full.tar.gz` | ||
| `spotifyd-aarch64-apple-darwin-default.tar.gz` | ||
| `spotifyd-armv7-unknown-linux-gnueabihf-slim.tar.gz` | ||
|
|
||
| **Legacy naming:** (deprecated, available only in version 0.4.2 for backwards compatibility) | ||
| `spotifyd-linux-x86_64-full-legacy.tar.gz` | ||
| `spotifyd-macos-aarch64-default-legacy.tar.gz` |
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.
As the current version is 0.4.2, this should probably be 0.4.3. (And before we forget to remove it later on, maybe we can just omit this part or at least the one about the legacy naming, what do you think?)
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 makes sense, since this is a one time transition, the version specific documentation and legacy naming section will become outdated quickly. I'll remove those parts to keep the docs cleaner and avoid future maintenance burden. Thanks for thinking ahead on this!
My apologies, yes, I used AI (Claude) to help refine the YAML changes and improve the README documentation. Specifically, it helped with:
I should have disclosed this upfront. Going forward, I'll be more transparent about AI assistance and ensure all contributions align with the project's guidelines. |
Summary
This PR adds support for
cargo-binstalland transitions release assets to use Rust's standard target triple naming convention.Changes
Updated
.github/workflows/cd.yml:Updated
Cargo.toml:package.metadata.binstallconfiguration pointing to thefullvariantUpdated installation documentation:
cargo-binstallas the recommended installation methodMigration Plan
Testing
Users will be able to install spotifyd with:
cargo binstall spotifydTesting Note
Full end-to-end testing of
cargo-binstallfunctionality requires a release with the new asset naming format. The workflow changes have been validated for syntax and logic, but actual installation testing will be possible after the first release with these changes.I'm happy to help verify the installation works once a test release is created.
Closes #1200