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.
The following user-facing changes are included here: - `aws-lc` is used instead of `ring` for a cryptography backend - Expands our certificate signature algorithm support to include ECDSA_P256_SHA512, ECDSA_P384_SHA512, ECDSA_P521_SHA256, ECDSA_P521_SHA384, and ECDSA_P521_SHA512 - `--native-tls` is deprecated in favor of a new `--system-certs` flag, avoiding confusion with the TLS implementation used (we use `rustls` not `native-tls`, see prior confusion at #11595) - NASM is a new build requirement on Windows, it is required by `aws-lc` on x86-64 and i386 - `rustls-platform-verifier` is used instead of `rustls-native-certs` for system certificate verification - On macOS, certificate validation is now delegated to `Security.framework` (`SecTrust`). Performance when using `--system-certs` is improved by avoiding exporting and parsing all the certificates from the keychain at startup. - On Windows, certificate validation is now delegated to `CertGetCertificateChain` and `CertVerifyCertificateChainPolicy` - On Linux, certificate validation should be approximately unchanged - Some previously failing chains may succeed, and some previously accepted chains may fail; generally, this should result in behavior closer matching browsers and other native applications - macOS and Windows may now perform live OCSP fetches for early revocation, which could add latency to some requests - Empty `SSL_CERT_FILE` values are ignored (for consistency with `SSL_CERT_DIR`) The following internal changes are included here: - Certificate loading has been refactored to use a newtype with helper methods - The certificate tests have been rewritten - We use `webpki-root-certs` instead of `webpki-roots`, see #17543 (comment) - We request `identity` encoding for range requests, see astral-sh/async_http_range_reader#3 (comment) - Various dependencies (including forks) updates to versions which use reqwest 0.13+ This is a replacement of #17543 with an updated description. See that pull request for prior discussion. I've made the following changes from the initial approach there: - Previously, the `native-tls` TLS implementation was added which included an OpenSSL build. We don't currently use the `native-tls` implementation, but the `--native-tls` flag there was erroneously updated to enable it. - Previously, there was a `--tls-backend` flag to toggle between `native-tls` and `rustls`. Since we currently always use `rustls`, this is deferred to future work (if we need it at all). - Previously, there were unintentional breaking changes to `SSL_CERT_FILE` and `SSL_CERT_DIR` handling, including merging with the base certificates instead of replacing them, dropping support for OpenSSL hash-named certificate files, skipping deduplication of certificates. Here, we retain use of `rustls-native-certs` for loading certificates from the system as it handles these edge cases. Closes #17427 --------- Co-authored-by: salmonsd <22984014+salmonsd@users.noreply.github.com>
The following user-facing changes are included here: - `aws-lc` is used instead of `ring` for a cryptography backend - Expands our certificate signature algorithm support to include ECDSA_P256_SHA512, ECDSA_P384_SHA512, ECDSA_P521_SHA256, ECDSA_P521_SHA384, and ECDSA_P521_SHA512 - `--native-tls` is deprecated in favor of a new `--system-certs` flag, avoiding confusion with the TLS implementation used (we use `rustls` not `native-tls`, see prior confusion at #11595) - NASM is a new build requirement on Windows, it is required by `aws-lc` on x86-64 and i386 - `rustls-platform-verifier` is used instead of `rustls-native-certs` for system certificate verification - On macOS, certificate validation is now delegated to `Security.framework` (`SecTrust`). Performance when using `--system-certs` is improved by avoiding exporting and parsing all the certificates from the keychain at startup. - On Windows, certificate validation is now delegated to `CertGetCertificateChain` and `CertVerifyCertificateChainPolicy` - On Linux, certificate validation should be approximately unchanged - Some previously failing chains may succeed, and some previously accepted chains may fail; generally, this should result in behavior closer matching browsers and other native applications - macOS and Windows may now perform live OCSP fetches for early revocation, which could add latency to some requests - Empty `SSL_CERT_FILE` values are ignored (for consistency with `SSL_CERT_DIR`) The following internal changes are included here: - Certificate loading has been refactored to use a newtype with helper methods - The certificate tests have been rewritten - We use `webpki-root-certs` instead of `webpki-roots`, see #17543 (comment) - We request `identity` encoding for range requests, see astral-sh/async_http_range_reader#3 (comment) - Various dependencies (including forks) updates to versions which use reqwest 0.13+ This is a replacement of #17543 with an updated description. See that pull request for prior discussion. I've made the following changes from the initial approach there: - Previously, the `native-tls` TLS implementation was added which included an OpenSSL build. We don't currently use the `native-tls` implementation, but the `--native-tls` flag there was erroneously updated to enable it. - Previously, there was a `--tls-backend` flag to toggle between `native-tls` and `rustls`. Since we currently always use `rustls`, this is deferred to future work (if we need it at all). - Previously, there were unintentional breaking changes to `SSL_CERT_FILE` and `SSL_CERT_DIR` handling, including merging with the base certificates instead of replacing them, dropping support for OpenSSL hash-named certificate files, skipping deduplication of certificates. Here, we retain use of `rustls-native-certs` for loading certificates from the system as it handles these edge cases. Closes #17427 --------- Co-authored-by: salmonsd <22984014+salmonsd@users.noreply.github.com>
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.