search

http-rs / surf

Fast and friendly HTTP client framework for async Rust
https://docs.rs/surf
Apache License 2.0
1.45k stars 119 forks source link

Fix h1-client and default-client feature #282

Closed taiki-e closed 3 years ago

taiki-e commented 3 years ago

Context: https://github.com/http-rs/http-client/pull/67#issuecomment-778734155

This also adds a CI task to check all feature combinations work properly. (This is almost the same as that used in crossbeam, futures-rs, hyper, etc.)

JEnoch commented 3 years ago

Hi @taiki-e,

Thanks for your insights in here and http-rs/http-client#67 + http-rs/http-client#69 ! I realise you're far more experienced than I am with Cargo features...

The full purpose of this is to be able to configure Surf to use h1_client + rustls. See my attempt in #271.
I'm not sure to see how we would achieve that with your PR, since the h1-client defaults to http-client/native-tls. What's the best solution in your opinion ?

Fishrock123 commented 3 years ago

I'm not sure to see how we would achieve that with your PR, since the h1-client defaults to http-client/native-tls. What's the best solution in your opinion ?

I thought we were already disabling http-client's default features in Surf?

JEnoch commented 3 years ago

I'm not sure to see how we would achieve that with your PR, since the h1-client defaults to http-client/native-tls. What's the best solution in your opinion ?

I thought we were already disabling http-client's default features in Surf?

Yes. I meant that in the Cargo.toml of this PR the h1-client feature uses http-client/h1_client + http-client/native-tls. We would still lack a feature in Surf to use http-client/h1_client + http-client/rustls.

Fishrock123 commented 3 years ago

And looks like I have been bitten by this myself. I'll make sure to deal with this stuff properly tomorrow, finally.

Fishrock123 commented 3 years ago

Ok well I have merged the important part in https://github.com/http-rs/surf/pull/291

I'd rather not get rid of default-client if possible because it makes all of the cfg statements much easier to read, although it should probably be renamed to __default-client.

I did not take the wasm32 change because it unnecessarily complicates things.

Both of those features are just ignored in the cargo hack check because the first doesn't matter 9impliced by other features) and the second we check properly in a separate wasm context.