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

Upgrade criterion #1329

Merged
merged 4 commits into from
Apr 29, 2024
Merged

Upgrade criterion #1329

merged 4 commits into from
Apr 29, 2024

Conversation

vks
Copy link
Collaborator

@vks vks commented Jul 28, 2023

No description provided.

@vks
Copy link
Collaborator Author

vks commented Aug 20, 2023

The MSRV issues still need to be investigated.

@dhardy
Copy link
Member

dhardy commented Aug 21, 2023

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:

  • Limit testing on the MSRV to avoid using dev-dependencies. Probably our best option?
  • Bump our own MSRV: 1.64 is 11 months old so not too fresh though fresher than what we'd usually use.
  • Try to convince Criterion or Clap to change something...

@dhardy dhardy mentioned this pull request Oct 31, 2023
23 tasks
@dhardy
Copy link
Member

dhardy commented Dec 30, 2023

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
Copy link
Member

dhardy commented Dec 30, 2023

... at least until Cargo is MSRV aware.

@vks
Copy link
Collaborator Author

vks commented Dec 30, 2023

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
Copy link
Member

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
Copy link
Member

dhardy commented Feb 13, 2024

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
Copy link
Member

dhardy commented Mar 21, 2024

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
Copy link
Collaborator Author

vks commented Mar 22, 2024

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
Copy link
Member

dhardy commented Mar 22, 2024

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
Copy link
Member

dhardy commented Mar 22, 2024

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
Copy link
Member

dhardy commented Mar 24, 2024

@vks can you rebase this now?

@dhardy dhardy added the D-changes Do: changes requested label Apr 10, 2024
@vks vks force-pushed the upgrade-criterion branch from 71f5a2e to d507f7e Compare April 26, 2024 11:46
@vks
Copy link
Collaborator Author

vks commented Apr 26, 2024

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

@vks
Copy link
Collaborator Author

vks commented Apr 26, 2024

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.

@vks vks requested a review from dhardy April 26, 2024 12:26
@dhardy dhardy mentioned this pull request Apr 27, 2024
1 task
@vks vks merged commit 0935356 into rust-random:master Apr 29, 2024
12 checks passed
@vks vks deleted the upgrade-criterion branch July 31, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-changes Do: changes requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants