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.
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
#293 Set MSRV to 1.70.0 #298
#293 Set MSRV to 1.70.0 #298
Changes from 3 commits
d168e8f
bd1dc09
c46111f
e8f5667
774487f
a4f23df
4088c18
90ed3c4
93a731a
1bc1fd0
1fb9260
2f780b9
8fc8b0e
bec796e
554a974
b6fc562
aa422f5
22fd4a1
a6aef12
af762e2
83e83a9
120525c
762fe21
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Tests don't affect MSRV
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 interesting, this is still used? was it in preview for 1.70.0 or something and so it's okay inside of tests?
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.
we commented at the same time
so but even though our rustup is set to 1.70.0 in our Justfile, this still works?
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.
Good catch. Updated Justfile back to 1.80.0 and we'll just have the MSRV check run separately. Makes for an easier dev environment for the places where we don't need to worry about MSRV
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 kind of feel like we should just full commit to whatever our MSRV is, even in our local dev env, such that we don't develop only to find out that we have an incompatible feature until the CI pipeline is ran. Or would the
rust-version
property in theCargo.toml
already enforce that? Like if I have 1.80.0 in my local env, then I set therust-version
to 1.70.0 in myCargo.toml
and then I use aLazyLock
in non-test code, and I runcargo build
will it fail?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.
It will not. It's only after the MSRV check that it would get flagged.
Word, let me try it
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.
We install packages during CI that require above 1.70.0. If we want to run everything on 1.70.0, not just the web5 crate where it is useful to lower MSRV, then we'll need to refactor those aspects of the CI and remove LazyLock from our tests. @KendallWeihe do you still want to use 1.70.0 for the entire codebase?
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.
Another option is to use
crates/web5/rust-toolchain.toml
to build and test the web5 crate with rustc 1.70.0, while using 1.80.0 in the rest of the codebase. rustc 1.70.0 will only be used when the current directory is in the crates/web5/ directory.Edit: in this case, we'll need to remove
LazyLock
from our testsThere 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 my proposal is we go with 1.74.0 for right now #303
And then let's open a ticket on the backlog to see if we can push it further lower at a later date