Uh oh!
There was an error while loading. Please reload this page.
Use getrandom crate - #62082
Conversation
rust-highfive
commented
Jun 23, 2019
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
commented
Jun 23, 2019
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 |
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Joe ST <joe@fbstj.net>
rust-highfive
commented
Jun 23, 2019
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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rust-highfive
commented
Jun 24, 2019
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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
newpavlov
commented
Jun 24, 2019
BTW can you help with updating |
rust-highfive
commented
Jun 24, 2019
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 |
Centril
commented
Jun 25, 2019
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. |
newpavlov
commented
Jun 25, 2019
Do you mean moved to rust-lang organization? I don't see why it should be specifically part of rust-lang/rust. @dhardy |
dhardy
commented
Jun 25, 2019
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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| 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 |
dhardy
commented
Jun 25, 2019
Interesting. Then we should also CC @ebarnard and consider whether we want to adjust |
rust-highfive
commented
Jun 25, 2019
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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Oliver Middleton <olliemail27@gmail.com>
rust-highfive
commented
Aug 14, 2019
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 |
newpavlov
commented
Aug 14, 2019
It's alive! @sfackler |
| 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.
alexcrichton
commented
Aug 14, 2019
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:
iffalse{
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.
dhardy
commented
Aug 15, 2019
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. |
alexcrichton
commented
Aug 15, 2019
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. |
bors
commented
Aug 16, 2019
☔ The latest upstream changes (presumably #63640) made this pull request unmergeable. Please resolve the merge conflicts. |
| [[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.
Mark-Simulacrum
commented
Oct 24, 2019
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. |
newpavlov
commented
Dec 19, 2020
FYI: I have created #80149 instead of updating this PR. |
Closes: #62079