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 Criterion, MSRV #51

Merged
merged 18 commits into from
May 24, 2024
Merged

Update Criterion, MSRV #51

merged 18 commits into from
May 24, 2024

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented May 24, 2024

This replaces #50.

All crates are now using MSRV = 1.61, the same as rand. If we're going to maintain these crates, we should at least do so using the same Rust version we use elsewhere.

To do later (in new PRs):

  • rustfmt, add Clippy/rustfmt CI checks
  • Depend on rand version from master until next release; move rand_pcg here.

@dhardy dhardy requested review from vks and newpavlov May 24, 2024 08:47
Cargo.toml Outdated
[workspace]
members = [
"benches",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to exclude benches from the root workspace and test its build in a separate CI job.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is part of the workspace.

Copy link
Member

@newpavlov newpavlov May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's our decision whether to include a crate into workspace or not. In rand we exclude benches because criterion is a relatively heavy dependency and it's somewhat wasteful to build it during crate tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you managed to sneak that change into rust-random/rand#1448

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And your CI script tests the base repo, not the benches package. Check the log:
https://github.com/rust-random/rand/actions/runs/9008465527/job/24750605137

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Clippy complaining was probably because working dir was not changed in the previous version of the config.

I fixed that; it alone wasn't enough. And try e.g. cargo test -p rand_hc from the benches dir; it should not work but it does!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we also need to add package.workspace = "." to benches' Cargo.toml?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result:

$ cargo test
error: root of a workspace inferred but wasn't a root: /home/dhardy/projects/rand/rngs/benches/Cargo.toml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just a Cargo bug we have to deal with... lets merge this?

Copy link
Member

@newpavlov newpavlov May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. I meant:

[workspace]
members = ["."]

UPD: Hm, nope. cargo test -p rand_core still works from the benches folder.

rand_jitter/benches/jitter.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@dhardy dhardy force-pushed the rust-toolchain branch 2 times, most recently from e51461b to de7898e Compare May 24, 2024 13:41
rust-version = "1.61"
publish = false

[dependencies]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty dependencies section can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #52

@dhardy dhardy merged commit 165af79 into rust-random:master May 24, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants