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

Bump versions + Edition 2018 + Pcg64 #823

Merged
merged 12 commits into from
Jun 12, 2019
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jun 11, 2019

This combines a few things because we need a new minor version for several crates, hence:

  • switch to Edition 2018 for these crates
  • serde support is now available with either the serde or serde1 feature for most of these, excluding rand_isaac (which requires rand_core's serde1), though doc still says use serde1
  • I added Pcg64 (been on my to-do list a while)

@vks
Copy link
Collaborator

vks commented Jun 11, 2019

What's the motivation for using the serde instead of the serde1 feature?

@vks
Copy link
Collaborator

vks commented Jun 11, 2019

Looks good otherwise. I don't understand the test failures on Linux beta and stable:

error[E0601]: `main` function not found in crate `monte_carlo`
  |
  = note: consider adding a `main` function to `examples/monte-carlo.rs`
error[E0601]: `main` function not found in crate `monty_hall`
  |
  = note: consider adding a `main` function to `examples/monty-hall.rs`

@dhardy
Copy link
Member Author

dhardy commented Jun 11, 2019

Just because we can and some users may expect it to be just serde. Not a very good reason, which is why I didn't change the docs. 🤷‍♂️

I don't understand the failure either. Of course, we know those examples don't work with no_std due to use of thread_rng but why commit 01b1bf8 causes the command cargo test --no-default-features to fail is a head-scratcher.

@dhardy
Copy link
Member Author

dhardy commented Jun 11, 2019

Note: I can fix the examples to work without std, but then all the doc-tests fail. On most no-std tests we already use the --tests flag to avoid this issue.

    #[cfg(feature = "std")]
    let mut rng = rand::thread_rng();
    #[cfg(not(feature = "std"))] // no support for thread_rng
    let mut rng = <rand::rngs::StdRng as rand::SeedableRng>::seed_from_u64(0);

Note: Cargo patch was used since rand_os dependend on
rdrand on sgx. Now getrandom has its own rdrand impl.
@vks
Copy link
Collaborator

vks commented Jun 11, 2019

Just because we can and some users may expect it to be just serde. Not a very good reason, which is why I didn't change the docs. man_shrugging

I think this might be problematic if there is a serde2. Updating serde to be equivalent to serde2 would be a breaking change. Isn't that the motivation to have serde1 in the first place?

@dhardy
Copy link
Member Author

dhardy commented Jun 11, 2019

Yes, and that's precisely why I didn't document it (due to the way optional deps show up as features, one should never consider an undocumented feature a stable part of the API IMO). But you may be right that it's better simply not to implement serde support without serde1.

@dhardy
Copy link
Member Author

dhardy commented Jun 12, 2019

Reverted and pushed version bump for next RC

@dhardy dhardy merged commit 5610bda into rust-random:master Jun 12, 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.

None yet

2 participants