Skip to content

Commit

Permalink
Improve display of resolution errors for workspace member conflicts w…
Browse files Browse the repository at this point in the history
…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)
  • Loading branch information
zanieb committed Aug 16, 2024
1 parent e6ddce0 commit 89efe24
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 62 deletions.
54 changes: 29 additions & 25 deletions crates/uv-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ impl std::fmt::Display for NoSolutionError {

// Transform the error tree for reporting
let mut tree = self.error.clone();
collapse_unavailable_workspace_members(&mut tree);
collapse_no_versions_of_workspace_members(&mut tree, &self.workspace_members);

if self.workspace_members.len() == 1 {
let project = self.workspace_members.iter().next().unwrap();
Expand All @@ -248,10 +248,11 @@ impl std::fmt::Display for NoSolutionError {
}
}

/// Given a [`DerivationTree`], collapse any [`UnavailablePackage::WorkspaceMember`] incompatibilities
/// Given a [`DerivationTree`], collapse any `NoVersion` incompatibilities for workspace members
/// to avoid saying things like "only <workspace-member>==0.1.0 is available".
fn collapse_unavailable_workspace_members(
fn collapse_no_versions_of_workspace_members(
tree: &mut DerivationTree<PubGrubPackage, Range<Version>, UnavailableReason>,
workspace_members: &BTreeSet<PackageName>,
) {
match tree {
DerivationTree::External(_) => {}
Expand All @@ -260,33 +261,36 @@ fn collapse_unavailable_workspace_members(
Arc::make_mut(&mut derived.cause1),
Arc::make_mut(&mut derived.cause2),
) {
// If one node is an unavailable workspace member...
(
DerivationTree::External(External::Custom(
_,
_,
UnavailableReason::Package(UnavailablePackage::WorkspaceMember),
)),
ref mut other,
)
| (
ref mut other,
DerivationTree::External(External::Custom(
_,
_,
UnavailableReason::Package(UnavailablePackage::WorkspaceMember),
)),
) => {
// First, recursively collapse the other side of the tree
collapse_unavailable_workspace_members(other);
// If we have a node for a package with no versions...
(DerivationTree::External(External::NoVersions(package, _)), ref mut other)
| (ref mut other, DerivationTree::External(External::NoVersions(package, _))) => {
// First, always recursively visit the other side of the tree
collapse_no_versions_of_workspace_members(other, workspace_members);

// Then, if the package is a workspace member...
let (PubGrubPackageInner::Package { name, .. }
| PubGrubPackageInner::Extra { name, .. }
| PubGrubPackageInner::Dev { name, .. }) = &**package
else {
return;
};
if !workspace_members.contains(name) {
return;
}

// Then, replace this node with the other tree
// Replace this node with the other tree
*tree = other.clone();
}
// If not, just recurse
_ => {
collapse_unavailable_workspace_members(Arc::make_mut(&mut derived.cause1));
collapse_unavailable_workspace_members(Arc::make_mut(&mut derived.cause2));
collapse_no_versions_of_workspace_members(
Arc::make_mut(&mut derived.cause1),
workspace_members,
);
collapse_no_versions_of_workspace_members(
Arc::make_mut(&mut derived.cause2),
workspace_members,
);
}
}
}
Expand Down
29 changes: 25 additions & 4 deletions crates/uv-resolver/src/pubgrub/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,33 +196,54 @@ impl std::fmt::Display for PubGrubPackageInner {
name,
extra: None,
marker: None,
..
dev: None,
} => write!(f, "{name}"),
Self::Package {
name,
extra: Some(extra),
marker: None,
..
dev: None,
} => {
write!(f, "{name}[{extra}]")
}
Self::Package {
name,
extra: None,
marker: Some(marker),
..
dev: None,
} => write!(f, "{name}{{{marker}}}"),
Self::Package {
name,
extra: Some(extra),
marker: Some(marker),
..
dev: None,
} => {
write!(f, "{name}[{extra}]{{{marker}}}")
}
Self::Package {
name,
extra: None,
marker: None,
dev: Some(dev),
} => write!(f, "{name}:{dev}"),
Self::Package {
name,
extra: None,
marker: Some(marker),
dev: Some(dev),
} => {
write!(f, "{name}[{dev}]{{{marker}}}")
}
Self::Marker { name, marker, .. } => write!(f, "{name}{{{marker}}}"),
Self::Extra { name, extra, .. } => write!(f, "{name}[{extra}]"),
Self::Dev { name, dev, .. } => write!(f, "{name}:{dev}"),
// It is guaranteed that `extra` and `dev` are never set at the same time.
Self::Package {
name: _,
extra: Some(_),
marker: _,
dev: Some(_),
} => unreachable!(),
}
}
}
25 changes: 14 additions & 11 deletions crates/uv-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,19 +385,22 @@ impl PubGrubReportFormatter<'_> {
/// Return a display name for the package if it is a workspace member.
fn format_workspace_member(&self, package: &PubGrubPackage) -> Option<String> {
match &**package {
PubGrubPackageInner::Package { name, .. }
| PubGrubPackageInner::Extra { name, .. }
| PubGrubPackageInner::Dev { name, .. } => {
if self.workspace_members.contains(name) {
if self.is_single_project_workspace() {
Some("your project".to_string())
} else {
Some(format!("{name}"))
}
// TODO(zanieb): Improve handling of dev and extra for single-project workspaces
PubGrubPackageInner::Package {
name, extra, dev, ..
} if self.workspace_members.contains(name) => {
if self.is_single_project_workspace() && extra.is_none() && dev.is_none() {
Some("your project".to_string())
} else {
None
Some(format!("{package}"))
}
}
PubGrubPackageInner::Extra { name, .. } if self.workspace_members.contains(name) => {
Some(format!("{package}"))
}
PubGrubPackageInner::Dev { name, .. } if self.workspace_members.contains(name) => {
Some(format!("{package}"))
}
_ => None,
}
}
Expand Down Expand Up @@ -615,7 +618,7 @@ impl PubGrubReportFormatter<'_> {
reason: reason.clone(),
});
}
Some(UnavailablePackage::NotFound | UnavailablePackage::WorkspaceMember) => {}
Some(UnavailablePackage::NotFound) => {}
None => {}
}

