-
Notifications
You must be signed in to change notification settings - Fork 84
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
Implement and document MSRV #268
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #268 +/- ##
=======================================
Coverage 93.54% 93.54%
=======================================
Files 53 53
Lines 11662 11662
=======================================
Hits 10909 10909
Misses 753 753 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think cargo hack could be valuable here to not require consistency between a value in CI and the manifest. But I'd be open to other options to do the same deduplication.
Not as familiar as I'd like to be with GitHub's review tools, so let me know if something is unclear. |
f33ac97
to
3356c95
Compare
I agree that cargo hack seems useful for this purpose. Added it and rebased on top of current master. Also reworded the README similar to how you suggested, hope it's OK for you.
No worries, everything was clear |
53efed1
to
145d577
Compare
145d577
to
35e068d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, merging.
Looks like CI is stuck due to rust-lang/rust#130056 Not sure there's much to do about it at the moment. It's just nightly being affected |
Closes #242.
I figured out the current MSRV via manual testing.
first_key_value()
(found in empirical.rs) was stabilized in 1.66.0, which was released in December 2022.It's probably not very difficult to lower the MSRV a bit, but it wasn't a priority for this PR.
I added a lockfile containing MSRV-compatible dependency versions to the project. A change in MSRV is not considered to be breaking by most people AFAICT, so the msrv CI job uses the lockfile to ensure nothing breaks over time.1
Note that this file has no effect on compilation unless you manually
mv Cargo.lock.MSRV Cargo.lock
.This new lockfile however means that changes to the
[dependencies]
in Cargo.toml require updating theCargo.lock.MSRV
(CI is run with--locked
, so a mismatch betweenCargo.toml
andCargo.lock
will result in a compile-time error) if you want CI not to fail. It's relatively easy to do if you have access to a nightly toolchain:cargo +nightly update -Zmsrv-policy && cp Cargo.lock Cargo.lock.MSRV
.If you don't, another possibility is just updating all dependencies (
cargo update
) and then manually downgrading packages that are incompatible (cargo update --precise <msrv-compatible version> <broken package>
). That is pretty tedious though.Not 100% sure about the note in the README. It's the usual way I've seen it described and done so for other projects myself. Since statrs is pre-1.0, a MINOR version increase means a breaking change, so MSRV should be considered a PATCH version increase. Unless of course it's bundled with other changes, which often makes sense.
Footnotes
For example:
approx = 0.5.0
is resolved to version 0.5.1 now and compatible with 1.66.0. If version 0.5.2 is released and has a MSRV of 1.70, thencargo update
or a freshcargo build
(like CI) will resolve to the newer version by default, meaning compilation will break on 1.66.0. Locking approx to version 0.5.1 prevents this from happening. ↩