Skip to content

Update Criterion, MSRV#51

Merged
dhardy merged 18 commits intorust-random:masterfrom
dhardy:rust-toolchain
May 24, 2024
Merged

Update Criterion, MSRV#51
dhardy merged 18 commits intorust-random:masterfrom
dhardy:rust-toolchain

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 newpavlov and vks 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.

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.

@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
takumi-earth pushed a commit to earthlings-dev/rngs that referenced this pull request Jan 27, 2026
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