Default WASM bindings?

@tomaka

@tomaka recently brought up the fact that this crate still fails to compile on WASM by default since explicit activation of either stdweb or wasm-bindgen is required (see #676).

Note that these are now "features" forwarded to the rand_os crate thanks to @newpavlov's #643.

Should we enable one of these two features by default? Are wasm-bindgen and stdweb compatible now? Is it likely that both projects will continue to exist in the future?

I notice that we could now use web-sys bindings, though probably this achieves nothing over using wasm-bindgen directly and appears to lack the Node API bindings.

CC @koute @fitzgen @alexcrichton

If one of the features will be enabled, then it will be easy to forget to disable default features. I think WASM feature should be enabled only by application level crates, and all library crates should stay unopinionated and enable one of those features only for dev profile. It's not convenient, but until ecosystem around WASM will settle down (e.g. until we'll get wasm32-unknown-bindgen) it's the best we can do without being opinionated.

So, I have three points:

1- rand 0.6.2 and 0.6.3 broke builds that were working with rand 0.6.1. If a long discussion is needed before fixing this, IMO the right thing to do would be to yank these versions first.

2- Cargo features are not the right tool for this job. They are not compilation flags, and I have had to dig several times in the past through dependencies hell in order to fix this kind of bad usage of Cargo features. However, let's not care about this because almost nobody in the Rust ecosystem seems to care about this anyway, and because the only viable alternative is environment variables and build scripts, which people may not like.

3- If you take a look at this, the way it works is:

If the stdweb and wasm-bindgen features are enabled, wasm-bindgen is used.
If only the wasm-bindgen feature is enabled, wasm-bindgen is used.
If only the stdweb feature is enabled, stdweb is used.
If neither are enabled, the build fails.

Seeing this, why not remove the stdweb feature entirely? There is no use case for "the build fails". It would then simply become this:

If the wasm-bindgen feature is enabled, wasm-bindgen is used.
If it is not enabled, stdweb is used.

Which, for me, makes more sense.
Right now I don't see any reason to not have stdweb enabled all the time.

@alexcrichton

Cargo features are not the right tool for this job.

Yeah, but unfortunately we don't have better tools right now. Ideally "system RNG" should be selected with weak lang item (see e.g. discussion in #648), but for the time being we should work with that we have.

If the stdweb and wasm-bindgen features are enabled, wasm-bindgen is used.

I probably should've maid it a compilation error as well. I think good compilation errors should reduce time to spent debugging such issues to a minimum.

I hope @alexcrichton and/or @ashleygwilliams will help us here.

I probably should've maid it a compilation error as well. I think good compilation errors should reduce time to spent debugging such issues to a minimum.

That means that if you have a crate A that depends on rand+stdweb, and a crate B that depends on rand+wasm-bindgen, then there's no way can have a crate C that depends on both A and B.

Yes, this is why earlier I've wrote that library crates shouldn't enable those features outside of dev profiles. If we do not mind getting opinionated, then we could choose one of them as a default and use another one if feature is enabled (i.e. do as you proposed). One drawback here will be that with the enabled feature both stdweb and wasm-bindgen will be compiled, as IIRC cfg(not(feature="foo")) in Cargo.toml is not handled correctly right now.

I do have plans to make stdweb compatible with wasm-bindgen, however we're not there yet.

That said, I do think that wasm32-unknown-unknown target should stay neutral in this regard. By definition if the OS is unknown then we shouldn't assume anything about its capabilities (people also use this target for other things than the Web!), so by default any capabilities which depend on the OS-provided RNG should be disabled on wasm32-unknown-unknown unless the user explicitly opts into them.

What about adding a runtime error if both features are disabled?

My pragmatic concern is that right now if you write a library that has rand 0.6.2+ as a dependency, you cannot test on your CI whether it compile for wasm, unless either you choose between stdweb and wasm-bindgen (which is bad), or you add stdweb and wasm-bindgen features in your library as well (which is less bad but very annoying).

My pragmatic concern is that right now if you write a library that has rand 0.6.2+ as a dependency, you cannot test on your CI whether it compile for wasm, unless either you choose between stdweb and wasm-bindgen (which is bad), or you add stdweb and wasm-bindgen features in your library as well (which is less bad but very annoying).

I think rand should still compile without those features on wasm32-unknown-unknown, just without the OS-specific parts being available (OsRng, JitterRng::new, etc.). And by "without being available" I don't mean that the APIs will not be defined, I mean that they'll just return Result::Err when the user tries to initialize them.

I believe runtime error in this case will go against Rust philosophy of handling as many errors at runtime as possible and practical. As a user I will be probably very surprised when I'll find out that I have to enable one of the features and there is no compilation-time error.

you cannot test on your CI whether it compile for wasm

As I've wrote earlier in your Cargo.toml you can write the following:

