Skip to content

Narrow down getrandom troubleshooting fix to be target-dependent - #2542

Open
selvmaya wants to merge 3 commits into
bevyengine:mainfrom
selvmaya:getrandom-wasm-doc
Open

Narrow down getrandom troubleshooting fix to be target-dependent#2542
selvmaya wants to merge 3 commits into
bevyengine:mainfrom
selvmaya:getrandom-wasm-doc

Conversation

@selvmaya

@selvmaya selvmaya commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

When I wrote #2465, I hadn't looked at how you could make it fit inside a Cargo.toml used for multiple targets. The code block now explicitly mentions [target.'cfg(target_arch = "wasm32"'.dependencies], instead of just implying some sort of [dependencies] entry in a comment.

According to getrandom docs, it is not ideal to enable this feature unless you A) are only making (wasm1) binaries, B) unconditionally depend on a relevant WASM crates wasm-bindgen or js-sys.

I like this change because it more specifically solves the issue documented, and should only be applied where it is needed (aka does not change the other builds when they don't have an issue), and lets you discover the target-dependent dependencies option, which is otherwise hard to discover despite presumably being used in practice. I personally also use this to add the webgpu feature to web, or wayland only to linux targets.

I cannot speak for implementations across wasm/non-wasm backends of getrandom having perfect parity in output (in terms of cross-platform determinism), but I assume they would document state that more clearly if it wasn't the case.

Edit: I also changed some of the surrounding wording to be clearer and more useful.

Footnotes

  1. Worth noting that they do not explicitly say here that it is a bad idea to use for multi-platform binary crates, but it is implied that you want to avoid it unless you need to use it for bloat reasons, and it is stated just above it that it could potentially break builds for non-web platforms.

@Bluefinger

Copy link
Copy Markdown
Contributor

You can also look at bevy_rand for additional docs/examples of how to resolve this. Also, there are multiple wasm targets you need to account for, so the "fix" should look more like so:

[target.'cfg(all(target_family = "wasm", any(target_os = "unknown", target_os = "none")))'.dependencies]

@selvmaya

selvmaya commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

That change does improve the generalization of the final code, but the intent here is just to show that you can specify dependencies per target, and for most people (maybe handwavy) they just care about each major platform and not the difference between wasm targets, which is why the original PR got merged with no issue.

I can change it, maybe linking to documentation about what each wasm target means here. It just wasn't necessary to solve the problem I state in the description.

@Bluefinger

Copy link
Copy Markdown
Contributor

There is still the WASI targets as well (which don't need special-casing via getrandom features) and I think the original code would not exclude those targets.

So the two cases we want to include are unknown and none WASM targets, but not WASI, hence the suggested code. wasm64 is also becoming a thing now, so going forward, we can't assume just wasm32 as a target arch.

But as you said, we just wanna teach the initial start to fixing. But fixing WASM issues can be a bit trickier than at first glance, so we should at least arm or point folks to where they can solve or learn how to solve them.

@selvmaya

selvmaya commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@Bluefinger I've improved the wording surrounding the code blocks and used your suggestion.

@Bluefinger Bluefinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now, giving multiple pointers without being overwhelming or overloading with info. Great job!

@Based-A Based-A added S-Ready-For-Final-Review Ready for a maintainer to consider for merging C-Content A-Quick-Start About the Quick Start Guide labels Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Quick-Start About the Quick Start Guide C-Content S-Ready-For-Final-Review Ready for a maintainer to consider for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants