Allow external crates to define backends without RUSTFLAGS

[bushrat011899]

Background

getrandom is a foundational crate with a high level of importance across the entire Rust ecosystem. Unfortunately, the ergonomics around using this crate on fringe platforms within a library ecosystem is sub-par. Even more unfortunately, "fringe" includes Wasm in the browser, an otherwise excellent platform for Rust.

As an outsider to the project, I feel the same frustrations that have already been litigated in #658, but I also appreciate that this crate must walk a very fine line between performance, ergonomics, and security.

I've had a bit of experience in the embedded Rust ecosystem, where problems like this are quite prevalent (since there's no std to rely on for common APIs). One exemplar from that space is critical-section, which has a similar problem to solve: platform dependent backend which can only be declared once (globally) and a universal frontend. To achieve this, they use an Impl trait to describe a backend implementation, and a macro set_impl! which exports the implementation using extern "Rust" functions.

This technique has the benefit of moving the "a backend must be declared exactly once" problem into the linker, which already has such a requirement.

Objective

I'd like to demonstrate a prototype of getrandom which puts a larger design emphasis on external backends using the critical-section technique. Since a backend could only be declared exactly once, it is not possible for a rogue dependency to "sneak" an implementation in (similar to the current custom backend).

Solution

• Defined a new Backend trait which contains all methods which would previously be internally defined (e.g., fill_inner, inner_u32`, etc.).
• Created an implementation of Backend, ExternBackend, which defers all its methods to extern "Rust" functions.
• Created a macro, set_backend!, which exports a type implementing Backend as extern "Rust" functions compatible with ExternBackend.
• Switched to using ExternBackend in the implementation of the public-facing API.
• Adjusted the current backends to instead implement Backend on a unit struct, and then call set_backend! on said struct.
• Altered the current Error type to allow descriptions to be declared in Backend, allowing external backends to provide rich error messages.
• Added a feature, default-backends, which enables the current default backends. This allows getrandom to act like a core library crate, and getrandom = { features = ["default-backends"] } to be the consumer-facing one. This could also be achieved by splitting getrandom into getrandom_core and getrandom, but that's a much more controversial change.
• If a default backend is not available, allow a linking error to prevent final compilation instead of throwing an explicit compile_error!. This allows another crate in the dependency graph to provide a Backend implementation if and only if no other backend is declared. As previously stated, any attempts to "override" the backend will fail to link.

---

Notes

I haven't removed any of the current requirements for RUSTFLAGS usage for things like wasm_js. The goal of this change is to instead allow a 3rd party crate to cleanly be included by the user (or a relevant library such as Bevy, HAL, etc.) and provide the backend implementation. Since this is offloaded to a 3rd party dependency, it is substantially harder for a misconfigured library to break Wasm (non-web) compilation by simply activating a feature. Instead, they must bring in a whole extra dependency. This is also a benefit for libraries, as they can simply add such an implementation to dev-dependencies for CI.

I'm opening this PR in draft to highlight that I don't believe this is ready to just merge as-is; I want to start a discussion on practical changes that could be made to benefit ergonomics without sacrificing the other critical focus areas of the project.

[josephlr]

Thanks so much for looking into this @bushrat011899, I've opened #671 as an issue to track ergonomics around alternative backends. I'll try to take a look at this later this week to see if this would be an option for 0.3 or something we would want to do for a future major version.

[newpavlov]

On the first glance the proposed approach looks really messy. IIUC you effectively just expand the custom opt-in backend and use it for all backends. Relying on extern fns by default is not great. Not only it exposes symbols which may not be desirable (e.g. see #666), but it also can result in really awful linking errors.

[bushrat011899]

On the first glance the proposed approach looks really messy. IIUC you effectively just expand the custom opt-in backend and use it for all backends.

Yes, that's a good summary of the technique here. Part of the messiness I want to attribute to this being my first time actually looking at getrandom's internals (let alone a major refactor therein!). This is why I'm only opening this as a draft for discussion at this time, and I'm very grateful that is happening (thanks!)

I do think this trait-based approach is a direct improvement over the current custom backend, as it allows the u32 and u64 methods to also be declared externally. It also provides a clear path for evolution on the API in the future, since traits can have default implementations, but extern functions can not. So if the API was extended to include a u128 method (hypothetical), it could be added as a minor update.

Relying on extern fns by default is not great.

Agreed, we can make the default experience nicer by making the extern-backend a feature (critically not using RUSTFLAGS). The internal backends would just export their actual backend under a common name (similar to the existing fill_inner techique), allowing first-class platforms to entirely bypass the external linking.

Not only it exposes symbols which may not be desirable (e.g. see #666), but it also can result in really awful linking errors.

Agreed that exposing extra symbols isn't desirable. Perhaps I could refactor the technique to store a static V-table, so that only a single symbol is exported? This might have a negative effect on performance due to indirection and reduced possibility of LTO though.

As for the linking errors, I agree, they're painful to diagnose and resolve. But, this technique allows HALs to test and provide their own backend without user intervention (via RUSTFLAGS), which can be a much nicer experience. Again, looking at critical-section, crates like agb can provide an Impl implementation and activate it for the user, which in that context makes total sense; if you're using agb, you're compiling for the GameBoy Advance, and there is a known locking primitive that should be used.

This is speculation, but I also imagine it would be more likely for Rust to get better support for diagnostic warnings and custom linking errors messages than it would be to get something like binary-only features.

[newpavlov]

For historic reasons getrandom intentionally does not introduce its own traits. Instead we rely on the rand_core crate to do it. You could argue that getrandom should provide OsRng which implements rand_core::RngCore (instead of defining it in rand_core by wrapping getrandom functions), but it would mean that rand_core will not be able to provide the SeedableRng::from_os_rng method. SeedableRng::from_rng(&mut getrandom::OsRng) could work fine as an alternative, but we certainly do not want to change it in getrandom v0.3 and rand_core v0.9. We may change it in future breaking releases, but we do not plan to make them in the near future. Either way, it could be worth to raise such proposal in a rand repository issue.

Overall, I don't think that the cfg-based approach is that bad to go to such lengths to work around it. It's certainly somewhat unusual (at the very least it's not less unusual than relying on the extern fn hack), but I believe it's practical enough. If you want to improve ergonomics, then I think it would be better to advocate for addition of a configuration flags support to Cargo (e.g. something like this).

[janhohenheim]

@newpavlov coming in as an outsider, it's really jarring for me when I'm developing a game with Bevy, unknowingly pull in a dependency that uses getrandom transiently, and suddenly my Wasm builds are broken.
I've since learned that I can put the following in my global ~/.cargo/config.toml:

[target.wasm32-unknown-unknown]
rustflags = [
  "--cfg",
  "getrandom_backend=\"wasm_js\"",
]

If I'm collaborating with others, I always need to teach them this hack. Checking it into git in my_project/.cargo/config.toml is not a good option because it will override ~/.cargo/config.toml, which would contain the very crucial -Zthreads=0 and -Zshare-generics=y for people using nightly.
Or I could add a custom run scripts or cargo command, which is again extra stuff I need to teach others to use instead of running cargo run.

Another issue is that requiring RUSTFLAGS also results in too broad application of these settings, see DioxusLabs/dioxus#4146 (comment)

I understand that the current solution is very reasonable for direct dependents, but it's also source of repeated friction and confusion for transients dependents. I also agree that making cargo support these kinds of flags would be the best solution, but historically, cargo moves very slowly with these kind of feature requests. At least, in my experience.

Do you think you could reconsider your stance? Or is there a change that can be made to this PR so that you would accept it? :)

[jkelleyrtp]

For historic reasons getrandom intentionally does not introduce its own traits. Instead we rely on the rand_core crate to do it. You could argue that getrandom should provide OsRng which implements rand_core::RngCore (instead of defining it in rand_core by wrapping getrandom functions), but it would mean that rand_core will not be able to provide the SeedableRng::from_os_rng method. SeedableRng::from_rng(&mut getrandom::OsRng) could work fine as an alternative, but we certainly do not want to change it in getrandom v0.3 and rand_core v0.9. We may change it in future breaking releases, but we do not plan to make them in the near future. Either way, it could be worth to raise such proposal in a rand repository issue.

Overall, I don't think that the cfg-based approach is that bad to go to such lengths to work around it. It's certainly somewhat unusual (at the very least it's not less unusual than relying on the extern fn hack), but I believe it's practical enough. If you want to improve ergonomics, then I think it would be better to advocate for addition of a configuration flags support to Cargo (e.g. something like this).

just chiming in that crates who rely on getrandom 0.2 and migrate to getrandom 0.3 (uuid for example) now introduce a semver-incompatible change breaking old builds for no good reason.

we've been just pinning rand to 0.8 and not think about this problem, but more and more of the ecosystem is migrating to rand 0.9 and more and more of our builds stop working. As a maintainer of a large project, having to explain to users that they need to start mucking with rustflags just to get random numbers in their web apps is a tough sell. Every papercut counts.

Do you need someone with expertise here to fix the implementation? I'm more than happy to put some time into fixing the underlying issue, with or without traits. We want to fund cargo to get global non-unified features in the long run, but that would be a 6-12 month fix when I think with enough thought we could get a 2 week fix.

[bushrat011899]

Closing in favour of #672, which based on feedback here is much more likely to be actionable.

[newpavlov]

I answered to the comments here.