Skip to content

Move OsRng to rand_core #863

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Aug 22, 2019
Merged

Move OsRng to rand_core #863

merged 3 commits into from
Aug 22, 2019

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Aug 9, 2019

Closes #854 which discuses this idea.

We don't need to rush this decision, so this PR will stay open a while for discussion.

@dhardy
Copy link
Member Author

dhardy commented Aug 9, 2019

Note: this implies one breaking change when next publishing rand: OsRng::new is now removed. However this was deprecated since 0.7.0 so I think this is acceptable.

@vks
Copy link
Collaborator

vks commented Aug 9, 2019

What about the rand changelog?

@dhardy
Copy link
Member Author

dhardy commented Aug 9, 2019

There's not any change (unless removal of OsRng::new counts).

@newpavlov
Copy link
Member

I think it also worth to release rand_os v0.3.0 with a description stating that users should use rand_core::OsRng instead.

@dhardy
Copy link
Member Author

dhardy commented Aug 10, 2019

You mean we should count this as a breaking release for rand_os?

@newpavlov
Copy link
Member

No, I mean, if we deprecate rand_os, I think we should notify users that it should not be used anymore. Releasing an empty rand_os v0.3 is a good way of doing so in my opinion.

@dhardy
Copy link
Member Author

dhardy commented Aug 10, 2019

You don't think the deprecation warning and comment I added to the README already are enough?

@newpavlov
Copy link
Member

Ah, haven't noticed that.

@burdges
Copy link
Contributor

burdges commented Aug 11, 2019

I do kinda think you're better off with a free fn instead of the SeedableRng::from_entropy, but not my reason is that rand_core 0.5 does not have from_entropy, so this avoids one change that people like me may mistake for breaking changes. ;)

#[cfg(feature="getrandom")]
fn make_rng<R: SeedableRng, S: Default+RngCore = OsRng>() -> R {
    R::from_rng(S).unwrap()
}

@newpavlov
Copy link
Member

I think it's ready to be merged? Or do you want to merge together with #864?

@dhardy
Copy link
Member Author

dhardy commented Aug 17, 2019

Yes, I think it's ready. #864 will need to be rebased; unfortunately I will have very limited time available over the next few weeks.

@dhardy dhardy merged commit 18ce640 into rust-random:master Aug 22, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rand_os crate has no purpose now
4 participants