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

Include more sources to avoid lowest bound warning #9644

Merged
merged 3 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions crates/uv-resolver/src/resolution/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ impl ResolverOutput {
graph.retain_nodes(|graph, node| !graph[node].marker().is_false());

if matches!(resolution_strategy, ResolutionStrategy::Lowest) {
report_missing_lower_bounds(&graph, &mut diagnostics);
report_missing_lower_bounds(&graph, &mut diagnostics, constraints, overrides);
}

let output = Self {
Expand Down Expand Up @@ -879,6 +879,8 @@ impl From<ResolverOutput> for uv_distribution_types::Resolution {
fn report_missing_lower_bounds(
graph: &Graph<ResolutionGraphNode, UniversalMarker>,
diagnostics: &mut Vec<ResolutionDiagnostic>,
constraints: &Constraints,
overrides: &Overrides,
) {
for node_index in graph.node_indices() {
let ResolutionGraphNode::Dist(dist) = graph.node_weight(node_index).unwrap() else {
Expand All @@ -892,7 +894,8 @@ fn report_missing_lower_bounds(
// have to drop.
continue;
}
if !has_lower_bound(node_index, dist.name(), graph) {

if !has_lower_bound(node_index, dist.name(), graph, constraints, overrides) {
diagnostics.push(ResolutionDiagnostic::MissingLowerBound {
package_name: dist.name().clone(),
});
Expand All @@ -905,6 +908,8 @@ fn has_lower_bound(
node_index: NodeIndex,
package_name: &PackageName,
graph: &Graph<ResolutionGraphNode, UniversalMarker>,
constraints: &Constraints,
overrides: &Overrides,
) -> bool {
for neighbor_index in graph.neighbors_directed(node_index, Direction::Incoming) {
let neighbor_dist = match graph.node_weight(neighbor_index).unwrap() {
Expand All @@ -931,7 +936,10 @@ fn has_lower_bound(
for requirement in metadata
.requires_dist
.iter()
// These bounds sources are missing from the graph.
.chain(metadata.dependency_groups.values().flatten())
.chain(constraints.requirements())
.chain(overrides.requirements())
{
if requirement.name != *package_name {
continue;
Expand Down
21 changes: 18 additions & 3 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2277,11 +2277,11 @@ impl ForkState {
for_version: &Version,
urls: &Urls,
indexes: &Indexes,
mut dependencies: Vec<PubGrubDependency>,
dependencies: Vec<PubGrubDependency>,
git: &GitResolver,
resolution_strategy: &ResolutionStrategy,
) -> Result<(), ResolveError> {
for dependency in &mut dependencies {
for dependency in &dependencies {
let PubGrubDependency {
package,
version,
Expand Down Expand Up @@ -2313,6 +2313,22 @@ impl ForkState {
// A dependency from the root package or requirements.txt.
debug!("Adding direct dependency: {package}{version}");

let name = package.name_no_root().unwrap();

// Catch cases where we pass a package once by name with extras and then once as
// URL for the specific distribution.
has_url = has_url
|| dependencies
.iter()
.filter(|other_dep| *other_dep != dependency)
.filter(|other_dep| {
other_dep
.package
.name()
.is_some_and(|other_name| other_name == name)
})
.any(|other_dep| other_dep.url.is_some());
Copy link
Member

Choose a reason for hiding this comment

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

This is quadratic, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to add the comment to the PR: This only runs for direct deps, so the quadratic loop runs about once, and in the number of direct dependencies.


// Warn the user if a direct dependency lacks a lower bound in `--lowest` resolution.
let missing_lower_bound = version
.bounding_range()
Expand All @@ -2327,7 +2343,6 @@ impl ForkState {
"The direct dependency `{name}` is unpinned. \
Consider setting a lower bound when using `--resolution lowest` \
to avoid using outdated versions.",
name = package.name_no_root().unwrap(),
);
}
}
Expand Down
44 changes: 44 additions & 0 deletions crates/uv/tests/it/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19009,3 +19009,47 @@ fn lock_recursive_extra() -> Result<()> {

Ok(())
}

/// Don't show an unpinned lower bound warning if the package was provided both by name and by URL,
/// or if the package was part of a constraints files.
///
/// See <https://github.com/astral-sh/uv/issues/8155>, the original example was:
/// ```
/// uv pip install dist/pymatgen-2024.10.3.tar.gz pymatgen[ci,optional] --resolution=lowest
/// ```
#[test]
fn no_lowest_warning_with_name_and_url() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.12"
dependencies = [
"anyio[trio]",
"anyio @ https://files.pythonhosted.org/packages/e6/e3/c4c8d473d6780ef1853d630d581f70d655b4f8d7553c6997958c283039a2/anyio-4.4.0.tar.gz"
]

[tool.uv]
constraint-dependencies = [
"sortedcontainers==2.4.0",
"outcome==1.3.0.post0",
"pycparser==2.20",
]
"#,
)?;

uv_snapshot!(context.filters(), context.lock(), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 10 packages in [TIME]
"###);

Ok(())
}
Loading