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

Specify minimum rust version #1127

Merged
merged 10 commits into from
Nov 23, 2022
Merged

Specify minimum rust version #1127

merged 10 commits into from
Nov 23, 2022

Conversation

slondr
Copy link
Member

@slondr slondr commented Oct 13, 2022

Resolves #1126

@slondr slondr self-assigned this Oct 13, 2022
@slondr slondr requested a review from a team October 13, 2022 01:16
robinvd
robinvd previously approved these changes Oct 13, 2022
@eladyn
Copy link
Member

eladyn commented Oct 13, 2022

Cool, didn't know that. LGTM! I wonder if we could also pin that version in CI (not CD) so that we notice, when we need to bump the MSRV.

Edit: See for example how librespot is doing it.

@slondr
Copy link
Member Author

slondr commented Oct 13, 2022

Good idea @eladyn. I've added that to this MR.

@slondr slondr requested review from a team and robinvd October 13, 2022 22:08
@slondr
Copy link
Member Author

slondr commented Oct 13, 2022

Oh wait, that broke everything.

@slondr
Copy link
Member Author

slondr commented Oct 13, 2022

Ok, we're good now.

Comment on lines 59 to 60
toolchain:
- "1.62"
Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't be needed, since you aren't using ${{ matrix.toolchain }} anywhere.

@Plecra
Copy link
Contributor

Plecra commented Nov 22, 2022

Fantastic, LGTM :) These are almost the exact changes I made in response to #1123 , which this also fixes (and im going to sneakily link to this now ;)) I'm going to make some small changes to CI to ensure we're still getting the most recent lints

This improves the error reporting when the crate is
compiled with older versions of rustc.

Specifically, #1123 tracks instances of Ubuntu rustc
giving obscure errors when users attempt to compile
spotifyd

With this change, this error becomes

> error: package `spotifyd v0.3.3 (...)` cannot be built because it requires rustc 1.62 or newer, while the currently active rustc version is 1.61.0

which is much more helpful to users diagnosinig the issue.
Copy link
Contributor

@Plecra Plecra left a comment

Choose a reason for hiding this comment

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

assuming the CI is happy, lets get this done! It'd be nice to have a landing page in the documentation for the error ubuntu users will get:

error: package `spotifyd v0.3.3 (...)` cannot be built because it requires rustc 1.62 or newer,
while the currently active rustc version is 1.61.0

@Plecra
Copy link
Contributor

Plecra commented Nov 22, 2022

agh, clippy 🙄 fine, will make a quick PR for those

@Plecra Plecra mentioned this pull request Nov 22, 2022
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up!

@eladyn eladyn merged commit 1a1997c into master Nov 23, 2022
@slondr slondr deleted the msrv branch March 3, 2023 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formally specifcy MSRV
4 participants