Skip to content
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

Update dependencies #783

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

getong
Copy link

@getong getong commented Mar 2, 2025

Update dependencies:

  • rand to 0.9
  • bitcoin_hashes to 0.16
  • getrandom to 0.3

@apoelstra
Copy link
Member

I guess we can move to rand 0.9 now that our MSRV is 1.63.0.

Can you update the lockfiles included in this crate? You can run ./contrib/update-lock-files.sh to do so.

@getong
Copy link
Author

getong commented Mar 2, 2025

I guess we can move to rand 0.9 now that our MSRV is 1.63.0.

Can you update the lockfiles included in this crate? You can run ./contrib/update-lock-files.sh to do so.

done

@apoelstra
Copy link
Member

Thanks! Can you also update the getrandom dep to 0.3? As written this results in two versions of getrandom (and two versions of some of its wasm-related deps).

Finally, can I ask that you break this into three commits:

  • Update hashes
  • Update getrandom
  • Update rand

So that we can see the lockfile changes for each one?

@getong
Copy link
Author

getong commented Mar 3, 2025

Thanks! Can you also update the getrandom dep to 0.3? As written this results in two versions of getrandom (and two versions of some of its wasm-related deps).

Finally, can I ask that you break this into three commits:

  • Update hashes
  • Update getrandom
  • Update rand

So that we can see the lockfile changes for each one?

done

@getong getong changed the title update rand to 0.9, bitcoin_hashes to 0.16 update rand to 0.9, bitcoin_hashes to 0.16, getrandom to 0.3 Mar 3, 2025
@getong getong force-pushed the update-rand-0.9 branch from bef5cb3 to 2791338 Compare March 3, 2025 02:24
@getong getong mentioned this pull request Mar 3, 2025
8 tasks
@tcharding tcharding changed the title update rand to 0.9, bitcoin_hashes to 0.16, getrandom to 0.3 Update dependencies Mar 3, 2025
@tcharding
Copy link
Member

I edited your PR description and title, hope you don't mind.

@tcharding
Copy link
Member

I can investigate your CI fail later on today if you like. I'm surprised to see it with these small changes.

@tcharding
Copy link
Member

Oh there are re-names that need doing in the docs e.g., thread_rng was renamed to rng.

@getong getong force-pushed the update-rand-0.9 branch from 1084f93 to 78e7479 Compare March 4, 2025 15:02
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.

I believe the last commit needs to be merged with the one updating rand.

@@ -37,7 +37,7 @@ global-context-less-secure = ["global-context"]
secp256k1-sys = { version = "0.11.0", default-features = false, path = "./secp256k1-sys" }

hashes = { package = "bitcoin_hashes", version = "0.16", default-features = false, optional = true }
rand = { version = "0.9", default-features = false, optional = true }
rand = { version = "0.9", default-features = false, features = ["thread_rng"], optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't object to his but long term it'd be cool to propagate the feature.

@getong
Copy link
Author

getong commented Mar 4, 2025

getong#1

In this github ci , only two failed, but it does not matter with this repo code.

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 4, 2025

@getong we have additional policy that each commit separately must pass tests. This is (currently) not checked by Github but @apoelstra has his own CI where he tests it.

@apoelstra
Copy link
Member

The WASM error I think isn't related to this, but the other CI failure is because you're enabling the std feature of getrandom. I don't think we need this so it's ok to unconditionally disable it.

@getong getong force-pushed the update-rand-0.9 branch from 20a2351 to 78e7479 Compare March 6, 2025 02:32
@getong
Copy link
Author

getong commented Mar 6, 2025

The WASM error I think isn't related to this, but the other CI failure is because you're enabling the std feature of getrandom. I don't think we need this so it's ok to unconditionally disable it.

I try to add std feature to getrandom, but ci test still failed. So I delete std feature of getrandom.

@apoelstra
Copy link
Member

Unfortunately you need to explicitly disable it with default-features = false.

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.

4 participants