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

rand: Remove serde1 feature #830

Merged
merged 2 commits into from Jun 25, 2019
Merged

rand: Remove serde1 feature #830

merged 2 commits into from Jun 25, 2019

Conversation

vks
Copy link
Collaborator

@vks vks commented Jun 24, 2019

This feature does not do anything in the rand crate and was forwarded
to rand_core, where it was used for BlockRng and BlockRng64.
However, crates using this feature would use rand_core directly
instead, making the feature pointless for rand.

It is not expected to use this feature for any of the RNGs in rand.

This hopefully makes it possible to build rand on docs.rs again.
(See rust-lang/docs.rs#369.)

Now the serde1 feature does not do anything, but it is still available
for backwards compatibility.

This feature does not do anything in the `rand` crate and was forwarded
to `rand_core`, where it was used for `BlockRng` and `BlockRng64`.
However, crates using this feature would use `rand_core` directly
instead, making the feature pointless for `rand`.

It is not expected to use this feature for any of the RNGs in `rand`.

This hopefully makes it possible to build `rand` on docs.rs again.
(See rust-lang/docs.rs#369.)

Now the serde1 feature does not do anything, but it is still available
for backwards compatibility.
@vks vks requested a review from dhardy June 24, 2019 15:43
- `rand_xoshiro` should be built with the feature.
-  For `rand`, it does not do anything. Instead, test it for
   `rand_core`.
@vks
Copy link
Collaborator Author

vks commented Jun 24, 2019

The failures seem to be due to "store build cache" timing out on Travis.

@dhardy
Copy link
Member

dhardy commented Jun 24, 2019

You're right that we don't need to pass this to rand_core since e.g. rand_isaac does that when required.

I think the only reason we might want this is if we enabled serialisation for SmallRng, but currently we don't allow that anyway (with good reason since its not portable; OTOH it could still be useful for local checkpointing).

@vks
Copy link
Collaborator Author

vks commented Jun 25, 2019

I think the only reason we might want this is if we enabled serialisation for SmallRng, but currently we don't allow that anyway (with good reason since its not portable; OTOH it could still be useful for local checkpointing).

I don't think it makes sense to enable it for SmallRng, since we don't guarantee value stability, which also affects deserializing SmallRng. If people want to serialize the state, I think it is better to use one of the more stable RNG crates directly.

@dhardy dhardy merged commit fb7b260 into rust-random:master Jun 25, 2019
@vks vks deleted the serde-features branch June 25, 2019 11:55
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.

None yet

2 participants