Improving the error message for when a patched dependency does not resolve to anything #4607
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Thanks for the PR! I wonder if we could perhaps try to go the extra mile here as well? Ideally what we'd do is upon failure do a query again for a more relaxed version constraint, seeing if you just got the wrong version. If that fails we could say that the package doesn't even exist in the source. What do you think? |
|
I think that's a great idea! That would be much better than the behavior we have now. The reason this error is so confusing in the first place is because it isn't aligned with what I would expect cargo to do. It's not that rustfmt isn't there, it's that the version is incorrect. The error doesn't give me any ideas about what to look for to solve the problem. What do you think of modifying the error message for now and then opening a new issue for the more long term solution that you suggested? That way this is more clear for the time being and it'll get even better in the future once someone has time to implement it. Also, please don't hesitate to give me feedback on the wording of the message. I'm happy to update it! :) |
|
@alexcrichton Hi Alex, could you take a look at my comment above and reply to it? Thanks so much for your time! 😄 |
|
Ah right sorry forgot to come back to this. I'm somewhat hesitant to land this as-is without improvements because it can be a confusing error message. For example if you depend on a nonexistent package or a nonexistent version then that has nothing to do with Cargo.lock. On one hand I don't want to block this on more work happening for improving the message, but on the other I'm wary of the extra issues that will get opened asking for clarification :( |
|
Maybe we could add a link to the relevant issue in the error message? I think it's important to have something here so that people aren't left as confused as I was originally. The error message is quite misleading. Is there a message you think we could use until this is fixed with more sophisticated behaviour? |
|
Yeah that sounds good to me, something like the orignial message appended with |
|
@alexcrichton I created an issue (#4678) and included all of the details from this PR. Please feel free to edit it to add any details or change anything I wrote (comments are fine too). I updated this PR to include the changes you asked for including a link to that issue. This should be good to go now! Let me know if you would like me to make any other changes! 😄 |
|
@bors: r+ Thanks! |
|
📌 Commit 7b4a4b5 has been approved by |
Improving the error message for when a patched dependency does not resolve to anything Hi, I just spent a long time debugging what this error message means and so I thought I would try to help improve it. Could someone who knows the codebase well look at this and see if this error message even makes sense here. I know this is the information I would have needed, but the error may occur in other cases where this message doesn't make sense. I'm open to any suggestions that would help make it more clear and maybe point people to where they should look for more information. The specific problem that I ran into occurred when I was trying to upgrade the `rustfmt` submodule in the rust codebase. The `rustfmt-nightly` dependency changed version numbers and this conflicted with what was in the `Cargo.lock` file. After some detective work I was able to find the [documentation on `[patch]`](http://doc.crates.io/manifest.html#the-patch-section) which I had never read before and the following relevant paragraphs from [some other documentation](http://doc.crates.io/specifying-dependencies.html#testing-a-bugfix): > Next up we need to ensure that our lock file is updated to use this new version of uuid so our project uses the locally checked out copy instead of one from crates.io. The way [patch] works is that it'll load the dependency at ../path/to/uuid and then whenever crates.io is queried for versions of uuid it'll also return the local version. > > This means that the version number of the local checkout is significant and will affect whether the patch is used. Our manifest declared uuid = "1.0" which means we'll only resolve to >= 1.0.0, < 2.0.0, and Cargo's greedy resolution algorithm also means that we'll resolve to the maximum version within that range. Typically this doesn't matter as the version of the git repository will already be greater or match the maximum version published on crates.io, but it's important to keep this in mind! If it was not for the person who wrote those docs, I would not have known what to do here!
|
☀️ Test successful - status-appveyor, status-travis |
Hi,
I just spent a long time debugging what this error message means and so I thought I would try to help improve it. Could someone who knows the codebase well look at this and see if this error message even makes sense here. I know this is the information I would have needed, but the error may occur in other cases where this message doesn't make sense. I'm open to any suggestions that would help make it more clear and maybe point people to where they should look for more information.
The specific problem that I ran into occurred when I was trying to upgrade the
rustfmtsubmodule in the rust codebase. Therustfmt-nightlydependency changed version numbers and this conflicted with what was in theCargo.lockfile. After some detective work I was able to find the documentation on[patch]which I had never read before and the following relevant paragraphs from some other documentation:If it was not for the person who wrote those docs, I would not have known what to do here!