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

Update rand deps #86963

Closed
wants to merge 3 commits into from
Closed

Update rand deps #86963

wants to merge 3 commits into from

Conversation

klensy
Copy link
Contributor

@klensy klensy commented Jul 7, 2021

rand 0.7 -> 0.8
rand_xorshift 0.2 -> 0.3

This gives -6 crates for rustc compiler build

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2021
@klensy
Copy link
Contributor Author

klensy commented Jul 7, 2021

Should i add rand_core_0_6 here too?

rand_core_0_5 = { package = "rand_core", version = "0.5.1", features = ["getrandom", "alloc", "std"] }

There also rust-random/rand#1061:

ReadRng::next_u32 and next_u64 now use little-Endian conversion instead
of native-Endian, affecting results on Big-Endian platforms

@Mark-Simulacrum
Copy link
Member

Can you at least cite the changelogs for the updated crates? For example, the note about different results on big endian platforms may mean that we need to check our usage of those functions in the compiler, but it sounds like that investigation has not been done?

Generally speaking updates to dependencies of the compiler or standard library that aren't just used in tests need some scrutiny for potential breakage caused by their new implementations and it's quite helpful if you provide a summary of what checking has been done (e.g., for each compatibility note in the crates' changelog, or similar listing). Alternatively, if it seems like the bump has no compatibility implications, then explicitly indicating that in the PR/commit description and why would also be helpful.

@klensy
Copy link
Contributor Author

klensy commented Jul 11, 2021

Changes for rand 0.8 https://github.com/rust-random/rand/blob/master/CHANGELOG.md#080---2020-12-18 (this is for 0.8.0, description for up to 0.8.3 higher)

Changes for rand_xorshift 0.3 https://github.com/rust-random/rngs/blob/master/rand_xorshift/CHANGELOG.md#030---2020-12-18

Existing tests that was broken by dependency update was fixed.

There is only one place where rand used not for test (as far as i can see):

fn generate_session_dir_path(crate_dir: &Path) -> PathBuf {
let timestamp = timestamp_to_string(SystemTime::now());
debug!("generate_session_dir_path: timestamp = {}", timestamp);
let random_number = thread_rng().next_u32();
debug!("generate_session_dir_path: random_number = {}", random_number);
let directory_name = format!(
"s-{}-{}-working",
timestamp,
base_n::encode(random_number as u128, INT_ENCODE_BASE)
);
debug!("generate_session_dir_path: directory_name = {}", directory_name);
let directory_path = crate_dir.join(directory_name);
debug!("generate_session_dir_path: directory_path = {}", directory_path.display());
directory_path
}

and i don't see how this can broke anything, as we simply generate random number for folder name.

@klensy
Copy link
Contributor Author

klensy commented Jul 22, 2021

Should i add something?

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jul 22, 2021

📌 Commit cf571e393f6a956a32ca15755fe6aee1f2ddbb6a has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2021
@bors
Copy link
Contributor

bors commented Jul 23, 2021

⌛ Testing commit cf571e393f6a956a32ca15755fe6aee1f2ddbb6a with merge a3199280041baacbdc2714b3307ac95fe6507ac8...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 23, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 23, 2021
@klensy
Copy link
Contributor Author

klensy commented Jul 23, 2021

Ha-ha, actually breaks.
https://github.com/rust-random/getrandom/blob/master/CHANGELOG.md#020---2020-09-10

Unsupported targets no longer compile

https://docs.rs/getrandom/0.2.3/getrandom/#webassembly-support

Instead, if the "js" Cargo feature is enabled, this crate will assume that you are building for an environment containing JavaScript, and will call the appropriate methods.

@Mark-Simulacrum
Copy link
Member

I don't have the bandwidth to investigate this failure and suggest a course of action for the wasm32 target; if you have a specific proposal, happy to review that though.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2021
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2021
@bors
Copy link
Contributor

bors commented Aug 20, 2021

⌛ Testing commit cf571e393f6a956a32ca15755fe6aee1f2ddbb6a with merge 074c72f8394e0632a1b263791642362f60f37125...

@klensy
Copy link
Contributor Author

klensy commented Aug 20, 2021

I didn't changed anything yet, so it will fail for wasm again.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 20, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 20, 2021
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2021
@nikic
Copy link
Contributor

nikic commented Aug 27, 2021

@bors r- For some reason this is in the queue again.

@JohnCSimon
Copy link
Member

Ping for triage:

@klensy can you please post your status on this PR?

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 5, 2021

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

@apiraino apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 14, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@klensy
Please adjust the label with @rustbot ready if this PR is ready for another review.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2022
@JohnCSimon
Copy link
Member

@klensy
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Feb 27, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 27, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2023
…, r=Mark-Simulacrum

Update `rand` in the stdlib tests, and remove the `getrandom` feature from it.

The main goal is actually removing `getrandom`, so that eventually we can allow running the stdlib test suite on tier3 targets which don't have `getrandom` support. Currently those targets can only run the subset of stdlib tests that exist in uitests, and (generally speaking), we prefer not to test libstd functionality in uitests, which came up recently in rust-lang#104095 and rust-lang#104185. Additionally, the fact that we can't update `rand`/`getrandom` means we're stuck with the old set of tier3 targets, so can't test new ones.

~~Anyway, I haven't checked that this actually does allow use on tier3 targets (I think it does not, as some work is needed in stdlib submodules) but it moves us slightly closer to this, and seems to allow at least finally updating our `rand` dep, which definitely improves the status quo.~~ Checked and works now.

For the most part, our tests and benchmarks are fine using hard-coded seeds. A couple tests seem to fail with this (stuff manipulating the environment expecting no collisions, for example), or become pointless (all inputs to a function become equivalent). In these cases I've done a (gross) dance (ab)using `RandomState` and `Location::caller()` for some extra "entropy".

Trying to share that code seems *way* more painful than it's worth given that the duplication is a 7-line function, even if the lines are quite gross. (Keeping in mind that sharing it would require adding `rand` as a non-dev dep to std, and exposing a type from it publicly, all of which sounds truly awful, even if done behind a perma-unstable feature).

See also some previous attempts:
- rust-lang#86963 (in particular rust-lang#86963 (comment) which explains why this is non-trivial)
- rust-lang#89131
- rust-lang#96626 (comment) (I tried in that PR at the same time, but settled for just removing the usage of `thread_rng()` from the benchmarks, since that was the main goal).
- rust-lang#104185
- Probably more. It's very tempting of a thing to "just update".

r? `@Mark-Simulacrum`
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request May 31, 2023
…Simulacrum

Update `rand` in the stdlib tests, and remove the `getrandom` feature from it.

The main goal is actually removing `getrandom`, so that eventually we can allow running the stdlib test suite on tier3 targets which don't have `getrandom` support. Currently those targets can only run the subset of stdlib tests that exist in uitests, and (generally speaking), we prefer not to test libstd functionality in uitests, which came up recently in rust-lang/rust#104095 and rust-lang/rust#104185. Additionally, the fact that we can't update `rand`/`getrandom` means we're stuck with the old set of tier3 targets, so can't test new ones.

~~Anyway, I haven't checked that this actually does allow use on tier3 targets (I think it does not, as some work is needed in stdlib submodules) but it moves us slightly closer to this, and seems to allow at least finally updating our `rand` dep, which definitely improves the status quo.~~ Checked and works now.

For the most part, our tests and benchmarks are fine using hard-coded seeds. A couple tests seem to fail with this (stuff manipulating the environment expecting no collisions, for example), or become pointless (all inputs to a function become equivalent). In these cases I've done a (gross) dance (ab)using `RandomState` and `Location::caller()` for some extra "entropy".

Trying to share that code seems *way* more painful than it's worth given that the duplication is a 7-line function, even if the lines are quite gross. (Keeping in mind that sharing it would require adding `rand` as a non-dev dep to std, and exposing a type from it publicly, all of which sounds truly awful, even if done behind a perma-unstable feature).

See also some previous attempts:
- rust-lang/rust#86963 (in particular rust-lang/rust#86963 (comment) which explains why this is non-trivial)
- rust-lang/rust#89131
- rust-lang/rust#96626 (comment) (I tried in that PR at the same time, but settled for just removing the usage of `thread_rng()` from the benchmarks, since that was the main goal).
- rust-lang/rust#104185
- Probably more. It's very tempting of a thing to "just update".

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants