A project can not depend on itself as a path dependency#9702
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
If we want to explicitly disallow this then I think it's best to do it in the cyclic detection of the resolver rather than in the manifest parsing code, since the resolver has a better idea of what connects to what. |
ehuss
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #9827) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping @yerke Just checking in to see if you are still interested in working on this, or if you had any questions. |
|
@ehuss thanks for checking. I am sorry for taking forever. I hope I will have time around Christmas for this. |
yerke
force-pushed
the
forbid-self-path-dependency
branch
from
d13c889 to
2009343
Compare
There was a problem hiding this comment.
I am not sure how to correctly test this after my changes.
There was a problem hiding this comment.
@ehuss @alexcrichton Do you have any suggestions on how to modify that test? Thanks.
|
@ehuss @alexcrichton Do you mind having a look when you get a chance? I want to know if I am on the right track. Thanks! |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
This seems to be along the right lines, yeah. Can you add a comment in check_cycles about what this is doing? It feels a bit odd that it's only checking for one-level-deep cycles when cycles can generally be many layers deep, but this is only applicable for dev-dependecies-on-itself and other packages transitively depending back on the first seems ok
|
There is a discussion on why this shouldn't be made a hard-error just yet here: |
yerke
force-pushed
the
forbid-self-path-dependency
branch
from
2009343 to
ca0163b
Compare
|
@alexcrichton Thanks for your feedback. I added the comment. |
|
I'm going to close this for now, as there is some uncertainty if this is the right thing to do (per #9518 (comment)) and this is not a high priority for us to fix at this time. Thank you for the PR, though! We may resurrect this in the future when we have more capacity to review and clarity on how this should work. |
6 participants
Closes #9518