Remove dummy feature

[josephlr]

This is a breaking change for #98, so it's being merged against the 0.2 branch.

While this change alone would make it difficult for rand_core to use getrandom 0.2 without exposing a breaking change itself (due to wasm32-unknown-unknown breaking), the upcoming PR to address #4 should help in this regard.

[newpavlov]

We should be really careful to not leak v0.2 breaking changes to rand users, since we already got a fair bit of negative reactions. So I am interested to see how you intend to work around this problem.

Personally I think it should be fine to use the dummy impl for wasm32-unknown-unknown, it will simplify integration into std a tiny bit, plus guaranteed to not cause problems with downstream breakage. When and if getrandom will be exposed as part of std/core (probably as a separate crate), we can remove wasm32-unknown-unknown support completely.

[josephlr]

We should be really careful to not leak v0.2 breaking changes to rand users, since we already got a fair bit of negative reactions. So I am interested to see how you intend to work around this problem.

The basic idea here is that we only care about stdweb based randomness for supporting cargo web, every other wasm32-unknown-unknown environment I could find support bindgen (or is a more developed target like emscripten or wasi).

The basic idea would be to detect COMPILING_UNDER_CARGO_WEB in the build.rs file and use the stdweb implementation in that case, and the wasm-bindgen implementation in the other case. This gets rid of dealing with awkward features and (potentially) allows wasm32-unknown-unknown to work by default.

The alternative to the above would be making wasm-bindgen and stdweb "custom" rng providers, and use them with my #4 solution. This was my original idea, but it seems unnecessarily complex.

[newpavlov]

This gets rid of dealing with awkward features and (potentially) allows wasm32-unknown-unknown to work by default.

How would it work in the context of using getrandom in the std? We can't use wasm-bindgen there.

Also I am strongly against having for wasm32-unknown-unknown any default implementation except the dummy one. wasm-bindgen is tightly coupled with JS, while unknown target can not make any assumptions about an underlying environment. And it's not purely theoretical argument, people already quite actively use WASM outside of browsers.

@koute
What do you think? Would the proposed approach work for stdweb users?

[josephlr]

This gets rid of dealing with awkward features and (potentially) allows wasm32-unknown-unknown to work by default.

How would it work in the context of using getrandom in the std? We can't use wasm-bindgen there.

We would just not use the getrandom crate at all for wasm32-unknown-unknown. I think that's what the current patch does, something like

#[cfg(not(target_arch = "wasm32", target_os = "unknown"))]
getrandom::getrandom(&mut buf).unwrap();

and in the Cargo.toml

[target.'cfg(not(all(target_arch = "wasm32", target_os = "unknown")))'.dependencies]
getrandom = { version = "0.2", default-features = false, features = ["rustc-dep-of-std"] }

We could avoid the messy Cargo.toml by just having the crate be empty on unsupported targets, but that makes diagnosing issues harder, as you don't get a detailed compiler error.

Also I am strongly against having for wasm32-unknown-unknown any default implementation except the dummy one. wasm-bindgen is tightly coupled with JS, while unknown target can not make any assumptions about an underlying environment.

If we want to go this route, we would make stdweb and wasm-bindgen "custom" rng providers (like the various HWRNG chips for armv7-none-eabi, or RDRAND on x86_64-unknown-none). That could work, and rand/rand_core could avoid any external breakage by just enabling those "custom" RNGs based on features they get.

This actually might be more reasonable, as getrandom is a low-level crate, and it allows higher-level crates (like rand, ring, etc...) to just say "we don't care about non-bindgen" or say "we care, so here are a bunch of features".

And it's not purely theoretical argument, people already quite actively use WASM outside of browsers.

Are they using wasm32-unknown-unknown target in places without JS? I've seen wasm32-wasi but that's it's own target. There's also Node, but that has JS. It seems like no-JS wasm32 just end up with their own targets. There doesn't seem to be anyone upstream who wants a wasm32-unknown-js target, so it looks like the plain wasm32-unknown-unknown is (essentially) the main target for things like wasm-pack and wasm-bindgen.

[newpavlov]

Are they using wasm32-unknown-unknown target in places without JS?

See for example substrate, also I've heard folks are experimenting with WASM for plugin systems (although they could've migrated to WASI).

There doesn't seem to be anyone upstream who wants a wasm32-unknown-js target

I think there is some interest, but looks like it's not a priority right now. Either way, I don't think we should introduce conceptually wrong solutions, just because stuff is used in a roundabout way due to the lack of better alternatives.

[josephlr]

I think there is some interest, but looks like it's not a priority right now. Either way, I don't think we should introduce conceptually wrong solutions, just because stuff is used in a roundabout way due to the lack of better alternatives.

I think that's fair. Would the idea of having wasm-bindgen and stdweb as custom RNG provider crates work for you? Then for a crate (like rand 0.7) that has to care, we can do:

