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

Add MSRV 1.60.0 #181

Merged
merged 5 commits into from
Nov 12, 2023
Merged

Add MSRV 1.60.0 #181

merged 5 commits into from
Nov 12, 2023

Conversation

tkaitchuck
Copy link
Owner

@tkaitchuck tkaitchuck commented Oct 31, 2023

Adds a MSRV and an github actions test to prevent regression.
Fixes #179

Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Signed-off-by: Tom Kaitchuck <tom.kaitchuck@emc.com>
Copy link
Contributor

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Hope the feedback helps 😁

Here's the TL;DR: summary:

  • Your CI change with -Z msrv-policy probably isn't useful if you're not testing the Cargo.lock on the rust-version toolchain declared in Cargo.toml.
  • Not sure why you're raising the minimum versions in Cargo.toml for deps, usually you don't need to do that unless there's good reason to.
  • rust-version = "1.60.0" in Cargo.toml is good 👍 It's usefulness is more to do with when you need to raise it for ahash in a future release.
    • CI may be able to leverage it with -Z msrv-policy, but having the toolchain resolve a fresh Cargo.lock also communicates when that nightly feature resolves deps differently than the intended MSRV toolchain would. Until the feature is stabilized at least. Both approaches can fail, just differently.

.github/workflows/rust.yml Show resolved Hide resolved
serde = { version = "1.0.117", optional = true }
cfg-if = "1.0"
atomic-polyfill = { version="1.0.1", optional=true}
getrandom = { version = "0.2.7", optional = true }
zerocopy = { version = "0.7.14", default-features = false, features = ["simd"] }
zerocopy = { version = "0.7.20", default-features = false, features = ["simd"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Raising minimum version in semver is not necessary unless your crate fails to build / support lower versions.

There is -Z direct-minimal-versions which (unlike -Z minimal-versions) will resolve only the lowest bound of your semver range for your dependencies (-Z minimal-versions does this for every crate in Cargo.lock which is more likely to fail). Using this would then verify a build compatibility at your lower bound, while the implicit dependencies would resolve their semver as per usual.

Raising the minimum version adds a constraint on the semver if other crates have an overlapping range, narrowing what is accepted AFAIK.

If your declared rust-version builds / resolves correctly, then there is no need to raise the minimum? It does not benefit your crate when published AFAIK, semver will be resolved when ahash is pulled in and typically use the latest release for the dependency unless Cargo.lock for example manipulates that like with the mentioned -Z nightly options.

@@ -12,6 +12,8 @@ edition = "2018"
readme = "README.md"
build = "./build.rs"
exclude = ["/smhasher", "/benchmark_tools"]
rust-version = "1.60.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good 👍

Cargo.toml Show resolved Hide resolved
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
tkaitchuck and others added 2 commits November 12, 2023 14:25
Signed-off-by: Tom Kaitchuck <Tom.Kaitchuck@gmail.com>
@tkaitchuck tkaitchuck merged commit 9f6a2ad into master Nov 12, 2023
20 checks passed
@tkaitchuck tkaitchuck deleted the msrv branch November 12, 2023 22:49
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.

v0.8.4 increased MSRV to 1.61
2 participants