-
Notifications
You must be signed in to change notification settings - Fork 124
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
chore: MSRV bump #2104
chore: MSRV bump #2104
Conversation
We are currently pulling in two different version of aws-lc-syc from s2n-quic-crypto and s2n-tls-sys. Its possible to fix this by restricting aws-lc-rs version in s2n-quic-crypto or updating the aws-lc-rs in s2n-tls-sys. Ideally we bump version in s2n-tls-sys but need to discuss with team if its possible to do that soon and then do a release of s2n-tls. |
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 also need to update the rust-version
in all of the Cargo.toml files. This will notify users on older toolchains that it's unsupported.
quic/s2n-quic-crypto/Cargo.toml
Outdated
# this dependency once s2n-tls-sys starts consuming a higher version of aws-lc-sys. | ||
# | ||
# https://github.com/aws/s2n-quic/pull/2104#issuecomment-1918559366 | ||
aws-lc-rs = { version = "=1.5" } |
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.
This isn't great. It will require us to do the same thing every time aws-lc-sys gets a new minor release. I think either aws-lc-sys also needs to bump to the same major version as aws-lc-rs or aws-lc-rs needs to export the C headers/lib dir so s2n-tls-sys can use 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.
While you're here, we should also do default-features = false
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.
If you do default-features = false
, you will need to add features = ["aws-lc-sys"]
.
(Consumers will still be able to select the "fips" feature with this. If you use the "non-fips" feature, then "fips" is blocked.)
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.
Although, thinking about that a bit more, it does mean that if you enable fips that both sys and fips-sys will be built, since we've forced that dependency here. Not sure if there's a way around that, though.
Resolved issues:
#1735
#1750
#1751
#1752
#1804
#1991
#1992
#1993
Description of changes:
This PR bumps the MSRV to 1.71.1. It also bumps and unpins many crates that were previously pinned due to a lower MSRV.
Testing:
CI passes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.