Allow overriding the reqwest TLS backend - #327
Conversation
EdJoPaTo
left a comment
There was a problem hiding this comment.
Hey there! Thank you for your PR and bringing up this issue!
I am not entirely happy with this, but this is mainly due to the fact that I would prefer frankenstein to be only a thin wrapper around external clients like reqwest and not much more. But I think that's hard to reach due to the trait requirements of Rust… So nothing to blame you 😇
| all-features = true | ||
|
|
||
| [features] | ||
| default = ["rustls-tls"] |
There was a problem hiding this comment.
Just as an idea, if it doesn't work out well I'm also happy about it.
What if it would be default = ["reqwest?/rustls"] and no rustls related features? When someone wants the default rustls it still works fine. If someone wants something else, no-default-features and rustls as a dependency in your crate and define it yourself with all the options rustls provides.
There was a problem hiding this comment.
Let me see, but I don't think it works
There was a problem hiding this comment.
I'm building a test crate to make sure it actually works
There was a problem hiding this comment.
I've updated the code with your design
There was a problem hiding this comment.
I think its more clean that way. But I think now we need to adapt the feature toggle for the default feature, rather than on the reqwest feature for wasm32:
target.'cfg(not(target_arch = "wasm32"))'.dependencies.reqwest
|
@EdJoPaTo if you see how other popular request-dependent crates do, almost all of them have to expose this complexity :-( Different environments have different requirements and TLS backend is something people want to tweak pretty often. |
|
@EdJoPaTo do you think it's good to merge? |
rustls with the aws-lc-rs provider stays the default (via the default feature => reqwest?/rustls). To use a different backend, disable default features and add your own reqwest dependency selecting the TLS features you want; previously the rustls/aws-lc-rs provider was hardcoded and impossible to opt out of because Cargo features are additive.
baf844b to
c2682f7
Compare
|
Sorry for taking so long to answer currently 😔 |
Problem
The
client-reqwestfeature pulls inreqwestwith a hardcodedrustlsfeature:reqwest'srustlsfeature selects theaws-lc-rscrypto provider, which builds theaws-lc-syscrate via a slow C/CMake build script. Because Cargo features are additive, a downstream cannot opt out ofaws-lc-rsonceclient-reqwestis enabled — even if they configurereqwestelsewhere withrustls-no-providerornative-tls, therustls(aws-lc-rs) feature still wins.Change
Move the TLS backend selection into frankenstein's own features and stop hardcoding it on the
reqwestdependency:rustls-tls— rustls with theaws-lc-rsprovider. Enabled bydefault, so existing users keep the previous behaviour.rustls-tls-no-provider— rustls without a built-in crypto provider (install your own, e.g.rustls::crypto::ring::default_provider().install_default()); avoidsaws-lc-sys.native-tls— the platform's native TLS.Each maps to the corresponding
reqwestfeature with the weak-dependency syntax (reqwest?/...), so they are no-ops unless the reqwest client is enabled.Backwards compatibility
Users who keep default features (e.g.
features = ["client-reqwest"]) are unaffected —default = ["rustls-tls"]reproduces the oldrustls/aws-lc-rssetup.Users who already pass
default-features = falseand want a lighter provider can now do:Verification
cargo check --features client-reqwest(default) — still resolvesaws-lc-rs.cargo check --no-default-features --features client-reqwest,rustls-tls-no-provider— noaws-lc-rs/aws-lc-sysin the tree.cargo check --no-default-features --features client-ureqandcargo check --all-featuresboth build.