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

feat(download/rustls): use aws-lc instead of ring #3898

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Jun 22, 2024

Closes #3820 by replacing the ring backend with aws-lc.

Fortunately, with seanmonstar/reqwest#2301, it's now possible to totally disable ring at compile time.

Tested locally on macOS 14.5 via the following command with positive outcome:

> RUSTUP_LOG=TRACE RUSTUP_FORCE_ARG0=rustup rustup run stable cargo run -- update

@rami3l rami3l requested a review from djc June 22, 2024 03:15
@rami3l rami3l marked this pull request as draft June 22, 2024 03:42
@rami3l rami3l force-pushed the deps/aws-lc branch 2 times, most recently from beca5fb to a0bb52a Compare June 22, 2024 08:32
@rami3l

This comment was marked as resolved.

@rami3l rami3l force-pushed the deps/aws-lc branch 8 times, most recently from 0442a35 to 063a1bd Compare June 22, 2024 10:19
@rami3l rami3l removed the request for review from djc June 22, 2024 10:20
@rami3l rami3l force-pushed the deps/aws-lc branch 4 times, most recently from be21da8 to b419112 Compare June 24, 2024 12:36
@rami3l rami3l changed the title feat(download/rustls): use aws-lc and rustls-platform-verifier feat(download/rustls): use aws-lc instead of ring Jun 26, 2024
@rami3l rami3l force-pushed the deps/aws-lc branch 2 times, most recently from 9b03ed0 to 6e5c0c7 Compare June 29, 2024 01:28
@rami3l rami3l force-pushed the deps/aws-lc branch 2 times, most recently from 020ddf4 to d0aaa44 Compare June 30, 2024 13:11
@justsmth
Copy link

Failures related to symbol conflicts between aws-lc-rs and openssl, such as this one, should (hopefully) resolve with our next aws-lc-sys release.

We're also working to resolve Android-related issues, but I'm not sure whether our latest changes would resolve the failure. Based on the error message, I think it might just require setting the ANDROID_NDK environment variable?

@rami3l
Copy link
Member Author

rami3l commented Jul 31, 2024

Failures related to symbol conflicts between aws-lc-rs and openssl, such as this one, should (hopefully) resolve with our next aws-lc-sys release.

@justsmth Thanks a whole lot for the investigation! I just got a bit sidetracked due to personal reasons in the past few weeks, sorry about that... 🙇 Looks like the clash with OpenSSL is very likely the root cause, I'm looking towards the new version :)

We're also working to resolve Android-related issues, but I'm not sure whether our latest changes would resolve the failure. Based on the error message, I think it might just require setting the ANDROID_NDK environment variable?

You're right. Setting $ENV{ANDROID_NDK} did address the issue for me! Many thanks!

@justsmth
Copy link

We've now released aws-lc-sys v0.20.1. 🎉 (*fingers crossed* -- hoping all the builds succeed next time. ☺️ )

Please let us know if you have any other issues with building aws-lc-rs.

@rami3l
Copy link
Member Author

rami3l commented Jul 31, 2024

We've now released aws-lc-sys v0.20.1. 🎉 (fingers crossed -- hoping all the builds succeed next time. ☺️ )

Please let us know if you have any other issues with building aws-lc-rs.

@justsmth Ohhhhhh that's so cool! Thank you so much for chasing this all the way down!

One thing to notice though is that we currently don't test for all cross-compilation targets in PRs to save some quota, so there is a small possibility that I might need your help again when we're cutting a new release, as that's usually when all tests are actually run. That said, I'm definitely looking forward to shipping this with Rustup v1.28!

@rami3l rami3l requested a review from djc July 31, 2024 14:02
@rami3l rami3l marked this pull request as ready for review July 31, 2024 14:02
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Great work!

(Nit: IMO the commits here are not logically separate -- in particular, including the first commit would make CI fail until the last commit is added -- so I would just squash all of this into a single commit.)

@rami3l rami3l force-pushed the deps/aws-lc branch 2 times, most recently from a4c1628 to fd8526d Compare August 1, 2024 01:58
@rami3l
Copy link
Member Author

rami3l commented Aug 1, 2024

Great work!

(Nit: IMO the commits here are not logically separate -- in particular, including the first commit would make CI fail until the last commit is added -- so I would just squash all of this into a single commit.)

@djc Commits squashed as requested. Merging...

@rami3l rami3l enabled auto-merge August 1, 2024 02:00
@rami3l rami3l disabled auto-merge August 1, 2024 02:00
@rami3l rami3l enabled auto-merge August 1, 2024 02:03
@rami3l rami3l added this pull request to the merge queue Aug 1, 2024
Merged via the queue into rust-lang:master with commit 8fdc613 Aug 1, 2024
27 checks passed
@rami3l rami3l deleted the deps/aws-lc branch August 1, 2024 02:41
@rami3l rami3l mentioned this pull request Oct 6, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of support for p521 signatures with the ring-based reqwest/rustls backend
3 participants