Skip to content

Commit

Permalink
Alternative implementation
Browse files Browse the repository at this point in the history
  • Loading branch information
@zanieb
zanieb committed Sep 20, 2024
1 parent 0defe6e commit 9bf7a70
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 86 deletions.
133 changes: 50 additions & 83 deletions crates/uv-resolver/src/pubgrub/report.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::borrow::{Borrow, Cow};
use std::borrow::Cow;
use std::cmp::Ordering;
use std::collections::{BTreeMap, BTreeSet};
use std::ops::Bound;
Expand Down Expand Up @@ -546,6 +546,22 @@ impl PubGrubReportFormatter<'_> {
dependency,
dependency_set,
)) => {
// Check for a dependency on a workspace package by a non-workspace package.
// Generally, this indicates that the workspace package is shadowing a transitive
// dependency name.
if let (Some(package_name), Some(dependency_name)) =
(package.name(), dependency.name())
{
if workspace_members.contains(dependency_name)
&& !workspace_members.contains(package_name)
{
output_hints.insert(PubGrubHint::DependsOnWorkspacePackage {
package: package.clone(),
dependency: dependency.clone(),
workspace: self.is_workspace() && !self.is_single_project_workspace(),
});
}
}
// Check for no versions due to `Requires-Python`.
if matches!(
&**dependency,
Expand All @@ -562,7 +578,6 @@ impl PubGrubReportFormatter<'_> {
}
DerivationTree::External(External::NotRoot(..)) => {}
DerivationTree::Derived(derived) => {
Self::local_cycle_hint(workspace_members, derived, output_hints);
self.generate_hints(
&derived.cause1,
selector,
Expand Down Expand Up @@ -724,76 +739,6 @@ impl PubGrubReportFormatter<'_> {
}
}
}

/// Try to detect and warn if there is a cycle which involves a local package.
///
/// This shouldn't usually happen, but it could be the symptom of a naming
/// conflict with a transitive dependency existing on the index.
fn local_cycle_hint(
workspace_members: &BTreeSet<PackageName>,
derived: &Derived<PubGrubPackage, Range<Version>, UnavailableReason>,
output_hints: &mut IndexSet<PubGrubHint>,
) {
for local_package in workspace_members {
let pkg = PubGrubPackage::from_package(local_package.clone(), None, None.into());
if derived.terms.contains_key(&pkg) {
// Check for a direct loop between two dependencies, involving a local package...
if let DerivationTree::External(External::FromDependencyOf(src1, _, dst1, _)) =
derived.cause1.borrow()
{
let DerivationTree::External(External::FromDependencyOf(src2, _, dst2, _)) =
derived.cause2.borrow()
else {
continue;
};

// Detect a direct loop condition, with two opposing edges between the same
// packages.
let cycle_edge_first =
src1.name_no_root().is_some() && src1.name_no_root() == dst2.name_no_root();
let cycle_edge_second =
dst1.name_no_root().is_some() && dst1.name_no_root() == src2.name_no_root();
if cycle_edge_first && cycle_edge_second {
output_hints.insert(PubGrubHint::WorkspacePackageNoVersions {
package: pkg.clone(),
});
}
}
// ... otherwise, check left child for an error caused by a failed dependency
// on a local package...
else if let DerivationTree::External(External::NoVersions(_, _)) =
derived.cause1.borrow()
{
let DerivationTree::External(External::FromDependencyOf(_, _, dst, _)) =
derived.cause2.borrow()
else {
continue;
};
if *dst == pkg {
output_hints.insert(PubGrubHint::WorkspacePackageNoVersions {
package: pkg.clone(),
});
}
}
// ... otherwise, check right child for an error caused by a failed dependency
// on a local package...
else if let DerivationTree::External(External::NoVersions(_, _)) =
derived.cause2.borrow()
{
let DerivationTree::External(External::FromDependencyOf(_, _, dst, _)) =
derived.cause1.borrow()
else {
continue;
};
if *dst == pkg {
output_hints.insert(PubGrubHint::WorkspacePackageNoVersions {
package: pkg.clone(),
});
}
}
}
}
}
}

#[derive(Debug, Clone)]
Expand All @@ -819,9 +764,7 @@ pub(crate) enum PubGrubHint {
/// A package was not found in the registry, but network access was disabled.
Offline,
/// Metadata for a package could not be found.
MissingPackageMetadata {
package: PubGrubPackage,
},
MissingPackageMetadata { package: PubGrubPackage },
/// Metadata for a package could not be parsed.
InvalidPackageMetadata {
package: PubGrubPackage,
Expand Down Expand Up @@ -876,8 +819,10 @@ pub(crate) enum PubGrubHint {
// excluded from `PartialEq` and `Hash`
package_requires_python: Range<Version>,
},
WorkspacePackageNoVersions {
DependsOnWorkspacePackage {
package: PubGrubPackage,
dependency: PubGrubPackage,
workspace: bool,
},
}

Expand Down Expand Up @@ -919,8 +864,10 @@ enum PubGrubHintCore {
source: PythonRequirementSource,
requires_python: RequiresPython,
},
WorkspacePackageNoVersions {
DependsOnWorkspacePackage {
package: PubGrubPackage,
dependency: PubGrubPackage,
workspace: bool,
},
}

Expand Down Expand Up @@ -965,9 +912,15 @@ impl From<PubGrubHint> for PubGrubHintCore {
source,
requires_python,
},
PubGrubHint::WorkspacePackageNoVersions { package } => {
Self::WorkspacePackageNoVersions { package }
}
PubGrubHint::DependsOnWorkspacePackage {
package,
dependency,
workspace,
} => Self::DependsOnWorkspacePackage {
package,
dependency,
workspace,
},
}
}
}
Expand Down Expand Up @@ -1163,14 +1116,28 @@ impl std::fmt::Display for PubGrubHint {
package_requires_python.bold(),
)
}
Self::WorkspacePackageNoVersions { package } => {
Self::DependsOnWorkspacePackage {
package,
dependency,
workspace,
} => {
let your_project = if *workspace {
"one of your workspace members"
} else {
"your project"
};
let the_project = if *workspace {
"the workspace member"
} else {
"the project"
};
write!(
f,
"{}{} Resolution failed on a dependency ('{}') which has the same name as your local package ('{}'). Consider checking your project for possible name conflicts with packages in the index.",
"{}{} The package `{}` depends on the package `{}` but the name is shadowed by {your_project}. Consider changing the name of {the_project}.",
"hint".bold().cyan(),
":".bold(),
package,
package,
dependency,
)
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/uv/tests/edit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4812,7 +4812,7 @@ fn update_offset() -> Result<()> {
/// Check hint for <https://github.com/astral-sh/uv/issues/7329>
/// if there is a broken cyclic dependency on a local package.
#[test]
fn add_error_local_cycle() -> Result<()> {
fn add_shadowed_name() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
Expand All @@ -4834,7 +4834,7 @@ fn add_error_local_cycle() -> Result<()> {
× No solution found when resolving dependencies:
╰─▶ Because dagster-webserver==1.6.13 depends on your project and your project depends on dagster-webserver==1.6.13, we can conclude that your project's requirements are unsatisfiable.
hint: Resolution failed on a dependency ('dagster') which has the same name as your local package ('dagster'). Consider checking your project for possible name conflicts with packages in the index.
hint: The package `dagster-webserver` depends on `dagster` which is shadowed by your project. Consider changing the name of the project.
help: If this is intentional, run `uv add --frozen` to skip the lock and sync steps.
"###);

Expand All @@ -4859,7 +4859,7 @@ fn add_error_local_cycle() -> Result<()> {
depend on your project.
And because dagster-webserver==1.6.13 depends on your project and your project depends on dagster-webserver>=1.6.11,<1.7.0, we can conclude that your project's requirements are unsatisfiable.
hint: Resolution failed on a dependency ('dagster') which has the same name as your local package ('dagster'). Consider checking your project for possible name conflicts with packages in the index.
hint: The package `dagster-webserver` depends on `dagster` which is shadowed by your project. Consider changing the name of the project.
help: If this is intentional, run `uv add --frozen` to skip the lock and sync steps.
"###);

Expand Down

0 comments on commit 9bf7a70

Please sign in to comment.