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

Move OsRng to rand_core #863

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
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.

rand_os crate has no purpose now
4 participants