Put stdweb dependency behind a target feature

[pitdicker]

See #272 (comment), cc @tomaka, @quininer

[dhardy]

Shouldn't we just call it stdweb, or rather just use cfg(feature="stdweb") since that is already an optional dependency?

[pitdicker]

That would be my choice, but cargo does not support features that have the same name as dependencies. We have the same problem with serde, which has serde-1 as feature flag.

[pitdicker]

I didn't know using the name of an optional dependency was possible, much better!

[dhardy]

Don't forget the feature name here

[tomaka]

This is also theoretically the wrong fix unfortunately. What the stdlib does when the feature flag is enabled is assume that an FFI function exists, and calls it.

In other words, something like:

#[cfg(feature = "foo")]extern"C"{fnrand() -> usize;}

[pitdicker]

I don't completely follow what you mean, @tomaka. Can you say a bit more about what is exactly wrong?

[dhardy]

He means that js code shouldn't be there. I'm not really sure why that's beneficial though. @quininer can you also comment?

[quininer]

I don't understand.

[tomaka]

The general problem is assuming that wasm32-unknown-unknown is related to the web. Surely it was invented for the web, but in general it has a lot of traction for non-web usages.

With this PR, if you use a library A that depends on rand, then this library A will have propagate the stdweb feature to the user. This is really cumbersome.

[pitdicker]

But then are we not ok now? With this PR wasm32-unknown-unknown will again have stubs, and only have an implementation using stdweb when you set the feature flag.

[dhardy]

So do we want optional stdweb support and a feature calling unimplemented extern(C) function prototypes? I guess that's what you're asking for.

Then I'll answer that: no. We are planning on letting users replace OsRng anyway (for embedded platforms with no OS), but will probably do so in a different method. So I think this PR is all we need here (the custom WASM non-web entropy source can wait).

[tomaka]

Here is the whole background of the discussion if you are motivated for some lecture: rust-lang/rust#47102
Let's not reopen the same debate here!

[dhardy]

With this PR, if you use a library A that depends on rand, then this library A will have propagate the stdweb feature to the user. This is really cumbersome.

If and only if library A depends on the stdweb feature of Rand. This is exactly the same as the case with std and other features. I don't see a problem here.

The status (after merging this PR) is thus that any attempt to use Rand when targetting wasm32-unknown-unknown will panic (if the stdweb feature is not used), which sounds similar to std without wasm-syscall (although I did not read the whole PR linked). As I said we plan to add a more general way to fix this in the future, but not as part of this PR.

[tomaka]

If and only if library A depends on the stdweb feature of Rand. This is exactly the same as the case with std and other features. I don't see a problem here.

The problem is that it's not A's job to decide whether to enable the stdweb feature of rand. It's only the final binary that should do so.

[pitdicker]

The problem is that it's not A's job to decide whether to enable the stdweb feature of rand. It's only the final binary that should do so.

That seems like a good, although theoretical concern. If somewhere in the dependency chain A decides stdweb is available, I see no reason not to use it for OsRng.

But when things are worked out a bit more around WebAssembly, and we have a solution for #313, I think we can revisit this and move more in the direction you have in mind. But for now this simply solves a practical problem.

[dhardy]

There are two solutions to that:

1. A does not depend on the stdweb feature at all; the final binary may have to depend on Rand just to add the stdweb feature.
2. Lib A adds an optional stdweb feature itself, forwarding the dependency.

Oh, I see @pitdicker got here before I did 👍

[dhardy]

BTW rebase and merge?