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.

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

Merged
merged 11 commits into from
Feb 6, 2024
Merged

chore: MSRV bump #2104

merged 11 commits into from
Feb 6, 2024

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Jan 30, 2024

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.

@toidiu
Copy link
Contributor Author

toidiu commented Jan 31, 2024

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.

Copy link
Contributor

@camshaft camshaft left a 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.

# 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" }
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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.)

Copy link
Contributor

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.

@toidiu toidiu marked this pull request as ready for review February 6, 2024 18:15
@camshaft camshaft merged commit 9226e90 into main Feb 6, 2024
119 of 121 checks passed
@camshaft camshaft deleted the ak-msrv branch February 6, 2024 18:26
@toidiu toidiu added the MSRV label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants