http-rs / surf

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

fix: pin getrandom to work on wasm #316

Closed jkelleyrtp closed 3 years ago

jkelleyrtp commented 3 years ago

This PR pins getrandom to the "js" feature so that surf compiles on wasm32 again.

Solves #315

Before this change, when I tried to build the repo for wasm:

cargo build --target wasm32-unknown-unknown --features wasm-client --no-default-features

   Compiling getrandom v0.2.3
error: the wasm32-unknown-unknown target is not supported by default, you may need to enable the "js" feature. For more information see: https://docs.rs/getrandom/#webassembly-support
   --> /Users/jonkelley/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.3/src/lib.rs:219:9
    |
219 | /         compile_error!("the wasm32-unknown-unknown target is not supported by \
220 | |                         default, you may need to enable the \"js\" feature. \
221 | |                         For more information see: \
222 | |                         https://docs.rs/getrandom/#webassembly-support");
    | |_________________________________________________________________________^

error[E0433]: failed to resolve: use of undeclared crate or module `imp`
   --> /Users/jonkelley/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.3/src/lib.rs:246:5
    |
246 |     imp::getrandom_inner(dest)
    |     ^^^ use of undeclared crate or module `imp`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0433`.
error: could not compile `getrandom`

To learn more, run the command again with --verbose.

With these feature flags, I can build properly:

cargo build --target wasm32-unknown-unknown --features wasm-client --no-default-features

    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
CryZe commented 3 years ago

Keep in mind that the approach in this PR requires "target specific features", which means that this change is somewhat incompatible with resolver = 1. This may be fine, but it's something worth considering. It probably makes more sense to tie it to the wasm-client feature.

jkelleyrtp commented 3 years ago

Keep in mind that the approach in this PR requires "target specific features", which means that this change is somewhat incompatible with resolver = 1. This may be fine, but it's something worth considering. It probably makes more sense to tie it to the wasm-client feature.

Thanks for the pointers. I switched the flag selection to use normal feature flags instead of the target-dependent flags.

Fishrock123 commented 3 years ago

I'll try to get 2.3.0 out "soon".

CryZe commented 3 years ago

web-sys, getrandom and encoding now always get pulled in as dependencies. Well I guess at least it's semi optional via the feature, but the crates should still not be pulled in for targets that don't need them (or can't even compile them).

Fishrock123 commented 3 years ago

Those should only be pulled in when wasm-client is selected though?

CryZe commented 3 years ago

Yes, but due to resolver = 1, there are technically situations where that feature is active, but you aren't even compiling for WASM. (Resolver 1 is truly a mess)

Fishrock123 commented 3 years ago

Witch situations would those be? You have to manually activate the feature or have something else consciously activate it.

CryZe commented 3 years ago

I believe it can be if you have multiple final binaries in the same workspace, where one ends up activating the feature for WASM and one compiles for x86. Another alternative is that there might be a crate that depends on surf, but always expects to be used only on WASM, so that crate might end up in your dependency graph (since all targets are always in the dependency graph. you can often find fuchsia and redox specific crates in Cargo.lock) and then even if you never intend to compile for WASM, this feature will be active. That's the issue with resolver 1. Technically you can ignore resolver 1 and say that surf is "only" compatible with resolver 2 (which basically has separate dependency graphs when it comes to the features for each target), but this is trivially fixable by undoing a few lines of this PR.

I can send a PR tomorrow that fixes it.