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

Update docs for 0.2 Release #135

Merged
merged 10 commits into from
Sep 10, 2020
Merged

Update docs for 0.2 Release #135

merged 10 commits into from
Sep 10, 2020

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Feb 26, 2020

Final PR for 0.2 adding all the docs about the "rdrand", "js", and "custom" features.

To preview what will go up on crates.io, run:

git clone --single-branch --branch docs https://github.com/josephlr/getrandom.git
cd getrandom
cargo doc --no-deps --features=std,custom --open`

Signed-off-by: Joe Richey joerichey@google.com

@josephlr josephlr added this to the 0.2 milestone Feb 26, 2020
@josephlr josephlr mentioned this pull request Feb 26, 2020
9 tasks
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Good start on this.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@newpavlov
Copy link
Member

@dhardy
This PR is half-year old. We probably should commit your edits and move forward with 0.2 release. I would like to go back to rust-lang/rust#62079.

@josephlr
Copy link
Member Author

@newpavlov I 100% agree. I'll get this ready for final review ASAP. I also now have a strong work reason (briansmith/ring#744 (comment)) to make sure getrandom 0.2 is out asap

@josephlr josephlr changed the title WIP: Update docs for 0.2 Release Update docs for 0.2 Release Sep 8, 2020
@josephlr
Copy link
Member Author

josephlr commented Sep 8, 2020

@newpavlov @dhardy the docs are complete and are ready for review

src/custom.rs Outdated Show resolved Hide resolved
src/stdweb.rs Outdated Show resolved Hide resolved
The main things here are clarifying how fallback functionatliy works.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member Author

josephlr commented Sep 8, 2020

@stevenrutherford you've used this branch before, can you take a look at the docs.

Signed-off-by: Joe Richey <joerichey@google.com>
Include more links, exampales, and clarify wording

Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member Author

josephlr commented Sep 8, 2020

@newpavlov one last question (then we can merge this).

Right now the documented way to use a custom-RNG crate is:

  1. In the custom RNG crate (i.e. dummy-getrandom)
    • Depend on getrandom in the Cargo.toml with the "custom" feature enabled
    • Define a function (i.e. always_fail) with signature fn() -> Result<(), getrandom::Error>
    • Call register_custom_getrandom!(always_fail);
  2. In the binary crate
    • Depend on dummy-getrandom in the Cargo.toml.
    • Use the crate in src/main.rs by having use dummy_getrandom; at the top of the file.
      • Failure to do this step creates linker errors.
    • Binary crate doesn't need to depend on getrandom.

An alternative way to do things would be:

  1. In the custom RNG crate (i.e. dummy-getrandom)
    • Depend on getrandom in the Cargo.toml (no need to select any features)
    • Define a public function (i.e. always_fail) with signature fn() -> Result<(), getrandom::Error>
  2. In the binary crate
    • Depend on dummy-getrandom in the Cargo.toml.
    • Depend on getrandom in the Cargo.toml with the "custom" feature enabled
    • Have the following code in src/main.rs:
      use getrandom::register_custom_getrandom;
      use dummy_getrandom::always_fail;
      register_custom_getrandom!(always_fail);

Do you think we should recommend the current approach or the alternative approach? I think the alternative approch better mimics common patterns with #[panic_handler] and #[global_allocator].

EDIT: The alternative approach probably makes it harder to get linker errors, at the cost of having the user do more in the binary crate.

@newpavlov
Copy link
Member

Hm, I agree that the second approach is closer to the global allocator and panic handlers, plus it feels "less magic". We could reduce amount of setup a bit, by suggesting that dummy-getrandom could depend on getrandom with the enabled custom feature and re-export it, so users could write:

use dummy_getrandom::{always_fail, getrandom::register_custom_getrandom};
register_custom_getrandom!(always_fail);

But I am not sure if it's worth the trouble.

Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
This makes sure we get a good compiler error if we give a bad path.

Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member Author

josephlr commented Sep 9, 2020

Hm, I agree that the second approach is closer to the global allocator and panic handlers, plus it feels "less magic".

That was my thinking as well, docs have been updated to show the second approach. I also added a check in the macro (8e1ab1a) that the function has the exact correct signature. This makes the error messages much better if you feed it a bad function.

But I am not sure if it's worth the trouble.

Ya I agree. I think keeping it sort of close to how #[global_allocator] works is for the best.

@dhardy
Copy link
Member

dhardy commented Sep 9, 2020

Moving the binding to the binary crate does avoid possible confusion about how custom RNG crates work, and about what happens if multiple custom RNG crates were enabled. On the other hand it's a bit strange and inconvenient putting the binding in a binary crate if the binary crate doesn't use getrandom itself (e.g. if just using rand).

@josephlr
Copy link
Member Author

josephlr commented Sep 9, 2020

On the other hand it's a bit strange and inconvenient putting the binding in a binary crate if the binary crate doesn't use getrandom itself (e.g. if just using rand).

I agree it's a little inconvenient, but it does make things less magical. It's also consistent with the "js" and "rdrand" features, which are also enabled by directly depending on getrandom (even if the user was only using rand).

Fundamentally, if a user is doing this, they need to know they are overriding getrandom, otherwise they'll have issues/conflicts down the road.

I also like this approach because it's compatible with the eventual introduction of a #[random_handler] lang item as part of core/alloc/std.

src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member Author

I added target names to the table and it actually looks pretty good:

table

This should also help people who are confused about macos vs Darwin, or what windows UWP is.

@josephlr josephlr merged commit b929998 into rust-random:0.2 Sep 10, 2020
@josephlr josephlr deleted the docs branch September 10, 2020 07:45
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.

4 participants