[dev-dependencies]
rand_core = { version = "0.3", features = ["stdweb"] }

This way stdweb feature will be enabled for WASM CI tests and will be ignored for other targets.

I think rand should still compile without those features on wasm32-unknown-unknown

It already can be compiled right now, you just have to disable std feature (alloc feature can stay enabled), which I find logical if you compiling for wasm32-unknown-unknown without wasm-bindgen or stdweb backing.

I believe runtime error in this case will go against Rust philosophy of handling as many errors at runtime as possible and practical. As a user I will be probably very surprised when I'll find out that I whould enable one of the features and there is no compiler error.

These APIs already return a Result, so it should be natural for the user to assume that they can fail and handle such a case appropriately.

Not everyone tests their crates on wasm32-unknown-unknown; if you just not export those APIs on wasm32-unknown-unknown then every crate which uses rand and uses (for example) OsRng somewhere (but otherwise properly handles the case when there is no OsRng available) will not compile, even though it would otherwise work perfectly (just with limited functionality).

every crate which uses rand and uses OsRng somewhere will not compile, even though it would otherwise work perfectly

I think you misunderstood something. If we are talking about application crates, then you will have to just add rand_core = { version = "0.3", features = ["stdweb"] } to your [dependencies], and it should get compiled without issues (assuming that none of your dependencies have enabled wasm_bindgen feature, which should be counted as a bug). Library crates don't have to do anything (well except adding the line to [dev-dependencies] if they want WASM CI tests) and they can be developed without keeping WASM in mind.

@koute

rand 0.6.2 and 0.6.3 broke builds that were working with rand 0.6.1

Not sure why you're saying this because neither of these features were ever enabled by default. What 0.6.2 did break was moving them into rand_os and not forwarding them; 0.6.3 should be the same as 0.6.1 here.

I agree with @koute that we shouldn't make assumptions for wasm32-unknown-unknown.

@tomaka you can use a dev-dependency as suggested or add sub-crates into your project for testing which enable one of the features; I'm experimenting with the latter for Rand.

Not sure why you're saying this because neither of these features were ever enabled by default. What 0.6.2 did break was moving them into rand_os and not forwarding them; 0.6.3 should be the same as 0.6.1 here.

0.6.1 didn't seem to have a compile_error! in place. The OsRng was simply not available.

I‘m using neither stdweb nor wasm-bindgen and rand 0.6.3 (and I guess 0.6.2) seem to have broken my crate, so I‘d like to see the compatibility for the case of neither brought back as well.

It already can be compiled right now, you just have to disable std feature

There is no target dependent features in cargo, so this doesn‘t work.

True. I see; you want WASM with no extras to compile but with no OsRng? Which particular rand functionality do you use?

There is no target dependent features in cargo, so this doesn‘t work.

Why would you want std functionality only on some targets? Either your code needs that or it doesn't.

It looks like criterion (dev-dependency) and parking_lot use it. So rand is not a direct dependency, but an indirect one. criterion isn't used for the wasm case, but for testing, and it very likely needs the std feature. parking_lot very likely needs the std feature for all the other targets too. So I don't think forcing them to not use std is an option.

Presumably however you can run your tests without them using thread_rng or EntropyRng because those would need to pull from OsRng (the alternative, JitterRng, requires high-precision timers not available on WASM). In other words, run-time failure would solve your use-case.

The alternative is compile-time disabling OsRng but keeping run-time detection within EntropyRng (what we did do); unfortunately I don't think we can do this now we have a separate crate without disabling it by default (which we won't do). However, there would still be run-time detection somewhere so it also doesn't gain much.

I guess then we need to replace those compile_error! statements in rand_os with run-time failure.

I had a quick look at parking_lot and it does require an entropy source, so needs rand_os. So the only reason your tests worked before would be because they were not exercising this part of parking_lot. I don't believe however it makes sense to modularise everything / make it all configurable, so the only option is run-time failure on use.

Navigation Menu

Repository navigation

Default WASM bindings? #678

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Default WASM bindings? #678

Description

Activity

newpavlov commented on Jan 5, 2019

tomaka commented on Jan 5, 2019

newpavlov commented on Jan 5, 2019

tomaka commented on Jan 5, 2019

newpavlov commented on Jan 5, 2019

koute commented on Jan 5, 2019

tomaka commented on Jan 5, 2019

koute commented on Jan 5, 2019

newpavlov commented on Jan 5, 2019

koute commented on Jan 5, 2019

newpavlov commented on Jan 6, 2019

dhardy commented on Jan 6, 2019

tomaka commented on Jan 6, 2019

CryZe commented on Jan 6, 2019

dhardy commented on Jan 6, 2019

CryZe commented on Jan 6, 2019

dhardy commented on Jan 6, 2019

dhardy commented on Jan 6, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions