Conversation
src/lib.rs
Outdated
| .header( | ||
| reqwest::header::ACCEPT_ENCODING, | ||
| reqwest::header::HeaderValue::from_static("identity"), | ||
| ) |
There was a problem hiding this comment.
I'm not sure I love this, but what was happening is that reqwest was attempting to "double" decompress compressed files, leading to "Invalid gzip header" in ~9 tests. This was caused by reqwest moving to tower-http in 0.12.25 and 0.12.26 (here).
Those tests were:
uv::it lock::lock_find_links_http_sdist
uv::it lock::lock_find_links_http_wheel
uv::it pip_install::already_installed_dependent_editable
uv::it pip_install::already_installed_local_path_dependent
uv::it pip_install::install_git_private_https_interactive
uv::it pip_install_scenarios::no_wheels_with_matching_platform
uv::it pip_install_scenarios::no_wheels
uv::it pip_install_scenarios::no_binary
There was a problem hiding this comment.
Hmm, that's unfortunate. I'm not sure if we want to put a blanket Accept: identity in here or not; it's not incorrect but it might pessimize future uses of this crate (e.g. where we're doing range requests over resources that aren't already compressed).
Can we handle this on the uv side instead? I'm guessing what we need to do is set this header on the callsites where we do range requests/construct the client that does them.
CC @konstin for opinions as well.
(Separately, do you understand why the tower-http change induced this? Is it that reqwest is now more aggressive about response decompression? Not a blocker but I'd like to better understand the change itself 🙂)
There was a problem hiding this comment.
Hmm, that's unfortunate. I'm not sure if we want to put a blanket
Accept: identityin here or not; it's not incorrect but it might pessimize future uses of this crate (e.g. where we're doing range requests over resources that aren't already compressed).
yep, agreed, the fix feels very hacky
Can we handle this on the uv side instead? I'm guessing what we need to do is set this header on the callsites where we do range requests/construct the client that does them.
CC @konstin for opinions as well.
I can look into that, but since we need reqwest 0.13+ and the stream feature, i'm not sure there's an easy way out.
(Separately, do you understand why the tower-http change induced this? Is it that reqwest is now more aggressive about response decompression? Not a blocker but I'd like to better understand the change itself 🙂)
From what I can ascertain, the move to tower-http moves the decompression to the middleware layer as part of this commit. So decompression happens eagerly.
There was a problem hiding this comment.
Now that I think about this, we should be able to handle in this uv... will look more into this.
There was a problem hiding this comment.
From what I can ascertain, the move to
tower-httpmoves the decompression to the middleware layer as part of this commit. So decompression happens eagerly.
Make sense, thanks for explaining!
There was a problem hiding this comment.
@woodruffw reverted my changes, and implemented the fix in registry_client.rs here: astral-sh/uv@311d8ac
Local tests passed.
…-compression" This reverts commit 3ae4516.
This PR is 3 out of 4 PRs to fix astral-sh/uv#17427.
Will need astral-sh/reqwest-middleware#11 merged first, in order to update the Cargo.lock appropriately from the crates registry.