Skip to content

Upgrade rand crate or make it optional #938

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

Closed
dmitmel opened this issue Nov 1, 2019 · 3 comments · Fixed by #940
Closed

Upgrade rand crate or make it optional #938

dmitmel opened this issue Nov 1, 2019 · 3 comments · Fixed by #940

Comments

@dmitmel
Copy link
Contributor

dmitmel commented Nov 1, 2019

Problem

The rand crate is one of the dependencies of this crate. However, it is used in literally only one place:

impl Distribution<Color> for Standard {
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Color {
if rng.gen() {
Color::RGBA(rng.gen(), rng.gen(), rng.gen(), rng.gen())
} else {
Color::RGB(rng.gen(), rng.gen(), rng.gen())
}
}
}

First of all, while random number generation is a common requirement in graphical applications (e.g. games) it is not always. Although this is not my main concern. The sdl2 crate depends on an outdated version of rand, ^0.6. This results in the following dependency graph inside sdl2:

$ cargo tree --package rand
rand v0.6.5
├── libc v0.2.65
├── rand_chacha v0.1.1
│   └── rand_core v0.3.1
│       └── rand_core v0.4.2
│   [build-dependencies]
│   └── autocfg v0.1.7
├── rand_core v0.4.2 (*)
├── rand_hc v0.1.0
│   └── rand_core v0.3.1 (*)
├── rand_isaac v0.1.1
│   └── rand_core v0.3.1 (*)
├── rand_jitter v0.1.4
│   └── rand_core v0.4.2 (*)
├── rand_os v0.1.3
│   ├── libc v0.2.65 (*)
│   └── rand_core v0.4.2 (*)
├── rand_pcg v0.1.2
│   └── rand_core v0.4.2 (*)
│   [build-dependencies]
│   └── autocfg v0.1.7 (*)
└── rand_xorshift v0.1.1
    └── rand_core v0.3.1 (*)

See? This pulls in two different versions of rand_core - 0.3.1 and 0.4.2. Obvious code duplication. The total number of dependencies of sdl2 is 41.

Solution

This simple patch reduces dependency count to 33 and removes duplication of the mentioned crate rand_core:

diff --git a/Cargo.toml b/Cargo.toml
index 4814dff8..8bf29c2c 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,7 +18,7 @@ path       = "src/sdl2/lib.rs"
 [dependencies]
 bitflags = "^1"
 libc = "^0.2"
-rand = "^0.6"
+rand = "^0.7"
 lazy_static = "^1"
 
 [dependencies.num]

However, we can go even further and make rand an optional dependency with this patch:

diff --git a/Cargo.toml b/Cargo.toml
index 4814dff8..8e124218 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,9 +18,12 @@ path       = "src/sdl2/lib.rs"
 [dependencies]
 bitflags = "^1"
 libc = "^0.2"
-rand = "^0.6"
 lazy_static = "^1"
 
+[dependencies.rand]
+version = "^0.7"
+optional = true
+
 [dependencies.num]
 version = "^0.1"
 default-features = false
@@ -36,7 +39,7 @@ optional = true
 [features]
 
 unsafe_textures = []
-default = []
+default = ["rand"]
 gfx = ["c_vec", "sdl2-sys/gfx"]
 mixer = ["sdl2-sys/mixer"]
 image = ["sdl2-sys/image"]
diff --git a/src/sdl2/pixels.rs b/src/sdl2/pixels.rs
index c2d1d56d..08ddd818 100644
--- a/src/sdl2/pixels.rs
+++ b/src/sdl2/pixels.rs
@@ -1,6 +1,5 @@
+#[cfg(rand)]
 extern crate rand;
-use self::rand::Rng;
-use self::rand::distributions::{Distribution, Standard};
 
 use num::FromPrimitive;
 use std::mem::transmute;
@@ -171,8 +170,9 @@ impl From<(u8, u8, u8, u8)> for Color {
     }
 }
 
-impl Distribution<Color> for Standard {
-    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Color {
+#[cfg(rand)]
+impl self::rand::Distribution<Color> for self::rand::Standard {
+    fn sample<R: self::rand::Rng + ?Sized>(&self, rng: &mut R) -> Color {
         if rng.gen() {
             Color::RGBA(rng.gen(), rng.gen(), rng.gen(), rng.gen())
         } else {

Summary

Right now I propose to only update the version of rand dependency because this is a trivial change. Although I also would like the developers of rust-sdl2 to consider making rand an optional dependency (but only if the implications of such change won't be too significant).

@Cobrand
Copy link
Member

Cobrand commented Nov 3, 2019

I'm all for changing rand as an optional dependency (and upgrade it). Very few users will be using rand with Sdl2, most of them who need it will use a wrapper around SdlColor and implement Rand for that.

We could even argue it's not used at all and remove it entirely, I'm sure no one will say anything (you can replicate this code with a few lines regardless).

@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 5, 2019

@Cobrand I can open a PR if you wish, however, what should I do for now? Just upgrade rand, make it an optional dependency or remove it altogether?

Also, how often a new version of sdl2 is published to crates.io? And how stable is the master branch?

@Cobrand
Copy link
Member

Cobrand commented Nov 5, 2019

I think remove it altogether is the better choice.

There is no fixed schedule for crates.io updates, I usually do it when I have time to clean the changelog and make sure everything works alright with the past few PRs. The master branch is pretty unstable, you can make breaking changes, people are expected to use the versions from crates.io.

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.

2 participants