Resolver = 2 causing build issues with 1.8.1

[LucioFranco]

Hi!

Just started to see failures with an older msrv (1.46) with 1.8.1. https://github.com/tokio-rs/prost/runs/3976399124?check_suite_focus=true

I believe maybe it makes sense to only add resolver = 2 w/ 2021 edition update and not 1.8.1? The real solution for prost is to update to the new edition but I didn't expect this breakage.

Thanks!

[homersimpsons]

There is the same issue for nom: https://github.com/Geal/nom/runs/3976237272?check_suite_focus=true

[LucioFranco]

The fix for prost is quite simple but just required bumping the msrv. tokio-rs/prost#552

That said, I might suggest yanking 1.8.1 since I believe this might actually be a breaking change for crates that have a msrv older than 1.51. That said most projects follow the 6month msrv rule (which prost does and means we can bump without needing this crate yanked), so may be a non issue.

[Geal]

Unfortunately, some projects, like nom, still use older versions so they can ship on Debian stable (currently it uses 1.48): https://packages.debian.org/search?keywords=rustc

[starkat99]

So, 1.8.0 already specifically bumped the msrv to 1.51, so I'm not gonna yank 1.8.1, but I will put out a 1.8.2 without resolver=2 since that's mostly for CI to verify the 1.8.1 fixes, and technically it's only optional features that require the bumped msrv which is likely why some crates didn't see the breakage on older rust versions with 1.8.0. Crates that need to work with earlier msrv than 1.51 should really stick with 1.7 and earlier of this crate to prevent any further breakage with 1.8 series.

[Geal]

that's not something that's easy to control 😅 from nom's point of view, the dep branch is half > serde_cbor > criterion > nom

I'l probably be able to fix that by making criterion optional in CI, but there are probably other crates that will encounter it

[starkat99]

Ugh, the drawbacks to 6-month minor version bump MSRV strategy 😢 The alternative is to do a major version bump to 2.0 and yank all the 1.8 versions. I kinda hate yanking 1.8 entirely, but it does seem that a major version change for MSRV changes may be the way to go for this low level a crate...

[starkat99]

Thinking about it more, I think going with MSRV changes requiring a major version bump is the right policy here for this crate. I'm still not sure if I'm going to yank 1.8 entirely, but I will yank 1.8.1 and do the 1.8.2 without the resolver as planned

[starkat99]

1.8.2 has been published without resolver=2, which should resolve this issue

[Stargateur]

Can we have a explanation of the why of this problem ? I read https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html but I didn't see it.

Oh I get it, since you add resolver=2, you introduce a MSVR bump for stable that prevent nom from compiling.

[starkat99]

Correct. I'll sum up a TLDR here of this incident:

resolver attribute for cargo manifest was added in...1.50 I believe? And while officially 1.8.0 had bumped its MSRV to 1.51, that was only due to optional dependencies. Without those optional dependencies (most crates not using the particular optional dependency), the MSRV was effectively still 1.47/1.46 (according to lib.rs). So 1.8.1 was technically a breaking change in patch version, and why I decided yanking was a good idea.

1.8.2 is published and resolves the issue. Going forward, this crate will use a major version change for any MSRV changes, to prevent this issue, as its much lower-level than crates that can get away with 6-month-old MSRV support in minor version changes.

[Stargateur]

while officially 1.8.0 had bumped its MSRV to 1.51

Oh, Nom fault so no ? or criterion fault, I see, complex problem. Hope we could could solve this problem with a cargo msrv feature one day.

[Geal]

Nobody is really at fault here. @starkat99 bumped the MSRV in a major version, which is fine. And nom uses criterion only for benchmarks, so it should not affect tests and nom's MSRV. The basic issue here is that optional dev dependencies are not possible

[Geal]

Thank you for your help @starkat99 !

[homersimpsons]

Hope we could could solve this problem with a cargo msrv feature one day.

This is possible from rust 1.56 https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

I guess that after a major release to 1.56 as a MSRV, increasing MSRV could not be a breaking change anymore.

[Stargateur]

@homersimpsons unfortunately this is not enough, this is very useful to have explicit error message, but this doesn't prevent "mistake" by doing this recursively on all dep package (AFAIK) So this would not have prevent this issue. (but that clearly open the door to such feature)

Or it does and I miss read, I'm not good to understand thing without concrete explanation.

[homersimpsons]

@Stargateur you are right, this is currently under discussion on the following issue rust-lang/cargo#9930

[LucioFranco]

I appreciate the quick fix and help @starkat99! :)