-
Notifications
You must be signed in to change notification settings - Fork 211
Ergonomics improvements for alternative backends #671
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
Comments
From @newpavlov's comment: bevyengine/bevy#18047 (comment)
I'm inclined to agree, I don't really want to special case |
Good analysis of the problem and requirements. I'm very sympathetic: you're really not handed the right set of tools to solve this directly. Would it be viable to force users to declare a runtime RNG source using a static or the like? In Bevy, we had a similar set of requirements (only ever set by users, not libraries) for our global error handler and that was the flavor of solution we used. Obviously this is perf-critical though, so you'd need to be careful with locking data structures. |
This does not satisfy the fifth requirement since it allows for a random crate in the middle of your dependency tree to change the entropy source used by your whole application (assuming I understood your idea correctly). |
Would having a new |
Using something like a OnceLock, you could panic when it's set more than once. That would prevent this sort of attack, assuming you forced every user to explicitly declare an RNG source in their binary to use rand. |
It would greatly benefit specifically for the web case, but not so much beyond that. Being able to define a entropy source backend for something like |
I think there are two questions
For the second, that depends a lot on the people being impacted by the change |
Really what we need is Rust to have a Adding this target would help ecosystem-wide, as many low-level crates could now just implement the desired bindings to the necessary JS APIs without any flags/features/etc... when targeting As to @epage's point, I think the vast majority of people having problems here are having problems due to I would be fine with a stopgap solution which mostly only helped for |
Just to clarify, there's actually a lot of really well-supported
I would also support this, but I see some frustrating combinatorics here when you consider But, I do think we could potentially argue for a This is also forwards compatible with a potential future "web-only" target, since it would just always have A more general solution I would prefer to see for |
One potential solution which may improve ergonomics for Web WASM is to use an extern function defined in Overall, as I wrote elsewhere, we discussed the current design quite thoroughly before the v0.3 release and I am not keen on changing it drastically in getrandom v0.3. We may be open to an exception for Web WASM (e.g. like one described above), but it should not be a simple rehash of the Additionally, here my answers to some comments from #670:
While I agree that this situation is not ideal, I believe it's a toolchain problem and should be resolved by proposals like
We need a proper design which satisfies the requirements listed in the OP. For now, I haven't seen anything new which we haven't discussed previously. |
I agree, let's make sure we're working to the requirements listed above so we're productively moving towards a solution. Requirements
Met, the default behavior is identical to 0.3.3.
Met, the current mechanism of selecting a
Met, this will cause a linking error for the exact same reason the current
Met, a library can only provide a source of randomness if
Subjectively, I'd say met. While library crates can provide a backend, it can only be done in the case when
Met, this PR is entirely agnostic to the target.
Met in so far as it's the responsibility of the backend provider to do so. This PR makes no changes to dependencies in
Currently it is exactly the existing
Met. If If there are other requirements that haven't been laid out in the OP, or there's a disagreement about the PR's suitability, please let me know! This probably could be in the PR itself, but I think this discussion is important for the issue here. |
Relying on cryptic linking errors is not great. But I guess we can count it as "met".
I expect that in practice with your proposal we will have library crates providing fallback backend for Web WASM to make it more "convenient" for users and "to reduce number of papercuts". And you would get a mess of cryptic linking errors if two or more such crates will be in your dependency tree. It's less of a concern with the current opt-in custom backend, since adding the extern function is not enough and users still have to enabled the So I would say the requirement is not met. You could say it's the libraries fault for using the feature incorrectly, but it still would be a mess enabled by it. |
This is already happening with what we have at the moment 1, so I'm not sure why this is an argument against the proposal. If things are made easier/clearer from |
Linking the issue for |
I've been struggling with this issue while working on Basic requirements:
In order to satisfy the second requirement, we have to enable the
Including a
Alternatively, we could include both a This all amounts to a major introduction of complexity into a template that's supposed to be friendly for new users. Similarly, it's unreasonable to try to ease new users into understanding the nuances of this Considering the tradeoffs, our best way forward seems to be to |
@benfrankel is it viable to target |
@dhardy What's the status of WASI on browsers? Is this something being provided by browsers themselves or will it be an additional runtime to import/shim? Given the example provided by @benfrankel, that is for compiling/running a game on the browser, not for a WASM+WASI runtime like Wasmer. So likely targetting a WASI environment is not a viable solution. |
Given that all solutions for WASM-JS are hacky, it may even be worth promoting this approach; i.e. users add something like the following to the binary's [patch.crates-io]
getrandom = { version = "0.3", package = "getrandom-wasm-js" } This has some major caveats (must be added in the binary's ... or, I suppose we could publish a |
Regarding using
We effectively are specifying a different backend with the patch. We don't need to support another backend-specifier.
This is not the case for crates using
I'm not sure if this is the case? |
While I think the |
Because crates like @bushrat011899 the question should then be why we don't support JS on
So we can't please everyone. |
@dhardy I personally believe #672 actually does give everyone what they want by simply allowing better interop with 3rd party backends. This would allow the "should I think when posed with a difficult question that has no easy answer, the best path forward is the one that yields control to those with more information (the user). |
There's a movement currently (I believe coming from the rust project itself) away from |
This is a long issue, so I just opened #675 and #674; please discuss there (or open another issue if you have further proposals). I also closed #672, though I guess one could still argue in its favour. @NthTensor the real problem here is what to do on @josephlr despite the name, this issue really concerns only |
@dhardy This actually concerns more than just |
@Bluefinger most people have been talking about wasm32-u-u. Let's not assume that one solution is the best for all cases. The big problem I see here is that getrandom is hard to use on a common platform. |
@dhardy Fair, but any solution here also will have to apply to |
@Bluefinger @NthTensor |
Sorry, my post was in response to the mention of |
Uh oh!
There was an error while loading. Please reload this page.
As noted in many issues (#658, bevyengine/bevy#18047, https://users.rust-lang.org/t/getrandom-0-3-2-wasm-js-feature-issue/127584, n0-computer/iroh#3200), the use of a Rust
--cfg
flag can be unergonomic. See the current docs about this for context.This is mostly due to how tooling (
rustc
,cargo
, etc...) interact withRUSTFLAGS
and therustc
command line. This is exacerbated by the fact that thewasm32-unknown-unknown
target cannot have a default supported backend, as it is used in many environments (some of which do not have JavaScript). Leading to an unfortunate situation where, if someone wants to build any code which depends ongetrandom
targetingwasm32-unknown-unknown
, they must:Cargo.toml
to include:.cargo/config.toml
to include:This issue is to explore and evaluate approaches like #670 which seek to improve this story, either in
0.3
or in a future major release. It may turn out we end up keeping the current approach for a future major release, but we should explore all reasonable alternatives.Requirements
A bunch of the ergonomics issues stem from the requirements for this crate. Some of these requirements might be debatable or less important, but ideally any solution would have the following properties (note that the current approach only has most of these):
wasm32-unknown-unknown
should not be special-cased and should not call JavaScript by default (as the target makes it very clear that cannot be assumed).Cargo.lock
if that backend is not being used.#[global_allocator]
, as we are defining a global source of randomness.std
rust-lang/rust#130703, and ideally have a solution which works well even if that feature gets stabilized.The text was updated successfully, but these errors were encountered: