-
Notifications
You must be signed in to change notification settings - Fork 119
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
Fix h1-client and default-client feature #282
Conversation
- name: Check all feature combinations | ||
# * `--feature-powerset` - run for the feature powerset of the package | ||
# * `--no-dev-deps` - build without dev-dependencies to avoid https://github.com/rust-lang/cargo/issues/4866 | ||
run: cargo hack check --feature-powerset --no-dev-deps |
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.
If this check takes a long time, it can limit the max number of simultaneous feature flags by passing the --depth
flag. (like futures-rs, hyper, etc. do) AFAIK --depth 2
should be sufficient in most cases as described in taiki-e/cargo-hack#58.
0c55a69
to
0811620
Compare
This also adds a CI task to check all feature combinations work properly.
Hi @taiki-e, Thanks for your insights in here and http-rs/http-client#67 + http-rs/http-client#69 ! The full purpose of this is to be able to configure Surf to use |
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 |
And looks like I have been bitten by this myself. I'll make sure to deal with this stuff properly tomorrow, finally. |
Looks like I ran us into the same issue that was reported with a fix at http-rs/surf#282 Our HTTPS/TLS for client requests hasn't been working because we started building without TLS support in a9a1abb. This is because the default features for http-client are `["h1_client", "native-tls"]`, but Surf disables http client's default features, and right now only re-enabled `"h1_client"`, missing the TLS support. Uh, oops.
src/client.rs
Outdated
#[cfg(feature = "default-client")] | ||
#[cfg(all( | ||
feature = "default-client", | ||
any( |
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.
These should not be necessary - we allow building the wasm client even if the target arch is not strictly wasm32
.
Also, even if it was, most of these clients explicitly set the default-client
feature, and so the casing would only need to be for the wasm client.
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.
we allow building the wasm client even if the target arch is not strictly
wasm32
.
http-client's wasm module compiles only on wasm targets, so building wasm-client on non-wasm targets will fail to compile.
Also, even if it was, most of these clients explicitly set the
default-client
feature, and so the casing would only need to be for the wasm client.
The problem is when the default-client
feature is specified and no client is specified.
In that case, DefaultClient
is not defined, so the compilation will fail if only cfg(feature = "default-client")
is used.
Lines 10 to 20 in fc75a59
cfg_if! { | |
if #[cfg(feature = "curl-client")] { | |
use http_client::isahc::IsahcClient as DefaultClient; | |
} else if #[cfg(feature = "wasm-client")] { | |
use http_client::wasm::WasmClient as DefaultClient; | |
} else if #[cfg(feature = "h1-client")] { | |
use http_client::h1::H1Client as DefaultClient; | |
} else if #[cfg(feature = "hyper-client")] { | |
use http_client::hyper::HyperClient as DefaultClient; | |
} | |
} |
However, cfg(feature = "default-client")
isn't really needed here, so it seems right to remove it.
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.
The problem is when the
default-client
feature is specified and no client is specified.
In that case,DefaultClient
is not defined, so the compilation will fail if onlycfg(feature = "default-client")
is used.
Another way to deal with this is to explicitly fail the compilation if only the default-client
feature is specified: https://github.com/rust-lang/futures-rs/blob/c7328778999f8d20f5c3bb821a8ad38b72923ff9/futures-core/src/lib.rs#L13-L14
It will make managing cfg somewhat easier.
# this feature flag is no longer necessary. | ||
default-client = [] |
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.
All clients enable this feature and all cfg(feature = "default-client")
removed, so this feature is no longer used.
Ok well I have merged the important part in #291 I'd rather not get rid of 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. |
Context: http-rs/http-client#67 (comment)
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.)