Update to rand 0.9.0

[ChrisDenton]

Changes include:

• thread_rng has been renamed to rng
• Standard has been renamed to StandardUniform
• gen, gen_range, gen_bool have been renamed to random, random_range and random_bool respectively.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[ChrisDenton]

Oh getrandom updated the wasi dependency and that now includes a dependency on wit-bindgen-rt. That seems fine? I'll add it to the allowed deps but feel free to correct me!

[rustbot]

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

[bors]

☔ The latest upstream changes (presumably #136454) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

Hm, so I don't have any hard objections to wit-bindgen-rt, but it's a pretty weird crate... it has no README on crates.io, and looking at the raw artifacts there seems to be C and some non-source files (wit-bindgen-rt-0.39.0/src/cabi_realloc.o, wit-bindgen-rt-0.39.0/src/libwit_bindgen_cabi_realloc.a) in it? That seems to get linked in on wasm32 targets, which feels somewhat surprising to me -- I wouldn't have expected binary artifacts in crates.io.

@alexcrichton could we perhaps add a README to the crate explaining why it has compiled artifacts? I suspect that's just going to reference the comment in https://github.com/bytecodealliance/wit-bindgen/blob/main/ci/rebuild-libcabi-realloc.sh, but since that file isn't shipped with the crate I had to do some hunting to find it :) It's my understanding that at least some upstream consumers of Rust don't want compiled artifacts in the "source" which this would be semi-introducing (it looks like miri already depends on this crate), I'm not sure how much of a problem that is in practice given what this is doing but it feels at least somewhat iffy.

@wesleywiser / @davidtwco as T-compiler leads probably worth taking a look. Should we have some policy around this sort of thing? Maybe there's some other way we can help achieve the goal of ~weak symbols this crate wants on stable?

$ tar tf wit-bindgen-rt-0.39.0.tar.gz
wit-bindgen-rt-0.39.0/.cargo_vcs_info.json
wit-bindgen-rt-0.39.0/Cargo.lock
wit-bindgen-rt-0.39.0/Cargo.toml
wit-bindgen-rt-0.39.0/Cargo.toml.orig
wit-bindgen-rt-0.39.0/build.rs
wit-bindgen-rt-0.39.0/src/async_support/future_support.rs
wit-bindgen-rt-0.39.0/src/async_support/stream_support.rs
wit-bindgen-rt-0.39.0/src/async_support.rs
wit-bindgen-rt-0.39.0/src/cabi_realloc.c
wit-bindgen-rt-0.39.0/src/cabi_realloc.o
wit-bindgen-rt-0.39.0/src/cabi_realloc.rs
wit-bindgen-rt-0.39.0/src/lib.rs
wit-bindgen-rt-0.39.0/src/libwit_bindgen_cabi_realloc.a

[Mark-Simulacrum]

As one option I wonder if the pre-compiled sources could be replaced with a dependency on (for example) wasm-encoder and just producing them from the build script? That's likely to be slower to build but it would make confirming that the file is what it says it is to some extent easier.

[alexcrichton]

Oh dear apologies if this is causing issues! As an internal implementation detail of wit-bindgen we didn't pay too much attention to public-facing documentation but I'm happy to add a README and/or crate docs pointing to the build script at the bare minimum.

It's my understanding that at least some upstream consumers of Rust don't want compiled artifacts in the "source"

If it helps at all we have documented/automated build instructions for this artifact in CI and it's always ensured that the checked-in copy is always up-to-date. This is intended to show that it's at least reproducible and matches the sources in tree. I realize though that such a setup may still not be sufficient for some consumers.

Maybe there's some other way we can help achieve the goal of ~weak symbols this crate wants on stable?

I can confirm that the feature we're lacking in Rust to avoid this artifact is indeed weak symbols. Specifically we want the ability to have a symbol, cabi_realloc, defined in multiple crates across a single link unit. The semantics of that function are defined outside of Rust so it doesn't matter who provides it, we just need to be sure that someone does. On the wasm32-wasip2 target the Rust standard library provides this symbol (as weak symbols can be used), so the purpose of wit-bindgen-rt is to provide the symbol for the wasm32-wasip1 target.

As one option I wonder if the pre-compiled sources could be replaced with a dependency on (for example) wasm-encoder and just producing them from the build script?

This is indeed possible! That's more-or-less what this build script does for an unrelated part of wasm. We manually assemble a libfoo.a with a specially-cratfted object file. The problem is that it's basically a write-once script that can't really be understood after-the-fact. If this is blocking an update for rand I can try to set aside time to do this, but I'd personally prefer to not go so far as it drastically reduces the maintainability of the crate. Basically I'd prefer to push on other avenues if possible, but if this is the only avenue then such is life sometimes!

[Mark-Simulacrum]

Yeah, I agree that manually writing out the bytes is only sort of an improvement. It does feel plausible that in e.g. a reproducible builds or company import policy it would matter. One thought I did have is that you could plausibly produce the object "just in time" with a RUSTC_BOOTSTRAP rustc toolchain, but that doesn't seem like a great idea. Or maybe having some tool (a-la patchelf or a linker script or something) that edits the symbol a stable Rust compiler can produce.

I don't personally have a strong objection given the specific context here (and the lack of meaningful code in the blob) but it does seem like somewhat of a slippery slope.

In this particular context I suspect this dependency is never(?) actually used since rustc or miri probably never get built for wasm (at least not by us). So maybe we just say that makes this mostly a non-issue.

I'll nominate this for T-compiler to make a final call, and whatever we end up deciding we should probably make a policy note somewhere.

