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

Consider not forwarding the getrandom/custom feature #36

Open
josephlr opened this issue Jun 29, 2022 · 0 comments
Open

Consider not forwarding the getrandom/custom feature #36

josephlr opened this issue Jun 29, 2022 · 0 comments

Comments

@josephlr
Copy link

As one the maintainers of getrandom, I would advise against your getrandom_custom feature. Generally speaking, this feature is set by a user who is registering a custom handler (as explained in our docs) and not by a library that just calls getrandom::getrandom. The reason for this is because if a user enables the feature, but does not register a handler, they will get a confusing linker error.

It seems like this feature addition was done to reexport the register_custom_getrandom macro, but a user of nanorand-rs can already just register a handler by just directly depending on getrandom. It should also be noted that directly exporting our macro creates a dependency hazard, as the macro requires a function that returns getrandom::Error. So you would not be able to internally update your version of getrandom (say from 0.2 to 1.0) without having a breaking change yourself. Without this feature, you would be able to update getrandom without needing to release a breaking change in nanorand.

I couldn't find any discussion or PR about why 34ba243 was added. However, it seems like none of the released versions include it yet, so it could be reverted without too much issue.

All the other uses of getrandom's features seem reasonable to me. You could probably unconditionally enable the rdrand feature (even on non-x86 targets) as it has no effect on those targets. Keeping the js feature enabling separate is a good idea due to issues around feature unification and tkaitchuck/aHash#95 (comment)

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

1 participant