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

Set the rust-version, update edition. #46

Merged
merged 3 commits into from
Oct 19, 2024

Conversation

dcampbell24
Copy link
Contributor

You might as well set the MSRV so you know what the minimum supported rust version is. You may as well also icrement the edition because it requires no changes and the MSRV is supported by the edition increase.

I calculated the MSRV with cargo msrv.

You might as well set the MSRV so you know what the minimum
supported rust version is. You may as well also icrement the
edition because it requires no changes and the MSRV is supported
by the edition increase.

I calculated the MSRV with cargo msrv.
@pfernie
Copy link
Owner

pfernie commented Oct 10, 2024

I agree with the sentiment here; edition is straightforward. I have some questions/thoughts on MSRV, however.

  1. reqwest (the primary use case this crate is used alongside) specifies an MSRV of 1.63.0
  2. Using cargo-msrv, I end up w/ a possible MSRV of 1.60.0. This may be due to the version of cargo-msrv; I used the latest release 0.16.2. I had first tried with an older version (0.15.1), it yielded a different answer.
  3. If I enable all features (cargo msrv find --all-features), the MSRV becomes 1.64.0.

Reading around, I haven't found a definitive source on best practices w.r.t. MSRV; in particular in this case, the --all-features:

  1. Enables preserve_order, which brings in hashbrown transitively via indexmap, and this has MSRV 1.64.0. But, it seems we can bump indexmap to 2.6.0, which actually ends up resolving the hashbrown MSRV issue (they downgraded MSRVs).
  2. But, the new serde_ron feature brings in ron, which does unfortunately have a 1.64.0 MSRV.

With the indexmap change, I am inclined to set the MSRV to 1.60.0 (minimum w/o --all-features), in order to keep the MSRV w/i that of reqwest. serde_ron is a new feature, so reqwest does not utilize it (and I would not see it doing so in the future). And since MSRV is transitive, it seems reasonable to specify MSRV as the "out-of-the-box" MSRV (w/o --all-features); enabling optional features such as serde_ron would expose the MSRV of transitive dependencies like ron, effectively bumping cookie_stores MSRV when built this way(?) But, maybe I am thinking of MSRV wrong, philosophically?

Also, I see that you modified this PR to move the MSRV to 1.67.0, so I am a bit curious what version of cargo-msrv you are using, or why you might be coming up with that?

@dcampbell24
Copy link
Contributor Author

I was using an older version of cargo-msrv, but I updated it and tried running it with --all-featutes, and either way I got 1.67.1. After taking another look at the documentation 1.67 is the same as 1.67.0, so I should actually include the patch version, since it is 1.67.1. Maybe my OS/target makes the difference? I get:

Result:
   Considered (min … max):   Rust 1.31.1 … Rust 1.81.0
   Search method:            bisect
   MSRV:                     1.67.1
   Target:                   x86_64-unknown-linux-gnu

when using the latest version.

https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field
https://github.com/rust-lang/rust-clippy?tab=readme-ov-file#specifying-the-minimum-supported-rust-version

@pfernie
Copy link
Owner

pfernie commented Oct 11, 2024

Ah, I had not done a cargo update in my local checkout recently. A prior (semver compatible w/ the cookie_store Cargo.toml specified 0.3.16) version of time had a lower MSRV, but the current 0.3.36 has an MSRV of 1.67.0. Doing a cargo update --precise 0.3.20 time (0.3.20 being the newest with a suitable MSRV) and then checking the MSRV will yield 1.64.0 w/ --all-features and 1.63.0 without.

Which goes back to my prior thoughts: I don't think there is a consensus if bumping MSRV should be a semver compatibility break. From cookie_stores perspective, there is a set of semver-compatible packages that will compile on 1.63.0 (or 1.64.0 w/ --all-features). But a downstream dep, such as time here, could up their MSRV w/ a semver compatible release, which would be picked up by consumer of cookie_store unless the dependency specification were made more restrictive (which I don't think is desirable...), "breaking" our advertised MSRV (naively, at least).

Which is all to say I am fine adding MSRV here, but I'm not sure what the best practice should be. I would like to make it "truly minimal", or at least keep it to reqwest, in that there is a set of semver compatible dependency versions (& all cookie_store code) that will compile under the lower 1.63.0.

I've opened a urlo topic to solicit some community feedback on the general question.

Per discussion on [this urlo
thread](https://users.rust-lang.org/t/best-community-practices-for-msrv/119566),
there is no firm rule on utilization of MSRV. Here, we will opt to
advertise a MSRV that is compatible w/ `reqwest`'s current MSRV, and
compatible with this crate's current dependencies. Note that the
dependency `time` must be `0.3.20` or earlier to satisfy this MSRV; as
MSRV increase is not considered a semver breaking change, we do not
restrict our specification of the `time` dependency to uphold any
guarantee here :(.
@pfernie pfernie merged commit 5063890 into pfernie:master Oct 19, 2024
1 check passed
@pfernie
Copy link
Owner

pfernie commented Oct 19, 2024

Thanks for the contribution here; in the end the discussion on urlo led me to believe there isn't a best practice here, really. I opted to target 1.63.0 as that is the advertised MSRV of reqwest, and MSRV-changes aren't semver breaking. So, even if we chose what cargo msrv reports today, a downstream dep could make a semver compatible update bumping their MSRV, and we would be "out of date". So this ultimately means the MSRV here is "arbitrarily" chosen with the rationale "a set of downstream semver-compatible dependencies can be chosen which will allow compilation under 1.63.0`.

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.

2 participants