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

MSRV violation #68

Closed
Emoun opened this issue Dec 26, 2021 · 4 comments · Fixed by #91
Closed

MSRV violation #68

Emoun opened this issue Dec 26, 2021 · 4 comments · Fixed by #91
Assignees

Comments

@Emoun
Copy link
Contributor

Emoun commented Dec 26, 2021

macrotest no longer compiles on rust v1.34.2 because serde_json v1.0.73 depends on itoa v1.0.1 which doesn't compile on that version. See (duplicate build)[https://github.com/Emoun/duplicate/runs/4636793495?check_suite_focus=true].

As far as I can tell serde_json has no MSRV, which means macrotest should depend on a specific version to avoid violating MSRV.

@taiki-e
Copy link
Collaborator

taiki-e commented Dec 26, 2021

I would suggest raising the MSRV of macrotest to 1.36 which is the MSRV of serde_json. This crate is a crate for testing and there doesn't seem to be much worth sticking around to support a very old compiler.
See also rust-lang/api-guidelines#227 (comment) and rust-lang/api-guidelines@38e610d:

Some crate bumps MSRV of the crate itself due to MSRV bump of the dev-dependency, but in some cases, the crate itself can be compiled with an older version, even if the dev-dependency requires a newer version.
I'm not sure if it's really preferable to recommend cargo test instead of cargo build here.
AFAIK there are many projects that do not run cargo test on MSRV, but only run cargo build. (e.g., serde, syn, regex, tokio, futures, etc...)

In practice, dev-deps often have much more lenient MSRV policy than then crate under test. Using build rather than test works around that.


macrotest should depend on a specific version to avoid violating MSRV.

I think this is far worse than violating the MSRV because if other dependencies requires the recent version of serde_json, this causes compile error.

I think the right place to do this is in the end-user's Cargo.toml or Cargo.lock (example).


serde_json has no MSRV

No. serde_json v1.0.73's MSRV is Rust 1.36. See rust-version field in serde-json's Cargo.toml.

@Emoun
Copy link
Contributor Author

Emoun commented Dec 26, 2021

if other dependencies requires the recent version of serde_json, this causes compile error

Doesn't cargo just compile it twice?

serde_json v1.0.73's MSRV is Rust 1.36. See rust-version field in serde-json's Cargo.toml.

Thanks, i missed that. Though, does the project maintain the MSRV across versions?
Regardless, since serde_json's MSRV is higher than macrotest's, it seems to me fixing the version would be needed to comply with the current MSRV.

I'm not strictly against macrotest upping it's MSRV, I've been thinking about doing it for duplicate. However, I think it should be part of a major version bump.

@taiki-e
Copy link
Collaborator

taiki-e commented Dec 26, 2021

Doesn't cargo just compile it twice?

It will be compiled twice if the major versions (or minor versions if the major version is 0) are different. In other cases, it will be conflict and cause compile error. See smol-rs/async-process#4 for a specific example.

However, I think it should be part of a major version bump.

AFAIK, many cornerstone libraries (include serde_json) don't consider an MSRV increase as a breaking change.
This comment by matklad is one of the explanations that I find particularly reasonable as to why such a policy should be adopted.

See also:

@Emoun
Copy link
Contributor Author

Emoun commented Dec 26, 2021

AFAIK, many cornerstone libraries (include serde_json) don't consider an MSRV increase as a breaking change.

Yeah, my opinion is not mainstream sadly, and would therefore also fully respect if macrotest decides to only bump the minor version.

@eupn eupn self-assigned this Dec 28, 2021
@eupn eupn closed this as completed in #91 Aug 12, 2023
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 a pull request may close this issue.

3 participants