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.

Sign up for GitHub

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

Jump to bottom

update rand package to 0.8.3 #20

Merged
merged 1 commit into from Apr 21, 2021
Merged

update rand package to 0.8.3 #20

merged 1 commit into from Apr 21, 2021

Conversation

girazoki
Copy link

This PR updates the rand package to 0.8.3, which does not require the i128 support and grabs the StdRng from rand::rngs.

@JoshOrndorff
Copy link

This will avoid nasty version confusion similar to rust-random/rand#645 when using this in Frontier precompiles https://github.com/paritytech/frontier/tree/master/frame/evm/precompile/bn128

niklasad1
niklasad1 reviewed Apr 20, 2021
View reviewed changes
Cargo.toml
rustc-serialize = { version = "0.3", optional = true }
byteorder = { version = "1.0", features = ["i128"], default-features = false }
crunchy = "0.2.1"
lazy_static = { version = "1.4.0", features = ["spin_no_std"] }
rustc-hex = { version = "2", default-features = false }

[dev-dependencies]
rand = { version = "0.5", features = ["i128_support"] }
rand = { version = "0.8.3", features = ["std_rng"] }
Copy link
Member

@niklasad1 niklasad1 Apr 20, 2021 •
edited

Choose a reason for hiding this comment

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

this dev-feature will be leaked into features but it was like that before so probably fine.

niklasad1
niklasad1 approved these changes Apr 20, 2021
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM

@niklasad1 niklasad1 assigned ordian and unassigned ordian Apr 20, 2021
@niklasad1 niklasad1 requested review from dvdplm and ordian April 20, 2021 14:01
@girazoki girazoki changed the title update rand pacakge to 0.8.3 update rand package to 0.8.3 Apr 20, 2021
@ordian
Copy link
Member

ordian commented Apr 20, 2021 •
edited

There is an alternative PR #18, I don't have a strong opinion on this, @NikVolf could you take a look?

Also the latest published version of substrate-bn is 0.5.0, which doesn't match what's in master, so I don't know if there any other changes that are also not master? I mention this, because this change is breaking.

Other than that, no objection from me to merge this.

@dvdplm
Copy link

dvdplm commented Apr 20, 2021

There is an alternative PR #18, I don't have a strong opinion on this, @NikVolf could you take a look?

#18 seems to do a bit more than this one and while not completely orthogonal, it seems to me like we might want to have both this one and that one.

So this lgtm.

@ordian
Copy link
Member

ordian commented Apr 20, 2021

#18 seems to do a bit more than this one and while not completely orthogonal, it seems to me like we might want to have both this one and that one.

I say it's an alternative, because it removes the rand dependency and adds a dependency on more stable rand_core (which is at 0.6 version compared to 0.8 rand). So with #18 we don't actually need this PR.

@girazoki
Copy link
Author

girazoki commented Apr 21, 2021 •
edited

For us its fine either this PR or #18, as both of them remove the dependency of rand 0.5.6 which is the source of our issues with the bn128 precompiles. Let us know whether you finally want this to be merged or #18. From what I see there are some minor cosmetic changes between the version that was published for substrate (which I assume is the nv-publish-for-substrate branch) and master, but if you prefer I can PR that branch instead.

@ordian ordian merged commit b048fe1 into paritytech:master Apr 21, 2021
@dvdplm
Copy link

dvdplm commented Apr 21, 2021

Good pt about rand_core. I now have a slight preference for #18, let's merge that.

ordian added a commit that referenced this pull request Apr 21, 2021
@ordian
Merge branch 'master' into nv-publish-for-substrate
5d020c6 
* master:
  update rand pacakge to 0.8.3 (#20)
  fix edition and prepare publish (#19)
@ordian
Copy link
Member

ordian commented Apr 21, 2021

@dvdplm feel free to take over that PR and resolve conflicts, I'm inclined to publish after #22 is merged and leave for later though

@girazoki can publish as soon as @NikVolf adds paritytech as crate owners

@ordian
Copy link
Member

ordian commented Apr 21, 2021 •
edited

I'll publish it soon if there are no objections.

UPD: published 0.6.0.

@girazoki
Copy link
Author

Good for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

@ordian ordian Awaiting requested review from ordian

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@girazoki @JoshOrndorff @ordian @dvdplm @niklasad1