-
Notifications
You must be signed in to change notification settings - Fork 892
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
refactor(download): remove curl backend #3788
base: master
Are you sure you want to change the base?
Conversation
Have you taken stock of open issues referencing being solved by reverting to the curl backend? Have you considered the alternative of using curl with the rustls TLS backend? (Note: I am very much in favor of ditch curl and OpenSSL in favor of maximally-Rust solutions, but want to make sure we minimize the risk of getting users stuck.) |
@djc I've added a new section to track this.
Hmmm, with this option we are basically introducing First, I'm not sure if this would cause more problems or not, since we're introducing something new. In other words, can these existing issues be worked around with But indeed, this would allow the removal of PS: The 1.28.0 milestone is indicative, it only means I think this should be considered in this release cycle. |
So as I understand it:
There is no way of knowing whether the curl + rustls option would solve some of the issues that people have seen with using reqwest. Perhaps we should compile or release builds with tracing support built in so that people can give us more detailed logging without compiling their own rustup. At least for some of the issues, there's the suggestion that native-tls might actually be at fault, such that reqwest + rustls might actually do better than the default reqwest + native-tls option. I would advocate making the reqwest + rustls option the default first and shipping that -- I have high trust in the rustls maintainers (there are two full-time maintainers and myself) so I think any rustls issuers could be resolved pretty quickly. The one thing I would like is to ship support for rustls-platform-verifier in reqwest. |
@djc I guess that's a plus, yeah.
Do you think now is the right time to try out |
Yes, I think it would make sense to flip the default for the reqwest backend to rustls, but in that case I feel like we should keep the curl backend around for now as a backup option. And should definitely include a good tracing setup so that we can ask folks for more detailed information if issues occur. |
@djc In that case I guess we can still keep this PR as a potential second step. Just to be sure, should I remove |
Yes, I think this could be a good second step. To be clear, I don't think we should make the curl + openssl -> curl + rustls change at the same time as promoting the reqwest + rustls backend to be the primary, since curl + openssl is currently a reliable fallback for scenarios where the default doesn't work, so I think we should keep it for a while longer. So I think the steps are:
|
Part of #3790.
Revives #2034, superseding #1657.
(Thanks @lnicola!)
Concerns
Switching to the curl backend via
RUSTUP_USE_CURL
will no longer be a workaroundreqwest
backend #3122 (incomplete)reqwest/rustls
)