Skip to content
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

Expose features from rand to avoid compilation breakage on WASM #1804

Closed
wants to merge 1 commit into from

Conversation

najamelan
Copy link
Contributor

Current error:

error: target is not supported, for more information see: https://docs.rs/getrandom/#unsupported-targets
   --> /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.9/src/lib.rs:249:9
    |
249 | /         compile_error!("\
250 | |             target is not supported, for more information see: \
251 | |             https://docs.rs/getrandom/#unsupported-targets\
252 | |         ");
    | |___________^

In order to use join! and select! macros, we require the user to enable the
async-await and nightly features. However those features will pull in
rand. In order to work on WASM rand needs to have either the wasm-bindgen
or stdweb feature enabled. Otherwise compile time breakage happens due to the
getrandom crate.

Explanation can be found here: https://docs.rs/getrandom/0.1.9/getrandom/#unsupported-targets

This commit exposes those features through futures-util, allowing the end user to enable them
when compiling on WASM. This means futures does not need to choose which feature to enable
which would take away that decision from the end user.

An alternative would be to detect the wasm32 target in Cargo.toml and to choose
one of the features automatically.

Current approach requires action on part of the user to enable one of those features,
which means it should probably be documented somewhere. Currently the documentation
of join! and select! does not mention the need for the async-await and nightly
features.

I propose we add some documentation changes to this PR before merging. Following a similar
approach to getrandom by generating a compile time error to explain the situation to the user
might be the lowest friction for the end user. eg. when feature async-await is enabled on wasm32, verify
that one of the two required features is enabled as well, if not throw error. I can add that here
if it seems a desirable solution.

I have not tested the emscripten and WASI targets.

On some quick testing this alleviates the compile error on my system, but it would
be good if someone else had a look, or if we had CI testing for WASM.

Current error:
```
error: target is not supported, for more information see: https://docs.rs/getrandom/#unsupported-targets
   --> /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.9/src/lib.rs:249:9
    |
249 | /         compile_error!("\
250 | |             target is not supported, for more information see: \
251 | |             https://docs.rs/getrandom/#unsupported-targets\
252 | |         ");
    | |___________^
```

In order to use `join!` and `select!` macros, we require the user to enable the
`async-await` and `nightly` features. However those features will pull in
rand. In order to work on WASM rand needs to have either the `wasm-bindgen`
or `stdweb` feature enabled. Otherwise compile time breakage happens due to the
`getrandom` crate.

Explanation can be found here: https://docs.rs/getrandom/0.1.9/getrandom/#unsupported-targets

This commit exposes those features through `futures-util`, allowing the end user to enable them
when compiling on WASM. This means futures does not need to choose which feature to enable
which would take away that decision from the end user.

An alternative would be to detect the wasm32 target in Cargo.toml and to choose
one of the features automatically.

Current approach requires action on part of the user to enable one of those features,
which means it should probably be documented somewhere. Currently the documentation
of `join!` and `select!` does not mention the need for the `async-await` and `nightly`
features.

I propose we add some documentation changes to this PR before merging.

On some quick testing this alleviates the compile error on my system, but it would
be good if someone else had a look, or if we had CI testing for WASM.
@najamelan
Copy link
Contributor Author

Ps: another solution would be to see with getrandom why they prefer breaking rather than selecting wasm-bindgen as a default. That would simplify things.
@newpavlov ?

@newpavlov
Copy link

wasm32-unknown-unknown can not make any assumptions about a target on which it will be executed. It can be a browser, Node.JS, smart-contract, game module system, etc. So we simply don't have any "system" entropy source, thus we can not make wasm-bindgen feature enabled by default.

I am not sure if you need wasm-bindgen and stdweb features in this crate. The idea was that users will enable one of those feature only in the top level application crate.

@najamelan
Copy link
Contributor Author

@newpavlov The problem is that futures-util pulls in rand which pulls in getrandom. So the end user doesn't necessarily have getrandom in their Cargo.toml. They will depend on futures however.

I just verified and adding getrandom with the wasm-bindgen feature in my crate fixes the issue without modification to futures. If that is the desired approach, we need to document this!

@newpavlov
Copy link

If that is the desired approach, we need to document this!

Yes, I think it's the desired approach. If you have ideas of how we can improve getrandom docs, feel free to open a PR!

@najamelan
Copy link
Contributor Author

najamelan commented Aug 15, 2019

Ok, I filed rust-random/getrandom#89

I still think we should add section about "target platforms" on the readme of futures, and one explaining the "feature flags" too...

Also wondering, does this mean that if I have an application that depends on futures, but I don't set async-await feature, and at some point another dependency of my crate set's that feature, now I have to specify the getrandom dep with the correct feature. That means that turning on async-await as a library author is a breaking change! It doesn't feel quite right.

@Nemo157
Copy link
Member

Nemo157 commented Aug 15, 2019

I still think we should add section about "target platforms" on the readme of futures

I think having to re-document this in every crate that might transitively depend on getrandom is not worth it. A major reason to pull in semi-standard crates like this instead of rerolling randomness in every crate that needs it is to centralise platform support, it should be well documented by getrandom how to get it working on any odd platforms if it has partial support for them.

(I would not consider wasm32-unknown-unknown a platform that is always expected to work out of the box with std depending crates since it is lacking fundamental OS features like randomness. If there were no_std support for select then I would expect it to be possible to use that on wasm32-unknown-unknown, which reminds me I should open an issue about getting that working (EDIT: #1807).)

@najamelan
Copy link
Contributor Author

najamelan commented Aug 16, 2019

@Nemo157 Have you seen the discussion on the getrandom repo? I am proposing some improvements to the docs there. It turns out that this is a features application devs have to put, even if getrandom get's pulled in by a dependency.

I would propose that we do put a section about WASM in the readme where we explain things like this, but if the docs of getrandom are better, maybe the inconvenience is bearable, but I know I try to make docs for my crates so that users know everything they should in order to use them.

I shall close this PR to avoid confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants