Skip to content

Update rand, glam and encase to latest versions #18047

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Bluefinger
Copy link
Contributor

@Bluefinger Bluefinger commented Feb 26, 2025

Objective

New rand version, which means updating glam and encase to support the newer ecosystem update. Does mean that this changes how WASM builds need to be done in order to configure getrandom correctly, but this can be remedied with updated docs.

Solution

Updating all needed dependencies to their compatible versions. This PR is currently blocked by encase, which is waiting on this PR to be merged and then a new version published. This PR is no longer blocked, hexasphere is blocking this PR now due to not yet having a new release with the latest glam version support, The PR is all good to go now, everything in order across glam/rand deps.

Testing

  • Must pass CI for all checks, tests, not introduce breaking changes.

Migration Guide

With newer versions of glam & encase, the updated versions don't seem to have introduced breakages, though as always, best to consult their docs 1 2 for any changes.

rand changes are more extensive, with changes such as thread_rng() -> rng(), from_entropy() -> from_os_rng(), and so forth. RngCore is now split into infallible RngCore and fallible TryRngCore, and the distributions module has been renamed to distr. Most of this affects only internals, and doesn't directly affect Bevy's APIs. For the full set of changes, see rand migration notes.

getrandom is also updated, and will require additional configuration when building Bevy for WASM/Web (if also using rand). The full details of how to do this is in the getrandom docs 1 2.

@mnmaita mnmaita added C-Dependencies A change to the crates that Bevy depends on S-Blocked This cannot move forward until something else changes D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 26, 2025
@alice-i-cecile alice-i-cecile added the A-Math Fundamental domain-agnostic mathematical operations label Feb 26, 2025
Copy link
Contributor

@mrchantey mrchantey left a comment

Choose a reason for hiding this comment

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

Looks good, lots of files changed but as mentioned in the pr its a 1:1 mapping to the updated function naming convention. I'm sure the wasm rustflags will be a gotcha for many a rust newbie, it'll be helful for us to explain this as much as possible in docs and guides.

@Bluefinger Bluefinger force-pushed the rand-v0.9-upgrade branch from 02c1ca9 to 6b8bb28 Compare May 9, 2025 08:34
@Bluefinger Bluefinger marked this pull request as ready for review May 9, 2025 08:35
@tysen
Copy link

tysen commented May 16, 2025

@Bluefinger - hexasphere release 16.0.0 updates glam to 0.30 (thanks @OptimisticPeach!).

@janhohenheim janhohenheim removed the S-Blocked This cannot move forward until something else changes label May 17, 2025
@janhohenheim janhohenheim removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 17, 2025
@janhohenheim janhohenheim added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 17, 2025
@mcobzarenco
Copy link
Contributor

Could this be added to 16.1 milestone?

@Bluefinger
Copy link
Contributor Author

Bluefinger commented May 18, 2025

Could this be added to 16.1 milestone?

I think this would cause breaking changes that might be painful to deal with for 0.16 projects, which would not really fit nicely with semver. getrandom and WASM in particular will require some massaging and at least a blog post entry to notify people that they'll have to do some things a bit differently to get it to work.

I understand wanting to have this now though... shame it took like three months to get all the deps ready for this upgrade.

Just to add to this: bevy_rand has a compat feature enabled by default in order to make it less painful for v0.16 projects to use latest rand/rand_core with bevy. Putting this PR in for 16.1 would mean that now I have a situation where different minor versions won't need the compat feature enabled. For me, it would be a semver break which will require a major version release to correct, and also require that new version to require 16.1, not 16.0, which might be confusing for users if they get particular build errors.

I'd rather keep semver breaking updates to API changes and new bevy versions if possible. That's just my two cents however. Other folks can chip in with whether it is worth doing this or not.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label May 19, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone May 19, 2025
@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 19, 2025
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

@alice-i-cecile
Copy link
Member

I think this would cause breaking changes that might be painful to deal with for 0.16 projects, which would not really fit nicely with semver. getrandom and WASM in particular will require some massaging and at least a blog post entry to notify people that they'll have to do some things a bit differently to get it to work.

