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

Use available versions to simplify unsat error reports #547

Merged
merged 14 commits into from
Dec 12, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Dec 4, 2023

Uses pubgrub-rs/pubgrub#156 to consolidate version ranges in error reports using the actual available versions for each package.

Alternative to astral-sh/pubgrub#8 which implements this behavior as a method in the Reporter — here it's implemented in our custom report formatter (#521) instead which requires no upstream changes.

Requires astral-sh/pubgrub#11 to only retrieve the versions for packages that will be used in the report.

This is a work in progress. Some things to do:

  • We may want to allow lazy retrieval of the version maps from the formatter
  • We should probably create a separate error type for no solution instead of mixing them with other resolve errors
  • We can probably do something smarter than creating vectors to hold the versions
  • This degrades error messages when a single version is not available, we'll need to special case that
  • It seems safer to coerce the error type in resolve instead of solve if feasible

@zanieb
Copy link
Member Author

zanieb commented Dec 8, 2023

With astral-sh/pubgrub#12 this does not degrade error messages anymore.

@zanieb
Copy link
Member Author

zanieb commented Dec 8, 2023

Ready for review but the PubGrub changes should be merged first.

Additionally, we don't have a test case that's changed by this :) that is my next priority.

@zanieb zanieb marked this pull request as ready for review December 8, 2023 18:09
@charliermarsh
Copy link
Member

Request me as a reviewer if ready :)

Comment on lines +83 to +89
#[error("{package} {version} depends on itself")]
SelfDependency {
/// Package whose dependencies we want.
package: Box<PubGrubPackage>,
/// Version of the package for which we want the dependencies.
version: Box<PubGrubVersion>,
},
Copy link
Member

Choose a reason for hiding this comment

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

I know that is an error type in pubgrub, but fwiw we don't consider this an error (it's surprisingly popular in python packaging)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great it'd probably be good to remove it then

crates/puffin-resolver/src/pubgrub/report.rs Outdated Show resolved Hide resolved
@zanieb zanieb enabled auto-merge (squash) December 12, 2023 23:21
@zanieb zanieb merged commit 490fb55 into main Dec 12, 2023
3 checks passed
@zanieb zanieb deleted the zb/simplify-pubgrub-report branch December 12, 2023 23:25
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.

3 participants