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

Use getrandom crate for uuid #447

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Use getrandom crate for uuid #447

merged 4 commits into from
Jan 28, 2020

Conversation

ammgws
Copy link
Contributor

@ammgws ammgws commented Jan 17, 2020

I'm submitting a(n) other

Description

Use getrandom crate directly instead of rand for generating UUIDs using v4.

Motivation

Reduce the number of dependencies used by the crate.

Running the example in the README:

  • Before (10): c2-chacha, cfg-if, getrandom, libc, ppv-lite86, rand, rand_chacha, rand_core, rand_hc, wasi
  • After (4): cfg-if, getrandom, libc, wasi

Tests

cargo test locally and test usage in a simple program.

Related Issue(s)

Other

Wasn't sure whether I should bump the version here or not.

Copy link
Member

@kinggoesgaming kinggoesgaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few problems I see. See my relevant comments

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/v4.rs Outdated Show resolved Hide resolved
src/v4.rs Outdated Show resolved Hide resolved
src/v4.rs Outdated Show resolved Hide resolved
@ammgws ammgws changed the title Use getrandom crate for uuid [WIP] Use getrandom crate for uuid Jan 19, 2020
@ammgws ammgws force-pushed the getrand branch 6 times, most recently from 97e9bcf to bcc2fb1 Compare January 19, 2020 02:46
@ammgws ammgws changed the title [WIP] Use getrandom crate for uuid Use getrandom crate for uuid Jan 19, 2020
Reduces the number of dependencies.
Copy link
Member

@kinggoesgaming kinggoesgaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you accept the changes, I can merge. Later when I get the time, I will actually change the signature.

src/v4.rs Show resolved Hide resolved
/// This uses the [`rand`] crate's default task RNG as the source of random
/// numbers. If you'd like to use a custom generator, don't use this
/// method: use the `rand::Rand trait`'s `rand()` method instead.
/// This uses the [`getrandom`] crate to utilise the operating system's RNG
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KodrAus should we expose the fact that we are using the getrandom crate implementation in the docs?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/v4.rs Outdated Show resolved Hide resolved
@kinggoesgaming
Copy link
Member

Thanks @ammgws for working on this :)

ammgws and others added 3 commits January 21, 2020 19:04
Co-Authored-By: Hunar Roop Kahlon <[email protected]>
Co-Authored-By: Hunar Roop Kahlon <[email protected]>
Co-Authored-By: Hunar Roop Kahlon <[email protected]>
@ammgws
Copy link
Contributor Author

ammgws commented Jan 21, 2020

Shall I rebase this?

@kinggoesgaming
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Jan 28, 2020
447: Use getrandom crate for uuid r=kinggoesgaming a=ammgws

<!--
    If this PR is a breaking change, ensure that you are opening it against 
    the `breaking` branch.  If the pull request is incomplete, prepend the Title with WIP: 
-->

**I'm submitting a(n)** other

# Description

Use `getrandom` crate directly instead of `rand` for generating UUIDs using v4.

# Motivation

Reduce the number of dependencies used by the crate.

Running the example in the README: 
- Before (10): c2-chacha, cfg-if, getrandom, libc, ppv-lite86, rand, rand_chacha, rand_core, rand_hc, wasi  
- After (4): cfg-if, getrandom, libc, wasi

# Tests
<!-- How are these changes tested? -->
`cargo test` locally and test usage in a simple program.

# Related Issue(s)

# Other

Wasn't sure whether I should bump the version here or not.


Co-authored-by: Jason Nader <[email protected]>
Co-authored-by: Jason <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jan 28, 2020

@bors bors bot merged commit 4899012 into uuid-rs:master Jan 28, 2020
@ammgws ammgws deleted the getrand branch January 28, 2020 09:46
bors bot added a commit that referenced this pull request Dec 21, 2020
503: Un-breaking-change Uuid::new_v4 r=KodrAus a=CAD97

<!--
    If this PR is a breaking change, ensure that you are opening it against 
    the `breaking` branch.  If the pull request is incomplete, prepend the Title with WIP: 
-->

**I'm submitting a(n)** refactor

# Description

#447 changed from using `rand::thread_rng` to using `getrandom` in `Uuid::new_v4`. This also changed the return type from `Uuid` to `Result<Uuid, getrandom::Error>`. This PR reverts the signature to the simpler `new_v4() -> Uuid`.

# Motivation

This signature is much simpler to use, and avoids a breaking change. `getrandom` is _highly_ unlikely to fail, and previously we used `thread_rng` here, which also panics if it fails to initialize from the OS entropy source. If `getrandom` fails, it is highly unlikely that any program creating v4 UUIDs has any reasonable recovery other than to abort, as the system is in a broken state.

If users absolutely need to recover in this situation, they can call `getrandom` first themselves to make sure that their system is working, or generate the bytes themselves and create the UUID from those bytes.

Additionally, actually wrapping `getrandom::Error` in `uuid::Error` comes with its own fun set of problems, described in #502.

# Tests

N/A

# Related Issue(s)

#475: this undoes the breaking change to `Uuid::new_v4`, thus making the requested publish a patch update

Closes #502: alternate approach to the same TODO

Co-authored-by: CAD97 <[email protected]>
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.

2 participants