search

GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

Fix winnow version requirement in gix-object #1540

Closed NobodyXu closed 1 month ago

NobodyXu commented 1 month ago

Fixed #1538

spenserblack commented 1 month ago

Just ran into this issue.

I don't believe winnow released a breaking release as a patch. Here's my explanation why I think this is a bug in gix, not winnow.

Cargo resolves the 0.6.0 version of winnow as ^0.6.0, or >= 0.6.0, < 0.7.0 (source). This is why winnow was 0.6.16 in your Cargo.lock, even though you specified it as 0.6.0. To lock to exactly 0.6.0, you would use =0.6.0. This resulted in you using a higher version of winnow when working in your workspace, yet downstream users could have lower, potentially incompatible versions of winnow.

Bisected this to f5cd3baf57676bfee0c40fee577d2779958bbe72 by bisecting between gix 0.66 and 0.64, locking winnow to =0.6.0 at each step. Looks like you need at least winnow 0.6.15. This makes sense, since winnow introduced the .take() method in https://github.com/winnow-rs/winnow/commit/28fdb2f08067ce1b02a90b4e1d0d39c14e258b09, which was then released in 0.6.15. This issue is not that winnow broke usage. Just the opposite -- they renamed .recognize to .take, but they kept .recognize as a deprecated method to ensure backwards compatibility. The issue is that gix-object started calling a method that does not exist in winnow < 0.6.15, without specifying that it requires winnow >= 0.6.15. So downstream users continued to use winnow <0.6.15, causing the failure to compile.

In the future, to ensure the best compatibility, it's probably going to be best to not update direct dependencies directly in the Cargo.lock, but update the required version specs in your Cargo.toml files, instead. Especially if you're changing your usage of those dependencies. So, in this example, because you were dropping usage of the deprecated .recognize for .take, you should have updated your Cargo.toml to specify a version greater than or equal to the winnow version that introduced the deprecation. In other words, if you need to use the latest and greatest version of a dependency, your Cargo.toml should show this so downstream users get the message "hey, we need this new version, go get it!"

Hope this helps!

Byron commented 1 month ago

Bisected this to f5cd3ba by bisecting between gix 0.66 and 0.64, locking winnow to =0.6.0 at each step. Looks like you need at least winnow 0.6.15. This makes sense, since winnow introduced the .take() method in winnow-rs/winnow@28fdb2f, which was then released in 0.6.15. This issue is not that winnow broke usage. Just the opposite -- they renamed .recognize to .take, but they kept .recognize as a deprecated method to ensure backwards compatibility. The issue is that gix-object started calling a method that does not exist in winnow < 0.6.15, without specifying that it requires winnow >= 0.6.15. So downstream users continued to use winnow <0.6.15, causing the failure to compile.

Thanks for the detective-work! I see now how this could happen, and that I should have adjusted the minimum required version of winnow in Cargo.toml accordingly.

Hope this helps!

Yes, it does! Interestingly, I am very aware of the differences between Cargo.lock and Cargo.toml, yet in practice I have failed to do the right thing. Interesting.