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

Fix winnow version requirement in gix-object #1540

Merged
merged 1 commit into from
Aug 23, 2024
Merged

Conversation

NobodyXu
Copy link
Contributor

Fixed #1538

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

I still think that winnow can't break between patches without being non-compliant to semver.
In any case, I hope that this along with the improved msrv check will prevent such issues in future.

@Byron Byron merged commit f4898b9 into Byron:main Aug 23, 2024
18 checks passed
@NobodyXu NobodyXu deleted the patch-1 branch August 23, 2024 11:58
@spenserblack
Copy link
Contributor

spenserblack commented Aug 26, 2024

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 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.

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
Copy link
Owner

Byron commented Aug 27, 2024

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.

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.

gix-object v0.44.0 fail to compile
3 participants