-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
crates/web5/src/dids/did.rs
Outdated
use std::sync::LazyLock; | ||
|
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.
use std::sync::LazyLock; |
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 the Cargo.toml
already enforce that? Like if I have 1.80.0 in my local env, then I set the rust-version
to 1.70.0 in my Cargo.toml
and then I use a LazyLock
in non-test code, and I run cargo 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.
Like if I have 1.80.0 in my local env, then I set the rust-version to 1.70.0 in my Cargo.toml and then I use a LazyLock in non-test code, and I run cargo build will it fail?
It will not. It's only after the MSRV check that it would get flagged.
we should just full commit to whatever our MSRV is
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 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.
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
Closing this since we hashed it out in #303 |
Addresses #293
Set up github action to verify MSRV.
We have a few dependencies, including
tokio
with MSRV 1.70.0, so bringing our MSRV below 1.70.0 will be more difficult.