Use getrandom crate #62082
Use getrandom crate #62082
Conversation
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Co-Authored-By: Joe ST <[email protected]>
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
kennytm
added
S-blocked
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
BTW can you help with updating |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
If we do this then I feel that getrandom should be moved to rust-lang/rust so that the libs team can have some supervision over the crate. |
|
Do you mean moved to rust-lang organization? I don't see why it should be specifically part of rust-lang/rust. @dhardy |
|
That would be one less thing I need to keep an eye on, so fine by me. But what about you — you did much of the work on this. Also @josephlr has done a lot of work on the crate recently. The other thing worth mentioning is rust-random/getrandom#21 — not my idea, but previously the idea of adding some type of |
|
I think that moving it to rust-lang will improve visibility of the crate and will help with reviews, so personally I am all for it (I mean moving crate to rust-lang org, not copy-pasting code into rust-lang/rust). Note that this PR and parent issue do not propose to expose |
src/libstd/Cargo.toml
Outdated
| @@ -24,6 +24,7 @@ compiler_builtins = { version = "0.1.16" } | |||
| profiler_builtins = { path = "../libprofiler_builtins", optional = true } | |||
| unwind = { path = "../libunwind" } | |||
| hashbrown = { version = "0.4.0", features = ['rustc-dep-of-std'] } | |||
| getrandom = "0.1" | |||
There was a problem hiding this comment.
Caveat: this won't work on bare WASM since getrandom currently requires a feature flag to enable either stdweb or wasm_bindgen. There was mention of making wasm_bindgen the default; we might want to do this first?
There was a problem hiding this comment.
Personally I would prefer if a separate target will be introduced and wasm32-unknown-wunknown will stay assumptions-free regarding executor environment.
There was a problem hiding this comment.
IIUC without enabled stdweb or wasm_bindgen features outside of WASI and Emscripten it will use a dummy impl, so it will work in the same way as it does today by using a constant seed value.
|
Heads-up: this will use |
|
Interesting. Then we should also CC @ebarnard and consider whether we want to adjust |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Co-Authored-By: Oliver Middleton <[email protected]>
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
It's alive! @sfackler |
| @@ -25,6 +25,9 @@ profiler_builtins = { path = "../libprofiler_builtins", optional = true } | |||
| unwind = { path = "../libunwind" } | |||
| hashbrown = { version = "0.4.0", features = ['rustc-dep-of-std'] } | |||
|
|
|||
| [target.'cfg(not(all(target_arch = "wasm32", target_vendor = "unknown", target_os = "unknown")))'.dependencies] | |||
There was a problem hiding this comment.
Due to the usage of if cfg! the getrandom crate needs to be avialable on wasm32-unknown-unknown, and I think that's fine in that this header shouldn't be necessary anyway
There was a problem hiding this comment.
Without an enabled dummy,stdweb or wasm-bindgen feature the current version of getrandom will emit a compilation error when compiled for wasm32-unknown-unknown. If I am not mistaken without this config getrandom will be compiled unconditionally even if it was not used in the crate.
| let mut buf = [0u8; 16]; | ||
| // Use a constant seed on wasm32-unknown-unknown. | ||
| // `cfg(target = "..")` does not work right now, see: | ||
| // https://github.com/rust-lang/rust/issues/63217 |
There was a problem hiding this comment.
The second part of this comment can be removed, it's fine to just say that on wasm32-unknown-unknown we skip this.
|
I think the
|
Ah, I will try to prepare a PR then, either way it would've been needed for future removal of WASI Core API from
Do you have an approximate estimate regarding how long this decision would take?
As we have said earlier, we don't have any objections against transferring the repository to the rust-lang org. So I guess after a positive decision it can be done right away. |
| target_os = "unknown", | ||
| ))) { | ||
| getrandom::getrandom(&mut buf).unwrap(); | ||
| } |
There was a problem hiding this comment.
On wasm32-unknown-unknown, this will effectively be:
if false {
getrandom::getrandom(&mut buf).unwrap();
}... so it does require the getrandom dependency, even though this is dead code.
If you #[cfg(...)] gate this expression instead, it won't always need getrandom.
There was a problem hiding this comment.
But IIUC Cargo determines crates to be compiled only by looking at Cargo.toml, without taking crate code into account. So even if a dependency is not used, it will be still compiled, which in our case means a compilation error.
I forgot that we can use cfg like this:
#[cfg(..)]
getrandom::getrandom(&mut buf).unwrap();I will replace those lines in the next commit.
I also have no objection to this. Given however how active the https://github.com/rust-random/getrandom repository has been lately, perhaps it's sensible to wait until changes have slowed down somewhat? I don't believe we're likely to see too many more changes now, but there are still a couple of open PRs (e.g. |
Either way's fine, I don't have a preference how it's cascaded it just needs to be somehow :). It's probably easiest if
Currently the libs team meets every other week, but next week is Rustconf so we're unlikely to meet. The next meeting is on September 4. This PR already has an extremely long discussion history and is difficult to follow, so it would be best if there could be a write-up of what's happening here, why, etc, that the libs team could review. It would probably be best to place that somewhere separate like an issue writeup so we can FCP merge it to not block on a synchronous meeting.
Yes sorry I don't mean to invalidate what was said earlier, just that the libs team needs to make a decision one way or another and that hasn't happened yet. |
|
☔ The latest upstream changes (presumably #63640) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -1050,11 +1050,14 @@ dependencies = [ | |||
|
|
|||
| [[package]] | |||
| name = "getrandom" | |||
| version = "0.1.8" | |||
| version = "0.1.9" | |||
There was a problem hiding this comment.
We should update Cargo.lock here, as 0.1.9 has been yanked.
There was a problem hiding this comment.
It can wait until landing of VxWorks support.
alexcrichton
added
S-waiting-on-team
Mark-Simulacrum
removed
the
S-blocked
|
There doesn't appear to be any movement here in the past ~2 months so I'm going to close this and we can reopen when/if we're ready to move forward. It looks like the immediate action here is to open an issue with a concrete description of what all has happened on this PR (rationale, decisions, etc), which can then be moved into FCP by libs team after some discussion. |
Mark-Simulacrum
added
S-blocked-closed
and removed
S-waiting-on-team
|
FYI: I have created #80149 instead of updating this PR. |
jyn514
added
S-blocked
Closes: #62079