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 resolver error messages referencing workspace members #6092

Merged
merged 9 commits into from
Aug 15, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Aug 14, 2024

An extension of #6090 that replaces #6066.

In brief,

  1. Workspace member names are passed to the resolver for no solution errors
  2. There is a new derivation tree pre-processing step that trims NoVersion incompatibilities for workspace members from the derivation tree. This avoids showing redundant clauses like Because only bird==0.1.0 is available and bird==0.1.0 depends on anyio==4.3.0, we can conclude that all versions of bird depend on anyio==4.3.0.. As a minor note, we use a custom incompatibility kind to mark these incompatibilities at resolution-time instead of afterwards.
  3. Root dependencies on workspace members say your workspace requires bird rather than you require bird
  4. Workspace member package display omits the version, e.g., bird instead of bird==0.1.0
  5. Instead of reporting a workspace member as unusable we note that its requirements cannot be solved, e.g., bird's requirements are unsatisfiable instead of bird cannot be used.
  6. Instead of saying your requirements are unsatisfiable we say your workspace's requirements are unsatisfiable when in a workspace, since we're not in a "provide direct requirements" paradigm.

As an annoying but minor implementation detail, PackageRange now requires access to the PubGrubReportFormatter so it can determine if it is formatting a workspace member or not. We could probably improve the abstractions in the future.

As a follow-up, we should additional special casing for "single project" workspaces to avoid mention of the workspace concept in simple projects. However, it looks like this will require additional tree manipulations so I'm going to keep it separate.

@@ -388,7 +388,7 @@ werkzeug==3.0.1

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because flask==3.0.2 depends on click>=8.1.3 and you require click==7.0.0, we can conclude that your requirements and flask==3.0.2 are incompatible.
╰─▶ Because flask==3.0.2 depends on click>=8.1.3 and you require click==7.0.0, we can conclude that the requirements and flask==3.0.2 are incompatible.
Copy link
Member

Choose a reason for hiding this comment

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

"the" is maybe suspect here, is it better?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, sorry I should have called this out. This made it easier to consolidate some logic without changing a ton of snapshots. I'm planning to change it to "your" everywhere in a follow-up.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Rad.

@@ -903,9 +987,49 @@ struct PackageRange<'a> {
package: &'a PubGrubPackage,
range: &'a Range<Version>,
kind: PackageRangeKind,
formatter: Option<&'a PubGrubReportFormatter<'a>>,
Copy link
Member

Choose a reason for hiding this comment

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

Yeah that feels a bit off. But at least it's all contained in this file.

@zanieb zanieb force-pushed the zb/resolver-errors-workspaces branch from e4b0e3e to c32128c Compare August 15, 2024 00:30
@zanieb zanieb force-pushed the zb/resolver-errors-workspaces branch from c32128c to b03445b Compare August 15, 2024 00:37
@zanieb zanieb enabled auto-merge (squash) August 15, 2024 02:35
@zanieb zanieb merged commit 2e3e6a0 into main Aug 15, 2024
57 checks passed
@zanieb zanieb deleted the zb/resolver-errors-workspaces branch August 15, 2024 02:41
zanieb added a commit that referenced this pull request Aug 15, 2024
Extends #6092 to improve resolver
error messages for workspaces that have a single member.

As before, this requires a two-step approach of

1. Traversing the derivation tree and collapsing some members. In this
case, we drop the empty root node in favor of the project.
2. Using special-case formatting for packages. In this case, the
workspace package is referred to with "your project" instead of its
name.
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice!

╰─▶ Because there are no versions of xyz and project==0.1.0 depends on xyz, we can conclude that project==0.1.0 cannot be used.
And because only project==0.1.0 is available and you require project, we can conclude that the requirements are unsatisfiable.
╰─▶ Because there are no versions of xyz and project depends on xyz, we can conclude that project's requirements are unsatisfiable.
And because your workspace requires project, we can conclude that your workspace's requirements are unsatisfiable.
Copy link
Member

Choose a reason for hiding this comment

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

This looks much nicer.

zanieb added a commit that referenced this pull request Aug 16, 2024
…ith optional dependencies (#6123)

We have bad error messages for optional (extra) dependencies and
development dependencies in workspaces:

1. We weren't showing the full package, so we'd drop `:dev` and
`[extra]` by accident
2. We didn't include derived packages, e.g., `member[extra]` in tree
processing collapse operation, so we'd include extra clauses like the
ones we removed in #6092

Also

- Reverts
f0de4f7
— it turns out it wasn't quite correct and it didn't seem worth using
the custom incompatibility anymore.
- Fixes a bug in the display of `package:dev` which was not showing
`:dev` for some variants (see 94d8020)
zanieb added a commit that referenced this pull request Aug 16, 2024
Uses my expanding tree reduction knowledge from #6092 to improve the
long-standing issue of verbose messages for unavailable packages.

Implements pubgrub-rs/pubgrub#232, but
post-resolution instead of during resolution.

Partially addresses #5046
Closes #2519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Messaging when something goes wrong preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants