-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
hyper v1 upgrade #2059
hyper v1 upgrade #2059
Conversation
0fd22ac
to
0dca943
Compare
@seanmonstar What still needs to be done, can we help? With https://github.com/rustls/hyper-rustls/releases/tag/v%2F0.26.0 it should permit to progress |
cc21f90
to
d187492
Compare
9c3ce5a
to
74dc685
Compare
@seanmonstar I added a bunch of commits to this fork: https://github.com/grafbase/reqwest/commits/hyper-v1-and-wasm-from-parts-build-split/. All but the newest one ("WIP") might be useful. Some might need adjustment, but the fork works for us. 🙂 The test suite isn't passing yet, but a lot of tests were green after the adjustment in the "WIP" commit. |
Thanks! If you wanna open a PR against this branch with the working commits, I'll merge it in |
@seanmonstar sure, of course! Here it is: #2115. They're a somewhat minimal set of changes that I had to make in order to get reqwest working for us with hyper v1, but they may be neither sufficient, nor necessarily entirely correct. They're also dependent on hyperium/hyper-util#95, which I opened separately. Kindly let me know if I can be of more help. 🙂 |
4354bde
to
68a683d
Compare
@seanmonstar Thanks for working on this! I'd like to draw your attention to the fact that the current draft commit seems to have omitted @jakubadamw's e-mail from the |
rustls_pemfile::Item::X509Certificate(cert) => { | ||
certs.push(rustls::Certificate(cert)) | ||
rustls_pemfile::Item::X509Certificate(cert) => certs.push(cert.into()), | ||
rustls_pemfile::Item::PKCS8Key(key) => { |
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.
upgrading rustls_pemfile
would simplify this: https://github.com/seanmonstar/reqwest/pull/2139/files#diff-f229bc4b874c8edcd2c19a11fe9871e47d51519cf51ed829058f0bdbb1e4b27bR323
|
||
fn supported_verify_schemes(&self) -> Vec<SignatureScheme> { | ||
vec![ | ||
SignatureScheme::RSA_PKCS1_SHA1, |
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.
nit: sort alphabetically?
}); | ||
|
||
root_cert_store.add_trust_anchors(trust_anchors); | ||
root_cert_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); |
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.
nit: probably better extend_with_slice
: https://github.com/seanmonstar/reqwest/pull/2139/files#diff-0df5342f97493f9f55d6f43a14268f327ea9e791c7fdb8df6d88ec81f6baa721R469
Avoiding clone and thus additional moves I guess
|
||
let mut buf = [0; 8192]; | ||
let mut pos = 0; | ||
|
||
loop { | ||
let n = conn.read(&mut buf[pos..]).await?; | ||
let n = tokio_conn.read(&mut buf[pos..]).await?; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@seanmonstar thanks for your work on this. Out of curiosity, is there any blocker remaining to this getting merged? 🙂 |
The last thing before merging I need to figure out: releasing what's merged since the last release (and do I bother with deprecating/renaming builder options that have inconsistent names), and potential support plan for the "old" version. But some other higher urgency things have my attention this week. |
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: Sean McArthur <[email protected]> Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
Alright, v0.11.25 is out, maybe there were some other things that could have been squeezed in, but oh well. Let's get this merged! 🚀 |
To clarify for others: It looks like v0.11.25 was released prior to the merging of the pull-request that updates hyper to v1. Reason this came to my attention: I tried updating to the "hyper v1 compatible" version of reqwest by adding this to my
I waited for rust-analyzer to finish processing, then inspected the [[package]]
name = "reqwest"
version = "0.11.25"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0eea5a9eb898d3783f17c6407670e3592fd174cb81a10e51d4c37f49450b9946"
dependencies = [
[...]
"hyper 0.14.28", <-- the unexpected line
"hyper-rustls 0.24.2",
"hyper-tls",
[...]
] So I guess a version of If so, I'll just continue using GitHub as the crate source, but figured I'd mention this here for others who might have thought that v0.11.25 was supposed to support hyper v1. For others: This is how you can include the "hyper v1 compatible" version of reqwest in your project: (add this to your reqwest = {git = "https://github.com/seanmonstar/reqwest.git", rev = "2c11ef000b151c2eebeed2c18a7b81042220c6b0", <set your features here> } |
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
Follow up on this pull request seanmonstar#2059
* Add file function to async_impl::multipart * Add test for asynchronous file function in multipart * Fix doc of file function in blocking::multipart * Fix test Follow up on this pull request #2059 * Fix doc test
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
This is a big and breaking change. A lot of the internals were worked on. But the breakage shouldn't be that visible to public API. The main differences are: - Publicly exposes `http` v1, instead of v0.2. - Integration with `hyper::Body` has changed. Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <[email protected]> Co-authored-by: Jakub Wieczorek <[email protected]>
* Add file function to async_impl::multipart * Add test for asynchronous file function in multipart * Fix doc of file function in blocking::multipart * Fix test Follow up on this pull request seanmonstar#2059 * Fix doc test
The upgrade process to hyper v1, which is tracked in #2039. This branch isn't compiling yet, which is why it's marked as a draft. But I'm doing it in pieces, and if someone wants to grab some while I'm distracted with other things, it's here.
Closes #2039