search

eupn / macrotest

Test harness for declarative and procedural macros expansion via `cargo-expand`
47 stars 9 forks source link

MSRV violation #68

Closed Emoun closed 1 year ago

Emoun commented 2 years ago

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 commented 2 years ago

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 https://github.com/rust-lang/api-guidelines/pull/227#discussion_r527102913 and https://github.com/rust-lang/api-guidelines/pull/227/commits/38e610d13446dd81a09c81ec6121037706ee74d4:

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/smol-rs/async-process/pull/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 commented 2 years ago

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.