Expand Down
3 changes: 0 additions & 3 deletions crates/uv-resolver/src/resolver/availability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ pub(crate) enum UnavailablePackage {
InvalidMetadata(String),
/// The package has an invalid structure.
InvalidStructure(String),
/// No other versions of the package can be used because it is a workspace member
WorkspaceMember,
}

impl UnavailablePackage {
Expand All @@ -87,7 +85,6 @@ impl UnavailablePackage {
UnavailablePackage::MissingMetadata => "does not include a `METADATA` file",
UnavailablePackage::InvalidMetadata(_) => "has invalid metadata",
UnavailablePackage::InvalidStructure(_) => "has an invalid package format",
UnavailablePackage::WorkspaceMember => "is a workspace member",
}
}
}
Expand Down
15 changes: 0 additions & 15 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,21 +442,6 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
.expect("a package was chosen but we don't have a term");

if let PubGrubPackageInner::Package { ref name, .. } = &*state.next {
// Check if the decision was due to the package being a
// workspace member
if self.workspace_members.contains(name) {
state
.pubgrub
.add_incompatibility(Incompatibility::custom_term(
state.next.clone(),
term_intersection.clone(),
UnavailableReason::Package(
UnavailablePackage::WorkspaceMember,
),
));
continue;
}

// Check if the decision was due to the package being unavailable
if let Some(entry) = self.unavailable_packages.get(name) {
state
Expand Down
7 changes: 3 additions & 4 deletions crates/uv/tests/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1381,9 +1381,8 @@ fn workspace_unsatisfiable_member_dependencies_conflicting_extra() -> Result<()>
----- stderr -----
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
× No solution found when resolving dependencies:
╰─▶ Because only bar is available and bar depends on anyio==4.2.0, we can conclude that bar depends on anyio==4.2.0.
And because foo depends on anyio==4.1.0, we can conclude that foo and bar are incompatible.
And because your workspace requires bar and foo, we can conclude that your workspace's requirements are unsatisfiable.
╰─▶ Because bar[some-extra] depends on anyio==4.2.0 and foo depends on anyio==4.1.0, we can conclude that foo and bar[some-extra] are incompatible.
And because your workspace requires bar[some-extra] and foo, we can conclude that your workspace's requirements are unsatisfiable.
"###
);

Expand Down Expand Up @@ -1440,7 +1439,7 @@ fn workspace_unsatisfiable_member_dependencies_conflicting_dev() -> Result<()> {
----- stderr -----
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
× No solution found when resolving dependencies:
╰─▶ Because bar depends on bar and bar depends on anyio==4.2.0, we can conclude that bar depends on anyio==4.2.0.
╰─▶ Because bar depends on bar:dev and bar:dev depends on anyio==4.2.0, we can conclude that bar depends on anyio==4.2.0.
And because foo depends on anyio==4.1.0, we can conclude that bar and foo are incompatible.
And because your workspace requires bar and foo, we can conclude that your workspace's requirements are unsatisfiable.
"###
Expand Down

0 comments on commit 89efe24

Please sign in to comment.