From b3b4b47394c48e3375fee208780236bfa6a583d4 Mon Sep 17 00:00:00 2001 From: Chan Kang Date: Fri, 28 Jun 2024 09:28:39 -0400 Subject: [PATCH] fix the incorrect handling of markers in `pip tree` (#4611) ## Summary resolves https://github.com/astral-sh/uv/issues/4609 previously, the implementation of `required_with_no_extra` was incorrect, particularly when there are packages that do not require any extras but have other types of markers. ## Test Plan the existing tests also did cover this (my bad... missed it) but added a smaller test since this bug would've been more obvious with this new test. --- crates/uv/src/commands/pip/tree.rs | 34 ++++++---- crates/uv/tests/pip_tree.rs | 100 +++++++++++++++++++++-------- 2 files changed, 97 insertions(+), 37 deletions(-) diff --git a/crates/uv/src/commands/pip/tree.rs b/crates/uv/src/commands/pip/tree.rs index b1858d6f2..07dba401f 100644 --- a/crates/uv/src/commands/pip/tree.rs +++ b/crates/uv/src/commands/pip/tree.rs @@ -2,6 +2,7 @@ use std::fmt::Write; use distribution_types::{Diagnostic, InstalledDist, Name}; use owo_colors::OwoColorize; +use pep508_rs::MarkerEnvironment; use tracing::debug; use uv_cache::Cache; use uv_configuration::PreviewMode; @@ -47,9 +48,15 @@ pub(crate) fn pip_tree( // Build the installed index. let site_packages = SitePackages::from_environment(&environment)?; - let rendered_tree = DisplayDependencyGraph::new(&site_packages, depth.into(), prune, no_dedupe) - .render() - .join("\n"); + let rendered_tree = DisplayDependencyGraph::new( + &site_packages, + depth.into(), + prune, + no_dedupe, + environment.interpreter().markers(), + ) + .render() + .join("\n"); writeln!(printer.stdout(), "{rendered_tree}").unwrap(); if rendered_tree.contains('*') { writeln!( @@ -82,18 +89,16 @@ pub(crate) fn pip_tree( // For example, `requests==2.32.3` requires `charset-normalizer`, `idna`, `urllib`, and `certifi` at // all times, `PySocks` on `socks` extra and `chardet` on `use_chardet_on_py3` extra. // This function will return `["charset-normalizer", "idna", "urllib", "certifi"]` for `requests`. -fn required_with_no_extra(dist: &InstalledDist) -> Vec> { +fn required_with_no_extra( + dist: &InstalledDist, + marker_environment: &MarkerEnvironment, +) -> Vec> { let metadata = dist.metadata().unwrap(); return metadata .requires_dist .into_iter() .filter(|r| { - r.marker.is_none() - || !r - .marker - .as_ref() - .unwrap() - .evaluate_optional_environment(None, &metadata.provides_extras[..]) + r.marker.is_none() || r.marker.as_ref().unwrap().evaluate(marker_environment, &[]) }) .collect::>(); } @@ -116,6 +121,9 @@ struct DisplayDependencyGraph<'a> { // Whether to de-duplicate the displayed dependencies. no_dedupe: bool, + + // The marker environment for the current interpreter. + marker_environment: &'a MarkerEnvironment, } impl<'a> DisplayDependencyGraph<'a> { @@ -125,6 +133,7 @@ impl<'a> DisplayDependencyGraph<'a> { depth: usize, prune: Vec, no_dedupe: bool, + marker_environment: &'a MarkerEnvironment, ) -> DisplayDependencyGraph<'a> { let mut dist_by_package_name = HashMap::new(); let mut required_packages = HashSet::new(); @@ -132,7 +141,7 @@ impl<'a> DisplayDependencyGraph<'a> { dist_by_package_name.insert(site_package.name(), site_package); } for site_package in site_packages.iter() { - for required in required_with_no_extra(site_package) { + for required in required_with_no_extra(site_package, marker_environment) { required_packages.insert(required.name.clone()); } } @@ -144,6 +153,7 @@ impl<'a> DisplayDependencyGraph<'a> { depth, prune, no_dedupe, + marker_environment, } } @@ -182,7 +192,7 @@ impl<'a> DisplayDependencyGraph<'a> { path.push(package_name.clone()); visited.insert(package_name.clone()); - let required_packages = required_with_no_extra(installed_dist); + let required_packages = required_with_no_extra(installed_dist, self.marker_environment); for (index, required_package) in required_packages.iter().enumerate() { // Skip if the current package is not one of the installed distributions. if !self diff --git a/crates/uv/tests/pip_tree.rs b/crates/uv/tests/pip_tree.rs index 029f9f0e4..fdaea3f87 100644 --- a/crates/uv/tests/pip_tree.rs +++ b/crates/uv/tests/pip_tree.rs @@ -75,6 +75,54 @@ fn single_package() { ); } +// `pandas` requires `numpy` with markers on Python version. +#[test] +#[cfg(not(windows))] +fn python_version_marker() { + let context = TestContext::new("3.12"); + + let requirements_txt = context.temp_dir.child("requirements.txt"); + requirements_txt.write_str("pandas==2.2.1").unwrap(); + + uv_snapshot!(context + .pip_install() + .arg("-r") + .arg("requirements.txt") + .arg("--strict"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 6 packages in [TIME] + Prepared 6 packages in [TIME] + Installed 6 packages in [TIME] + + numpy==1.26.4 + + pandas==2.2.1 + + python-dateutil==2.9.0.post0 + + pytz==2024.1 + + six==1.16.0 + + tzdata==2024.1 + + "### + ); + + uv_snapshot!(context.filters(), tree_command(&context), @r###" + success: true + exit_code: 0 + ----- stdout ----- + pandas v2.2.1 + ├── numpy v1.26.4 + ├── python-dateutil v2.9.0.post0 + │ └── six v1.16.0 + ├── pytz v2024.1 + └── tzdata v2024.1 + + ----- stderr ----- + "### + ); +} + #[test] fn nested_dependencies() { let context = TestContext::new("3.12"); @@ -677,7 +725,7 @@ fn multiple_packages() { let mut filters = context.filters(); if cfg!(windows) { - filters.push(("colorama v0.4.6\n", "")); + filters.push(("└── colorama v0.4.6\n", "")); } context.assert_command("import requests").success(); uv_snapshot!(filters, tree_command(&context), @r###" @@ -746,17 +794,17 @@ fn multiple_packages_shared_descendant() { boto3 v1.34.69 ├── botocore v1.34.69 │ ├── jmespath v1.0.1 - │ └── python-dateutil v2.9.0.post0 - │ └── six v1.16.0 + │ ├── python-dateutil v2.9.0.post0 + │ │ └── six v1.16.0 + │ └── urllib3 v2.2.1 ├── jmespath v1.0.1 (*) └── s3transfer v0.10.1 └── botocore v1.34.69 (*) pendulum v3.0.0 ├── python-dateutil v2.9.0.post0 (*) - └── tzdata v2024.1 - time-machine v2.14.1 - └── python-dateutil v2.9.0.post0 (*) - urllib3 v2.2.1 + ├── tzdata v2024.1 + └── time-machine v2.14.1 + └── python-dateutil v2.9.0.post0 (*) (*) Package tree already displayed ----- stderr ----- @@ -838,22 +886,23 @@ fn no_dedupe_and_cycle() { boto3 v1.34.69 ├── botocore v1.34.69 │ ├── jmespath v1.0.1 - │ └── python-dateutil v2.9.0.post0 - │ └── six v1.16.0 + │ ├── python-dateutil v2.9.0.post0 + │ │ └── six v1.16.0 + │ └── urllib3 v2.2.1 ├── jmespath v1.0.1 └── s3transfer v0.10.1 └── botocore v1.34.69 ├── jmespath v1.0.1 - └── python-dateutil v2.9.0.post0 - └── six v1.16.0 + ├── python-dateutil v2.9.0.post0 + │ └── six v1.16.0 + └── urllib3 v2.2.1 pendulum v3.0.0 ├── python-dateutil v2.9.0.post0 │ └── six v1.16.0 - └── tzdata v2024.1 - time-machine v2.14.1 - └── python-dateutil v2.9.0.post0 - └── six v1.16.0 - urllib3 v2.2.1 + ├── tzdata v2024.1 + └── time-machine v2.14.1 + └── python-dateutil v2.9.0.post0 + └── six v1.16.0 uv-cyclic-dependencies-c v0.1.0 └── uv-cyclic-dependencies-a v0.1.0 └── uv-cyclic-dependencies-b v0.1.0 @@ -915,22 +964,23 @@ fn no_dedupe() { boto3 v1.34.69 ├── botocore v1.34.69 │ ├── jmespath v1.0.1 - │ └── python-dateutil v2.9.0.post0 - │ └── six v1.16.0 + │ ├── python-dateutil v2.9.0.post0 + │ │ └── six v1.16.0 + │ └── urllib3 v2.2.1 ├── jmespath v1.0.1 └── s3transfer v0.10.1 └── botocore v1.34.69 ├── jmespath v1.0.1 - └── python-dateutil v2.9.0.post0 - └── six v1.16.0 + ├── python-dateutil v2.9.0.post0 + │ └── six v1.16.0 + └── urllib3 v2.2.1 pendulum v3.0.0 ├── python-dateutil v2.9.0.post0 │ └── six v1.16.0 - └── tzdata v2024.1 - time-machine v2.14.1 - └── python-dateutil v2.9.0.post0 - └── six v1.16.0 - urllib3 v2.2.1 + ├── tzdata v2024.1 + └── time-machine v2.14.1 + └── python-dateutil v2.9.0.post0 + └── six v1.16.0 ----- stderr ----- "###