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

Consider hiding StdRng behind a feature flag #938

Closed
matklad opened this issue Feb 17, 2020 · 6 comments · Fixed by #948
Closed

Consider hiding StdRng behind a feature flag #938

matklad opened this issue Feb 17, 2020 · 6 comments · Fixed by #948

Comments

@matklad
Copy link
Contributor

matklad commented Feb 17, 2020

Hi!

So I've noticed that even if I use rand with what I think is the minimal profile,

rand = { version = "0.7.1", features = [ "small_rng" ], default_features = false }

I still get both chacha (with its deps) and hc.

However, I am specifically interested in a single fast non-cryptographic manually seeded rng (and small Cargo.lock :) ).

I've tried to apply the following patch to rand, and it seems like it did the trick?

diff --git a/Cargo.toml b/Cargo.toml
index c1a0fe3..b72d3b2 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -39,6 +39,7 @@ getrandom = ["getrandom_package", "rand_core/getrandom"]
 # Configuration:
 simd_support = ["packed_simd"] # enables SIMD support
 small_rng = ["rand_pcg"]    # enables SmallRng
+std_rng = ["rand_chacha", "rand_hc" ]
 
 [workspace]
 members = [
@@ -78,9 +79,9 @@ libc = { version = "0.2.22", default-features = false }
 # Emscripten does not support 128-bit integers, which are used by ChaCha code.
 # We work around this by using a different RNG.
 [target.'cfg(not(target_os = "emscripten"))'.dependencies]
-rand_chacha = { path = "rand_chacha", version = "0.2.1", default-features = false }
+rand_chacha = { path = "rand_chacha", version = "0.2.1", default-features = false, optional = true }
 [target.'cfg(target_os = "emscripten")'.dependencies]
-rand_hc = { path = "rand_hc", version = "0.2" }
+rand_hc = { path = "rand_hc", version = "0.2", optional = true }
 
 [dev-dependencies]
 rand_pcg = { path = "rand_pcg", version = "0.2" }
diff --git a/src/prelude.rs b/src/prelude.rs
index 3c386e8..451faad 100644
--- a/src/prelude.rs
+++ b/src/prelude.rs
@@ -19,6 +19,7 @@
 //! ```
 
 #[doc(no_inline)] pub use crate::distributions::Distribution;
+#[cfg(feature="std_rng")]
 #[doc(no_inline)] pub use crate::rngs::StdRng;
 #[cfg(feature="small_rng")]
 #[doc(no_inline)] pub use crate::rngs::SmallRng;
diff --git a/src/rngs/mod.rs b/src/rngs/mod.rs
index abf3243..aa8f5cf 100644
--- a/src/rngs/mod.rs
+++ b/src/rngs/mod.rs
@@ -105,6 +105,7 @@ pub mod mock;   // Public so we don't export `StepRng` directly, making it a bit
                 // more clear it is intended for testing.
 #[cfg(feature="small_rng")]
 mod small;
+#[cfg(feature="std_rng")]
 mod std;
 #[cfg(feature="std")] pub(crate) mod thread;
 
@@ -113,6 +114,7 @@ mod std;
 
 #[cfg(feature="small_rng")]
 pub use self::small::SmallRng;
+#[cfg(feature="std_rng")]
 pub use self::std::StdRng;
 #[cfg(feature="std")] pub use self::thread::ThreadRng;
 
@dhardy
Copy link
Member

dhardy commented Feb 18, 2020

I'm not keen on this. ThreadRng also uses ChaCha. I mean, yes, we can make things more optional/modular, but I think it's also worth providing default options that cover most users' requirements?

@vks @newpavlov thoughts?

@matklad
Copy link
Contributor Author

matklad commented Feb 18, 2020

Yeah, it should definitely be an default feature. But I don't used ThreadRng, b/c I don't use std feature, so for me that would be an easy way to cut about 4 entries from Cargo.lock, and I do find that valuable.

@newpavlov
Copy link
Member

newpavlov commented Feb 18, 2020

I am in support of adding an enabled-by-default feature for StdRng, thus making all dependencies except rand_core optional. But I am afraid it will be a breaking change, since crates which use rand without default features can use StdRng, so adding this feature will result in a build failure for them.

@vks
Copy link
Collaborator

vks commented Feb 18, 2020 via email

@matklad
Copy link
Contributor Author

matklad commented Feb 18, 2020

Are you using the emscripten target?

No, but it still ends up in Cargo.lock, because Cargo.lock is created for a union of all targerts.

@dhardy
Copy link
Member

dhardy commented Feb 19, 2020

Fair enough, I guess we can introduce an on-by-default feature flag.

Next question is, one or two? We have no guarantee that ThreadRng and StdRng will continue to use the same CSPRNG, although that will probably be the case, so a single std_rng flag is probably okay.

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 a pull request may close this issue.

4 participants