rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.67k stars 431 forks source link

Upgrade criterion #1329

Closed vks closed 7 months ago

vks commented 1 year ago

The MSRV issues still need to be investigated.

dhardy commented 1 year ago

Ah yes: criterion depends on clap v4, the latest release of which depends on Rust 1.64 (2022-09-22) whereas our MSRV is 1.56 (2021-10-21).

Bumping an MSRV in a patch release is a common enough policy. I guess our options are:

dhardy commented 11 months ago

Attempting to reproduce now (rebased on master):

error: package `clap_lex v0.6.0` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.60.0

1.70.0 is now about six months old. While I wouldn't discount the option of pushing the MSRV to this for the next release of Rand, I'm not happy about usage of a dependency with a history of updating its MSRV to somewhat recent releases frequently.

We could use a pinned version of Cargo.lock when testing the MSRV, which would avoid the issue of breaking CI. However, in that case we should also test that the library at least builds with the MSRV version using latest dependencies, IMO.

Debian stable currently uses rustc 1.63.0, so we should probably support that.

@newpavlov @vks should we do this? (Full test on MSRV with pinned Cargo.lock and test builds on MSRV with latest dependencies.)

dhardy commented 11 months ago

... at least until Cargo is MSRV aware.

vks commented 11 months ago

I would prefer to exclude benchmarks (and possibly dev dependencies) from our MSRV guarantees.

I think we could only rely on MSRV-aware Cargo once our MSRV supports it, so this would be quite far in the future.

newpavlov commented 11 months ago

I agree with @vks. I also think that it could be worth to separate benchmarks, so they would not be built as part of tests. Especially considering that criterion is a relatively heavy dependency.

dhardy commented 9 months ago

We could use a pinned version of Cargo.lock

I forgot that we already do: #1275. So maybe all we need to do is update that (if needed) and use a separate CI test for MSRV which only builds?

dhardy commented 8 months ago

I've been trying to produce a Cargo.lock which builds with 1.60.0, but I keep running into the following:

error: failed to run custom build command for `proc-macro2 v1.0.63`

Caused by:
  process didn't exit successfully: `/home/dhardy/projects/rand/rand/target/debug/build/proc-macro2-f02e3034c0c8cca8/build-script-build` (signal: 6, SIGABRT: process abort signal)
  --- stderr
  fatal runtime error: assertion failed: thread_info.is_none()

(with various versions of proc-macro2, or indeed other crates).

The error isn't particularly helpful. Google finds a couple of other cases, but no real answers.

Other than this, -Z minimal-versions gets most of the way to something which may work. I also have a Cargo.lock which is (mostly) maximal versions of crates that don't appear to require a more recent rustc, but hit the same error.

At this point, I'm wondering if we simply can't support rustc 1.60.0?

vks commented 8 months ago

What about excluding our benchmarks from MSRV guarantees? It would be a problem if we want to compare performance across Rust versions, but I don't think benchmarks are core functionality that must work across all versions.

dhardy commented 8 months ago

What about excluding our benchmarks

I can't even get a build to work now:

$ cargo +1.60 build
   Compiling libc v0.2.153
   Compiling zerocopy v0.8.0-alpha.6
error: failed to run custom build command for `libc v0.2.153`

Caused by:
  process didn't exit successfully: `/home/dhardy/projects/rand/rand/target/debug/build/libc-0183ba5fb73466b7/build-script-build` (signal: 6, SIGABRT: process abort signal)
  --- stderr
  fatal runtime error: assertion failed: thread_info.is_none()
warning: build failed, waiting for other jobs to finish...
error: build failed

Possibly this zerocopy ver is not compatible with rust 1.60.0.

dhardy commented 8 months ago

I have a working build using rustc 1.61.0, but it fails on 1.60 as above. I'll post a new PR.

dhardy commented 8 months ago

@vks can you rebase this now?

vks commented 7 months ago

I rebased it, but some criterion dependencies are still causing trouble.

vks commented 7 months ago

I moved the benchmarks to their own crate, which should also help with compilation times for dev builds. I also fixed MSRV 1.61 compatibility by pinning Rayon.