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

Please consider undeprecating rngs::adapter::ReadRng<File> #1212

Closed
g-k opened this issue Jan 19, 2022 · 4 comments
Closed

Please consider undeprecating rngs::adapter::ReadRng<File> #1212

g-k opened this issue Jan 19, 2022 · 4 comments
Milestone

Comments

@g-k
Copy link

g-k commented Jan 19, 2022

Background

refs: #1130

What is your motivation?

It's used to implement --random-source=FILE for the shuf command here.

What type of application is this?

A "Cross-platform Rust rewrite of the GNU coreutils"

Feature request

Consider undeprecating rngs::adapter::ReadRng<File> or confirm that it will stay deprecated and need to be reimplemented in downstream projects. Thanks!

@dhardy
Copy link
Member

dhardy commented Jan 19, 2022

Thanks for the request — it only took eight months to find a legitimate use of ReadRng!

Personally I'm fine with undeprecating ReadRng, though it has very few users so we'll probably introduce a read_rng feature gate. Lets not do anything immediately though — it'll stay in a deprecated state until 0.9, and we'll make a decision then. That allows some time for further comment.

@vks
Copy link
Collaborator

vks commented Jan 19, 2022

Would it be feasible to move the code downstream? It's not a lot of code.

I would recommend to deprecate --random-source=FILE and replace it with the possibility to set the random seed. Like ReadRng, it is a footgun: It can run out of random data and it's non-trivial to generate large amounts of unpredictable random data.

@newpavlov
Copy link
Member

I am in favor of keeping it deprecated. It's not a lot of code, so it can be easily implemented in a stand alone crate or copied into a project, while having it part of rand could be misleading considering the footgunny nature of it (i.e. users may use it instead of relying on OsRng).

@g-k
Copy link
Author

g-k commented Jan 20, 2022

Whoops @tertsdiepraam pointed out that this was already discussed in #1152.

Thanks for your quick input! Given that:

  • uutils/coreutils is the only known, public user of ReadRng
  • ReadRng presents a potential footgun for other rand crate users (as vks and newpavlov pointed out)
  • it's not a lot of code to copy (per newpavlov)
  • --random-source=FILE is an outdated flag

I'll close this and copy the relevant code into uutils/coreutils or see if uutils wants to switch to a --random-seed flag.

@g-k g-k closed this as completed Jan 20, 2022
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

4 participants