[dependancies]
getrandom = "0.2"
# The 'rand' crate can just rename optional dependencies in lieu of features
wasm-bindgen = { package = "getrandom-wasm-bindgen", version = "0.2", optional = true }
stdweb = { package = "getrandom-stdweb", version = "0.2", optional = true }

without breaking back-compat.

A crate that only cares about wasm-bindgen can just do:

[dependancies]
getrandom = "0.2"
getrandom-wasm-bindgen = "0.2"

[newpavlov]

I am not yet sure how your custom RNG provider idea works yet, so I can't really tell. But note that we should not break users who have lines like this in their Cargo.toml:

[dependencies]
rand = "0.7"
getrandom = { version = "0.1", features = ["wasm-bindgen"] }

I think we can work around cascading feature from v0.1 to v0.2 by using semver-trick, but I don't see how it would work with your idea. In the worst case scenario we may have to wait until rand v0.8 and rand_core v0.6 to do a v0.3 release.

[josephlr]

Huh, TIL about semver-trick. That’s very clever. I’m pretty sure I can make it work, but I’ll need to double check with a few tests. Supporting this scenario is important though, and we definitely need it if we want to avoid just updating everything at one.

…

On Thu, Sep 19, 2019 at 19:05 Artyom Pavlov ***@***.***> wrote: I am not yet sure how your custom RNG provider idea works yet, so I can't really tell. But note that we should not break users who have lines like this in their Cargo.toml: [dependencies]rand = "0.7"getrandom = { version = "0.1", features = ["wasm-bindgen"] } I *think* we can work around cascading feature from v0.1 to v0.2 by using semver-trick, but I don't see how it would work with your idea. In the worst case scenario we may have to wait until rand v0.8 and rand_core v0.6 to do v0.3 release. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#107?email_source=notifications&email_token=ABKAIDHQOUUJXT6M2A6ZBBLQKQVWNA5CNFSM4IYQ3GFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7FKEXI#issuecomment-533373533>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKAIDBHDYF76U7FXKJVIVTQKQVWNANCNFSM4IYQ3GFA> .

[dhardy]

The problem with semver-trick is that you need a new getrandom 0.1.x release dependent on getrandom 0.2.0, while users with an existing dependency on 0.1.x (e.g. via the TOML snippet above) may end up using an old 0.1.x and 0.2.0. (This has caused many complaints about rand 0.6.5 BTW due to a build error when this happens; with getrandom I guess you'd only get errors when version 0.2 is used on wasm32-unknown-unknown without required feature flags.)

---

To back up a bit, I feel like I missed the whole rationale for this PR. In #71 we added the dummy feature just over a month ago, as an opt-in switch to run-time errors. This is only really useful for people on wasm32-unknown-unknown who need to build but don't happen to use getrandom in their tests, but since then what has changed? A bump to 0.2 allows breaking changes but doesn't provide another solution to this; meanwhile two solutions have been mentioned in this thread (but are so far only possibilities).

Shouldn't we wait until we have implemented a new solution for wasm32-unknown-unknown before removing this?

---

Regarding @josephlr's proposals:

The basic idea would be to detect COMPILING_UNDER_CARGO_WEB in the build.rs file

Sounds interesting and possibly worth wider testing, if we can find a way to do that without risking lots of breakage (ideally with a pre-release version; unfortunately they don't tend to get enough attention; worse in this case that most users come through rand and will not see this without a dependency from rand).

Though given the rationales against making assumptions about wasm32-unknown-unknown, it might simply be best to avoid this.

If we want to go this route, we would make stdweb and wasm-bindgen "custom" rng providers

This could work, but would be opt-in, akin to the current feature flags; in this case I think there's still good reason to keep the dummy feature?

[josephlr]

@dhardy this is a fair point. Do you want me to push the custom RNG proposal in this PR? Or close this PR and open a new one?

[dhardy]

I think we can just leave this PR on the side while you open a new PR for that.

[dhardy]

As mentioned in #109 I'm happy to merge this, since if it proves necessary to enough people we can add again later (but hopefully we won't need to). On the other hand I see no reason we need to remove the dummy feature; it could also remain (with lower priority than custom).

[newpavlov]

@josephlr
So if I understand correctly you intend to replace the dummy feature with the custom impl crate, right? But I still don't understand how you intend to work around wasm32-unknown-unknown breakage in downstream crates, without a clear plan I don't think we should remove the dummy impl.

[dhardy]

Indeed, this may be a problem for rand_core (unless we make a breaking release there, which isn't desirable).

@josephlr sorry for the poor communication on my part; the reason I didn't merge this myself was to wait for comment from @newpavlov. Lets leave this for now and move on to #109, but we may have to revisit this (perhaps for 0.2.1).