[Mark-Simulacrum]

I do think a README would be good to add. I'll also note the published artifact doesn't have a LICENSE file (again, not sure it matters, and is probably not uncommon, but perhaps not strictly ideal).

[alexcrichton]

I've added a README at bytecodealliance/wit-bindgen#1162 and for LICENSE stuff we generally rely on the license field of bytecodealliance crates to avoid copying around LICENSE files everywhere.

[bors]

☔ The latest upstream changes (presumably #136845) made this pull request unmergeable. Please resolve the merge conflicts.

[ChrisDenton]

Would it be possible to have a feature or even a global config so that the compiler can opt-in to using the nightly feature for weak symbols instead of the precompiled artefacts?

Generally the compiler is fine with using nightly features that have no stability guarantees but I can understand not wanting to expose your users to that.

e.g. for the windows crate we have the windows-raw-dylib cfg for something a bit similar.

[alexcrichton]

I'd be happy to do that yeah, but would want to confirm that would help here? That wouldn't actually remove the binary artifacts, it'd only configure things to not actually use them at compile-time. Is that sufficient for users who would push back against the inclusion of binary artifacts? (it's sort of just a pinky-promise to not use them as opposed to a guarantee)

[bors]

☔ The latest upstream changes (presumably #137164) made this pull request unmergeable. Please resolve the merge conflicts.

[ChrisDenton]

I'll wait for @wesleywiser to comment on that. I can only say that what windows does is to have the binaries in separate crates that aren't include when the --cfg is set and that seems to have been accepted.

[apiraino]

Sorry for the late feedback, we discussed this PR last week in the triage meeting (on Zulip).

@alexcrichton we basically wanted to know more about (and how feasable) removing the binary blob completely (that you mentioned here). Maybe Wesley can add more.

Also mentioned that since we clearly know what this blob is (comment), maybe we can also evaluate a byte-per-byte tidy check?

@rustbot label -I-compiler-nominated

[alexcrichton]

Completely removing this blob is unfortunately probably going to take a bit of time. I'm kind of short on time right now and don't have the cycles to invest in rewriting the construction process for this blob in a build script instead of having it checked in. It's certainly feasible to do, but it's quite nontrivial and requires shuffling things around (plus picking up nontrivial build-time dependencies which has further impacts, etc).

To answer some questions/thoughts that came up in that discussion though:

• The Rust feature that removing this blob depends on is weak symbols. With weak symbols it would be a pretty trivial refactoring to remove this blob. I realize though that stabilizing weak symbols is no small task though, so blocking this PR on that seems not great.
• Hashing this blob and requiring the exact bytes may work ok, but the bytes of this blob will change version-to-version. The source for this blob uses $sym which has the version number of the crate in there, meaning that the blob will change (due to symbol references changing)
• Removal of this blob is possible, albeit a fair chunk of work. It would require adapting this build script to the needs of this blob. That's a lot of new build-time dependencies (wasm-encoder, ar, etc) where there are currently none.
• While weak symbols are on nightly Rust and this crate is indeed compiled in rustc where nightly features can be used the crate is also more generally intended to be used on stable in the rest of the ecosystem. While it's possible to add a crate feature to use nightly features it would consequently not remove the blob, only conditionally avoid using it at build time. I'm led to believe that wouldn't satisfy the "no binary blob" desire.

Overall I feel like this is relatively side-tracked from the actual thrust of this PR, updating the rand crate. I personally want to get out of the way of the critical path here if I can as I hate to block this on a concern for a relatively niche crate. The most expedient solution, if acceptable, is to probably say "this single binary blob with this hash is acceptable" with the understanding that the precise hash is going to change here over time as the crate updates.

I also realize that the preferred compiler solution is to remove the blob entirely, and I agree with that! I mostly want to clarify that doing so is going to take a nontrivial chunk of work of my time (or someone else's) and has consequences on the crate's usage in the ecosystem which aren't free.

Also if desired I'm happy to chat on Zulip in a meeting about this or similar, or move this discussion to a separate issue or similar.

[riking]

The rand crate has been updated in the library/ folder in #136983, which uses default-features=false everywhere to not bring in these concerns.

[Mark-Simulacrum]

The most expedient solution, if acceptable, is to probably say "this single binary blob with this hash is acceptable" with the understanding that the precise hash is going to change here over time as the crate updates.

Here's a counter proposal -- I think arguably simpler -- we just pin the version of this crate in Cargo.toml (= dependency) somewhere, ensuring that we only intentionally bump it. At that point this binary blob is not really any different to our "binary blob" dependency on providing a rustc, gcc, etc. -- it can be built from source but usually isn't. That also mitigates risk since if/when we do actually bump this dependency we can check that the new blob is still small, etc.

I'd personally be happy to r+ this with that change (explicit pinning) made.

[ChrisDenton]

we just pin the version of this crate in Cargo.toml (= dependency) somewhere, ensuring that we only intentionally bump it.

I couldn't figure out how to do this robustly. Cargo is designed to support multiple versions of the same dependency. We can lock a direct dependency but if a dependency of a dependency bumps the major wit-bindgen-rt version then I don't think we can protect against that using Cargo.toml alone. We'll just end up with multiple versions.

My counter-counter proposal that I just pushed is to support PERMITTED_RUSTC_DEPENDENCIES having an optional version (using Cargo's @ syntax). I think this might work better?

[bors]

☔ The latest upstream changes (presumably #137927) made this pull request unmergeable. Please resolve the merge conflicts.