-
Notifications
You must be signed in to change notification settings - Fork 149
Fixes for rust 1.90 #1626
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
Fixes for rust 1.90 #1626
Conversation
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.
Code Review
This pull request introduces a set of changes to ensure compatibility with Rust 1.90. The modifications include updating the Minimum Supported Rust Version (MSRV) to 1.84.0, removing dead code identified by the new compiler version, and fixing new warnings about unnecessary parentheses. The changes are correct, well-contained, and improve the overall health of the codebase. This is a solid maintenance update.
|
Hm errors seem like fallout from #1619 ? |
|
|
||
| #[derive(Serialize, Deserialize, Debug, PartialEq, Eq)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub(crate) enum VerityState { |
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.
Edited to add a Closes: #1605
| # In general we try to keep this pinned to what's in the latest RHEL9. | ||
| # However right now, we bumped to 1.82 as that's what composefs-rs uses. | ||
| rust-version = "1.82.0" | ||
| rust-version = "1.84.0" |
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.
What's in c9s now is 1.88 so we can bump to that; what made you choose 1.84?
I guess really this should be updated by renovate.
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.
Oh sorry it's in the commit message. Yeah fine as is.
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 asked brew what was in rhel-9.6.0 to be conservative
Yes, though not obvious to me how that PR could have merged but this one fails |
FWIW I can reproduce the CI errors on main with |
|
This will just need rebasing once #1628 lands otherwise LGTM |
Signed-off-by: John Eckersberg <[email protected]>
Signed-off-by: John Eckersberg <[email protected]>
We already required at least 1.83.0 since we're using `Option::unwrap` in const context in a few places, but rust-1.90 now correctly points this out and emits an error. RHEL 9.6 is on 1.84.0 so bump to match that. Signed-off-by: John Eckersberg <[email protected]>
Closes: #1605