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.

Sign up for GitHub

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

Jump to bottom

#293 Set MSRV to 1.70.0 #298

Closed
wants to merge 23 commits into from
Closed

#293 Set MSRV to 1.70.0 #298

wants to merge 23 commits into from

Conversation

diehuxx
Copy link
Contributor

@diehuxx diehuxx commented Aug 20, 2024

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.

KendallWeihe
KendallWeihe reviewed Aug 20, 2024
View reviewed changes
crates/web5/src/dids/did.rs Outdated
Comment on lines 114 to 115
use std::sync::LazyLock;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use std::sync::LazyLock;

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

@diehuxx diehuxx Aug 21, 2024 •
edited
Loading

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

Copy link
Contributor

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

@KendallWeihe KendallWeihe mentioned this pull request Aug 20, 2024
Open
@diehuxx
Use 1.70.0 everywhere
8fc8b0e
@diehuxx
Use rust-toolchain.toml to set 1.70.0 just in web5 crate
bec796e
@diehuxx
Scope MSRV check back to web5 crate
554a974
@diehuxx
Update just test and just build to use both rustc versions
b6fc562
@diehuxx
1.74.0 through the repo
aa422f5
@diehuxx
1.75.0
22fd4a1
@diehuxx
Merge branch 'main' into msrv-1.70.0
a6aef12 
* main:
  Use error for Verifier::verify(), Add ed25519 unit tests, Add Web5Error::Crypto (#300)
  Test Secp256k1Generator (#302)
  Generate X25519 keys (#301)
  Support X25519 in did:dht verification method (#299)
  add sign and verify test vectors (#292)
  Add feature complete Jwk (#294)
@diehuxx
Copy link
Contributor Author

diehuxx commented Aug 22, 2024

Closing this since we hashed it out in #303

@diehuxx diehuxx closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KendallWeihe KendallWeihe KendallWeihe left review comments

@nitro-neal nitro-neal Awaiting requested review from nitro-neal nitro-neal is a code owner

@kirahsapong kirahsapong Awaiting requested review from kirahsapong

@shamilovtim shamilovtim Awaiting requested review from shamilovtim shamilovtim is a code owner

@decentralgabe decentralgabe Awaiting requested review from decentralgabe

@frankhinek frankhinek Awaiting requested review from frankhinek frankhinek is a code owner

@leordev leordev Awaiting requested review from leordev leordev is a code owner

@ALRubinger ALRubinger Awaiting requested review from ALRubinger

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@diehuxx @KendallWeihe