Skip to content

Update rand to 0.9.x #788

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

Merged
merged 1 commit into from
Apr 16, 2025
Merged

Update rand to 0.9.x #788

merged 1 commit into from
Apr 16, 2025

Conversation

dmitrmax
Copy link

@dmitrmax dmitrmax commented Apr 6, 2025

Closes #789

Kixunil
Kixunil previously approved these changes Apr 7, 2025
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK a9fe576

@Kixunil Kixunil dismissed their stale review April 7, 2025 09:08

Forgot to check CI

@Kixunil
Copy link
Collaborator

Kixunil commented Apr 7, 2025

You need to run update lock files from contrib directory.

@dmitrmax
Copy link
Author

dmitrmax commented Apr 7, 2025

Done

@apoelstra
Copy link
Member

You also need to run the formatter and fix the compilation errors with the rand feature enabled.

@Kixunil
Copy link
Collaborator

Kixunil commented Apr 8, 2025

Also please squash the commits.

@dmitrmax
Copy link
Author

dmitrmax commented Apr 8, 2025

Ok, i did:

  1. Fixed compilation errors
  2. Squashed the commits
  3. Run the rustfmt. But it formatted a lot of things that I've never touched myself. Is it ok?

@dmitrmax
Copy link
Author

dmitrmax commented Apr 9, 2025

I reverted rustfmt for the files which are not affected by the transition to rand v0.9.0. Only those touched by me are reformatted now.

@apoelstra
Copy link
Member

Seems like you are using the wrong version of the formatter. You need to use a nightly compiler to run cargo fmt.

@dmitrmax
Copy link
Author

dmitrmax commented Apr 9, 2025

Done

@dmitrmax
Copy link
Author

Fixed the no-std test

@apoelstra
Copy link
Member

The wasm failure looks like it's related to a call to memmove in libsecp. That isn't related to this PR. Similarly the Windows CI failure is hard to read but I think it's also unrelated. I'll run my local CI on this and see if it passes.

@dmitrmax
Copy link
Author

Ok. Let me know if there is anything more I can do for this PR.

@apoelstra
Copy link
Member

Ok, right now we depend on getrandom 0.2 with the js feature enabled. This is done when compiling for wasm to turn this feature on because rand depends on it.

But rand 0.9 does not depend on getrandom 0.2. It depends on getrandom 0.3. It appears that getrandom 0.3 no longer has the js feature, even though the README for rand 0.9 says it does https://crates.io/crates/rand

I'm not sure what exactly to do here, but we should definitely get rid of the getrandom 0.2 dependency.

@dmitrmax
Copy link
Author

Ok, I've updated getrandom to 0.3. Somehow that required to update js-sys crate up to recent version 0.3.77. Sorry, I'm not a wasm guy, I've called contrib/wasm.sh and it ran with the following error when executing wasm-bindgen:
failed to find intrinsics to enable clone_ref function

Not sure what it means.

Also I've updated rand_core dependency from 0.6 to 0.9.

I've made a separate commit to simplify the review. If it looks good to you I can squash both commits into one.

@apoelstra
Copy link
Member

Can you squash? I think it's easiest to review this in one commit.

@dmitrmax
Copy link
Author

Squashed

@apoelstra
Copy link
Member

Thanks for iterating so much! This looks great now. Nice to move to syn 2 across the board. Hopefully this works with our MSRV. CI will tell us.

I will also run my local CI on this which will do a bunch more checks, and then we should be good to merge.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 993b4b7; successfully ran local tests

@apoelstra apoelstra merged commit f5d0769 into rust-bitcoin:master Apr 16, 2025
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update rand dependency
3 participants