Correct: 0.17 only unfortunately :(

@Bluefinger, we've changed how migration guides work as of the 0.17 cycle. Could you copy your excellent migration guide into a dedicated file quick? Once that's done I'll merge this!

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 19, 2025
@Bluefinger
Copy link
Contributor Author

@alice-i-cecile Done. Feel free to double check the content or adjust wording.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 19, 2025
@alice-i-cecile
Copy link
Member

getrandom is also updated, and will require additional configuration when building Bevy for WASM/Web (if also using rand). The full details of how to do this is in the getrandom docs

FYI, @mockersf has warned me that the changes needed here for Web for getrandom are quite painful. Reading the docs, I agree with that initial assessment. I want to be careful to evaluate the impact there before merging this, as reverting this will not be fun.

I don't intend to leave us on the old versions forever, but it's important to note that this is much higher stakes than normal dependency bumps.

@Bluefinger
Copy link
Contributor Author

getrandom is also updated, and will require additional configuration when building Bevy for WASM/Web (if also using rand). The full details of how to do this is in the getrandom docs

FYI, @mockersf has warned me that the changes needed here for Web for getrandom are quite painful. Reading the docs, I agree with that initial assessment. I want to be careful to evaluate the impact there before merging this, as reverting this will not be fun.

I don't intend to leave us on the old versions forever, but it's important to note that this is much higher stakes than normal dependency bumps.

If we don't go forward with the upgrade, please let's signal such to the rand/getrandom folks the reasoning for this. Though there's also some exploratory work being done by @bushrat011899 to see if this can be improved without all the ergonomic hits getrandom v0.3 has while still maintaining their desired security threshold. But I want to ensure there's motivation on both ends here to resolve what is a pretty... awful UX with the getrandom crate.

@newpavlov
Copy link

awful UX with the getrandom crate.

If you want a good out-of-box support, then you should advocate for a proper Web WASM target. For example, getrandom has absolutely no issues with the WASI targets. The "awfulness" is caused by the wide and long standing abuse of the wasm32-unknown-unknown target in the Rust ecosystem. Alternatively, you could push for adding entropy source to std/core (e.g. see rust-lang/rust#130703) with an ability to overwrite it similarly to #[global_allocator].

As a getrandom maintainer I would like to declare that we do not consider any potential changes to the wasm32-unknown-unknown handling in getrandom v0.3. We may change it in a future breaking release (which is unlikely to happen in several years), but we do not have better ideas right now how to handle this problem with the existing Rust/Cargo capabilities (e.g. global features could help).

@alice-i-cecile
Copy link
Member

If you want a good out-of-box support, then you should advocate for a proper Web WASM target.

Yep, this is high on our wishlist too :)

Alternatively, you could push for adding entropy source to std/core with an ability to overwrite it similarly to #[global_allocator].

Makes sense: no objections to this design either.

As a getrandom maintainer I would like to declare that we do not consider any potential changes to the wasm32-unknown-unknown handling in getrandom v0.3. We may change it in a future breaking release (which is unlikely to happen in several years)

This is useful to know. Moderately frustrating, as I would like to be unblocked on updating our dependencies well before that, but I understand that as a foundational crate you don't want to release breaking changes too frequently. If we discover a good solution, we may temporarily fork to get unblocked sooner, although that will require coordination with our other dependencies.

The other option is for us to build out and push tooling to help manage this pain for users on the web, e.g. bevy_cli. That's not quite ready yet, but it is a solution that's entirely within our control, which is desirable.

@josephlr
Copy link

josephlr commented May 19, 2025

Hey everyone, I'm the other maintainer of getrandom, and I'd like to know if there's anything we can improve here (subject to our constraints).

For context as @newpavlov mentioned, wasm32-unknown-unknown is one of the most annoying targets for us to deal with. Unlike other fully-supported Web targets (wasm32-unknown-emscripten,wasm32-wasip1,wasm32-wasip2), this target assumes nothing about the external environment, so we cannot even assume that JavaScript exists when building for this target. This means we need to have some control (be it Cargo flags, --cfg flags, some macro invocation, etc...) for a user to say "hey I'm compiling for the web".

Further complicating things, overriding the source of randomness is a very security-sensitive thing to do, so we don't want to allow any crate in the dependency graph to override it, making it challenging to only use Cargo features (which can be set by any crate on any other crate). So we use RUSTFLAGS to override it via --cfg getrandom_backend=some_custom_backend. Ideally, we would have something similar to #[global_allocator], instead of RUSTFLAGS (which are not ideal), but we need more support from Rust itself for that to work (we tried doing something like @bushrat011899's exploration, but we couldn't get it working without feature(random) or feature(extern_item_impls)).

The other problem is that the wasm-bindgen crates have a very large number of transitive dependencies, so we need to have an off-by-default Cargo feature to avoid issues like rust-random/getrandom#433 (comment). This leads to the situation where you have both --cfg getrandom_backend="wasm_js" and features = ["wasm_js"].

Without releasing a breaking change (which would be quite disruptive), we could:

  • Improve the documentation, error messages, etc...
  • Make it so features = ["wasm_js"] activates getrandom_backend="wasm_js" by default (potentially with a warning saying --cfg getrandom_backend="wasm_js" is missing).

As a getrandom maintainer I would like to declare that we do not consider any potential changes to the wasm32-unknown-unknown handling in getrandom v0.3.

I would take a slightly more relaxed stance, that we cannot have any breaking changes to our handling of the target, but if there's something we could do that's not a breaking change, I would consider it.

@newpavlov
Copy link

Make it so features = ["wasm_js"] activates getrandom_backend="wasm_js" by default

Personally, I am against it. The unfortunate reality is that people unconditionally enable such features in library crates for "convenience" despite strongly worded warnings against it. This breaks builds for users who target non-Web WASM. Even worse, just having wasm-bindgen in project's dependency tree can be problematic in such cases (we had one such user, unfortunately I can not find his comment right now).

Overall, we discussed this problem long enough before settling on the current solution with the full understanding of drawbacks. I think that the ecosystem should just bite the bullet here. Hopefully, it will add a bit of motivation for improving the unsatisfactory Web WASM situation. In my opinion, introducing new exceptions and workarounds at this stage would do more harm than good.

@bushrat011899
Copy link
Contributor

we tried doing something like @bushrat011899's exploration, but we couldn't get it working without feature(random) or feature(extern_item_impls)

@josephlr If I completed that exploration branch, would it be a design the getrandom project would be interested in adopting? I don't believe I need either of those experimental features, as I'm basing my design off of the already stable (and widely used) critical-section crate.

Obviously no one person speaks for the project, so "interest" is good enough for me to be motivated to complete the prototype. I have no desire to fragment the Rust ecosystem for an ergonomics change (that's the worst ergonomics!).

@Bluefinger
Copy link
Contributor Author

@alice-i-cecile @mockersf Just as a heads up, I don't think we can avoid the effect of RUSTFLAGS/getrandom v0.3 since it is already within our dependencies: uuid via bevy_reflect. So if we are worried about the ergonomics of this on our endusers, we either have to revert to an older uuid version in the mean time, or go ahead with the upgrade here (so to reduce the amount of duplicate deps a bit).

@josephlr
Copy link

I've opened rust-random/getrandom#671 so we can move most of the non-bevy related conversion there (and avoid spamming your issue tracker).

@josephlr If I completed that exploration branch, would it be a design the getrandom project would be interested in adopting? I don't believe I need either of those experimental features, as I'm basing my design off of the already stable (and widely used) critical-section crate.

Obviously no one person speaks for the project, so "interest" is good enough for me to be motivated to complete the prototype. I have no desire to fragment the Rust ecosystem for an ergonomics change (that's the worst ergonomics!).

Thanks so much for opening rust-random/getrandom#670 !! I'll try to take a look when I have time to really dive into it. I'm definitely interested if it could work (either in 0.3 or in a future release).

@alice-i-cecile @mockersf Just as a heads up, I don't think we can avoid the effect of RUSTFLAGS/getrandom v0.3 since it is already within our dependencies: uuid via bevy_reflect. So if we are worried about the ergonomics of this on our endusers, we either have to revert to an older uuid version in the mean time, or go ahead with the upgrade here (so to reduce the amount of duplicate deps a bit).

This makes sense, and I would expect that most of your users targeting wasm32-unknown-unknown will run into this sort of problem.

@Bluefinger
Copy link
Contributor Author

@josephlr just to clarify, the issue with the getrandom v0.3 dep is actually in our v0.16 version, because uuid made a minor version update to update to getrandom v0.3 and rand v0.9, which is a semver breakage. So we will have to pin uuid version for v0.16.1 as a fix for the mean time, and then decide whether to go ahead with this PR or not for v0.17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Dependencies A change to the crates that Bevy depends on D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants