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

Improve resolve error messages #6665

Closed

Conversation

spacekookie
Copy link
Member

Continuation of PR #6374, including all previous commits
Now also includes the rebased changes from origin/master

Closes #6199

r? @Eh2406
cc/@Dylan-DPC

@Eh2406
Copy link
Contributor

Eh2406 commented Feb 14, 2019

Looks like the commits are from before we moved to edition=2018, so the rebase may have left incoherent code.

I like the use of the word satisfy! Do you have thoughts on how to make it less diagonal?

@spacekookie
Copy link
Member Author

The last commit should resolve the CI issues.

Also, reading through the feedback in the other thread, I really quite like this suggestion for structuring the error output.

I might give implementing that a go and see how it feels in pracise

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 2, 2019

Hay anything I can do to help you with this?

I just remembered another source of inspiration on resolver error messages. Dart's package manager has great error messages, we do not (yet) have the data structure to steal it wholesale, but it may be a great place to get inspiration.

@bors
Copy link
Collaborator

bors commented Mar 6, 2019

☔ The latest upstream changes (presumably #6653) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

I'm gonna close this to help clear out our queue, but it can of course be resubmitted!

bors added a commit that referenced this pull request Aug 25, 2021
Improve resolver message to include dependency requirements

Resolves #6199.

Thanks for previous efforts: #5452, #6374, #6665, which are great but somehow outdated, so I tweak them and create this PR. This will also be obsolete if we ship pubgrub-rs with cargo in the future 😃 But before that happens, IMO these changes are still helpful.

---

This PR changes the resolver error message from

https://github.com/rust-lang/cargo/blob/216f915c46b8ada2323423d049314ba18247ef95/tests/testsuite/build.rs#L1104-L1106

to

https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L1104-L1106

Also provide different message for different source kinds, such like:

https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L2810-L2812

## TODO?

From #5452 (comment), there shall be at least one task left behind:

> 3. Special case pind by a lock file and not a `"=1.1.2"` in a dependency. Also add a "note: try cargo update" to the end.

In this PR, `validate_links` also faces this issue that a dependency requirement is locked into a precise version `=0.1.0`.

https://github.com/rust-lang/cargo/blob/a5f8bc94f5d38539dd127f735ea4d3a515c230fd/tests/testsuite/build_script.rs#L1002-L1004

I am uncertain about how to resolve this. Besides  the function`validate_links`, is this problem really a thing that may happen? If not, since `validate_links` only handles old validation logic, it may be ok to drop the commit a5f8bc9 and leave it as is.
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.

Resolvers error messages should include the version requirements.
5 participants