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

Disable std feature of rand_core by default? #702

Closed
dhardy opened this issue Jan 23, 2019 · 4 comments
Closed

Disable std feature of rand_core by default? #702

dhardy opened this issue Jan 23, 2019 · 4 comments

Comments

@dhardy
Copy link
Member

dhardy commented Jan 23, 2019

If we do this,

There isn't a lot of functionality in rand_core which is enabled by the std feature, but there is a little:

  • rand_core::Error::take_error is defined
  • rand_core::Error implements std::error::Error
  • std::io::Error implements From<rand_core::Error>
  • RngCore implements std::io::Read

Of course, this is potentially a breaking change, so we should bump the version number to 0.4.0 and have a shim 0.3 → 0.4 which still enables std by default — except, since the change is unlikely to cause much breakage and we are already breaking some builds, I wonder if it is worth it?

@dhardy
Copy link
Member Author

dhardy commented Jan 23, 2019

Note: all our rand* crates which depend on rand_core disable its default features, and only rand itself optionally enables its std feature. Interestingly, rand_jitter has optional std support implementing std::error::Error for its error type, yet works fine without requiring rand_core's std feature.

In other words: most users of rand_core probably don't need std features, and users of rand get them anyway (unless disabling its default features), so IMO this is the right way to go (and also used to be the case, to workaround a Cargo issue).

@newpavlov
Copy link
Member

Isn't it disabled by default already? AFAIK all crates enable rand_core/std only as part of their own std feature. So I don't see any problems with the current setup.

@dhardy
Copy link
Member Author

dhardy commented Jan 25, 2019

Sorry, I just merged #703. Before that, std was enabled by default. Judging by the above links, this caused several problems.

@dhardy dhardy closed this as completed Jan 25, 2019
@newpavlov
Copy link
Member

Ah, then it's indeed was a wrong behavior.

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

No branches or pull requests

2 participants