How to assist with bug #5478

I've been regularly hitting up against Bug #5478, (Patching dependencies does not work if it's for the same location but a different branch) since I started writing Rust 4 years ago. Instead of continue to cursing it and complaining to my keyboard, I'm wondering what it would take to fix it myself?

However, my assumption is that it's remained open for 5 years because there are dragons involved, requiring deep understanding of how the Cargo resolver works. The tag S-needs-design was added last year, which also hints at there being complexities around it. But I don't have evidence for that assumption.

So my ask (as someone unfamiliar with the cargo processes) is if there are records of conversations, meetings, blogs, etc. that might help those of us outside the Rust team what one should consider before naively jumping in to try to fix this particular issue? I don't want to create work for others if it's a futile endeavor for the uninitiated, but if it's just a matter of turning the maintenance crank by someone who knows how to submit PRs, maybe I shouldn't be so daunted? Anyone care to offer advice?

2 Likes

This isn't a question of implementation complexity but coming up with a user-facing design that meets peoples needs, maintains compatibility, and would have a reasonable implementation. So far in the thread, no one has discussed ideas for what solving this would look like to users.

Thanks for the reply. The initial comment to the OP is that it's an unfortunate side-effect of the implementation of patch in that it only looks at URLs. But it's not obvious to me why one couldn't incorporate all the dependency coordinates in the uniqueness comparison. Is that a viable approach? Should I raise that in the ticket?

1 Like

If its just a question of implementation, you wouldn't notice the difference. This is a user-facing behavior and an analysis would need to be done to understand what impact on behavior changing this would have. Would this be a breaking change? Was this documented or intended behavior? If it isn't, this can give us a little more latitude if the scope or impact of a behavior change is small.

1 Like

Thank you for your time. I wouldn't even know where to start with answering questions like was this documented or intended behavior, so I suspect my attempting a solution will just create distracting work for others.

The current behavior is that Cargo fails to build the project; so permitting the patch requested in the issue to work can't be a breaking change, can it?

5 Likes

I would expect this feature to treat a git source as url + branch. Right now, it seems that only the url is used to identify the source, which causes a clash.

Since the default branch can be used, I'm not sure how this feature should behave if you patch a git dependency which uses the default branch with the same url, and same branch mentioned explicitly. I think I would want this not to count as a clash.

1 Like

I can't think of any case where users would want the "patches must point to different sources" error to happen. It doesn't seem like an intentional user-centric feature, but purely a limitation of the implementation or an unnecessary restriction.

I've also seen users wanting to patch dependencies with crates that have a different name (somecrate -> mycrate-fork) and different version (force-upgrade from v0.7.0 to v0.8.0).

From user-facing perspective the expectation is for Cargo to "just" take whatever has been specified as the replacement and use it.

The complication here is from what Cargo sees as separate copies of a crate in the dependency tree, and the ambiguity of the patch syntax:

  • there can be multiple versions of the same crate, but patch can't specify which version to replace. It ends up requiring that the version is the same, which is like specifying version to replace backwards, from the replacement crate. ([replace] was too specific in the other direction wanting pkgid with the exact patch version)

  • there can be multiple instances of a git crate with the same name from the same git repo, caused by the fact that rev and branch are part of pkgid literally (not even translated to the target commit). From what I've seen this is completely unexpected by users. I'd be nice to change that in Cargo in general, but for the sake of the patch section Cargo should unify and replace all instances based on just the repo URL and package name.

9 Likes