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

Make rand_core optional #447

Merged

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Nov 30, 2022

As proposed in #442 moves rand_core as optional feature

This still gets used via dev-dependencies and depending on feature selection tests uses API impl by proxy or mock.

@pinkforest pinkforest changed the title Move rand_core as optional feature Make rand_core optional Nov 30, 2022
@pinkforest pinkforest force-pushed the feat-rand_core-feature branch from 93b3c34 to afde1c0 Compare November 30, 2022 12:44
As proposed in dalek-cryptography#442 this makes `rand_core` an
optional feature that is not covered by the
SemVer public API stability guarantees.
src/ristretto.rs Outdated Show resolved Hide resolved
src/ristretto.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/mocks.rs Show resolved Hide resolved
@@ -36,14 +36,15 @@ bincode = "1"
criterion = { version = "0.4.0", features = ["html_reports"] }
hex = "0.4.2"
rand = "0.8"
rand_core = { version = "0.6", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose not being default is justified by saying most dependencies already specify their own usages of the lower level methods.

@rozbb
Copy link
Contributor

rozbb commented Dec 7, 2022

This is nearly ready. An annoying issue is that rand_core is actually enabled by default. This is because std enables rand_core/std. @tarcieri I don't think there's any way around this other than:

  1. Kill rand_core/std and don't let anyone use it. The thing we lose here is only impl<R: RngCore> RngCore for Box<R>.
  2. Make rand_core a default dependency, breaking our opt-in semver-breakage policy

I think (1) is the right choice. I don't think anyone will miss it.

@rozbb rozbb mentioned this pull request Dec 7, 2022
@rozbb
Copy link
Contributor

rozbb commented Dec 8, 2022

@tarcieri thoughts on killing rand_core/std? If we do this, we get the extra benefit of killing std entirely actually.

@rozbb
Copy link
Contributor

rozbb commented Dec 8, 2022

Actually this can be solved with weak dependency features if we bump MSRV to 1.60. I think I'd be fine with that actually

@tarcieri
Copy link
Contributor

tarcieri commented Dec 8, 2022

AFAIK nothing in the crate itself actually needs or uses rand_core/std.

It provides two things:

  • transitive activation of rand_core/getrandom feature which provides OsRng
  • std::error::Error impl on rand_core::Error, which is not found in curve25519-dalek's public API

OsRng is used for testing in various places. It seems we can activate rand_core/getrandom in dev-dependencies and call it a day.

@tarcieri
Copy link
Contributor

tarcieri commented Dec 8, 2022

#458 removes activation of rand_core/std

@rozbb rozbb merged commit 47a0c3e into dalek-cryptography:release/4.0 Dec 8, 2022
@tarcieri tarcieri mentioned this pull request Dec 8, 2022
@pinkforest pinkforest mentioned this pull request Dec 9, 2022
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