From cae1b17c1c4e170b400e904aa0c9fb84185c8966 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 24 Oct 2024 12:06:39 -0400 Subject: [PATCH] Fix dangling non-platform extras in uv tree --- crates/uv-resolver/src/lock/tree.rs | 215 +++++++++++++--------------- crates/uv/tests/it/tree.rs | 70 ++++++++- 2 files changed, 165 insertions(+), 120 deletions(-) diff --git a/crates/uv-resolver/src/lock/tree.rs b/crates/uv-resolver/src/lock/tree.rs index 066584dd87ed1..05a3efc7cc26a 100644 --- a/crates/uv-resolver/src/lock/tree.rs +++ b/crates/uv-resolver/src/lock/tree.rs @@ -49,16 +49,6 @@ impl<'env> TreeDisplay<'env> { } for dependency in &package.dependencies { - // Skip dependencies that don't apply to the current environment. - if let Some(environment_markers) = markers { - if !dependency - .complexified_marker - .evaluate(environment_markers, &[]) - { - continue; - } - } - // Insert the package into the graph. let package_node = if let Some(index) = inverse.get(&package.id) { *index @@ -78,38 +68,15 @@ impl<'env> TreeDisplay<'env> { }; // Add an edge between the package and the dependency. - if invert { - graph.add_edge( - dependency_node, - package_node, - Edge::Prod(Cow::Owned(Dependency { - package_id: package.id.clone(), - extra: dependency.extra.clone(), - simplified_marker: dependency.simplified_marker.clone(), - complexified_marker: dependency.complexified_marker.clone(), - })), - ); - } else { - graph.add_edge( - package_node, - dependency_node, - Edge::Prod(Cow::Borrowed(dependency)), - ); - } + graph.add_edge( + package_node, + dependency_node, + Edge::Prod(Cow::Borrowed(dependency)), + ); } for (extra, dependencies) in &package.optional_dependencies { for dependency in dependencies { - // Skip dependencies that don't apply to the current environment. - if let Some(environment_markers) = markers { - if !dependency - .complexified_marker - .evaluate(environment_markers, &[]) - { - continue; - } - } - // Insert the package into the graph. let package_node = if let Some(index) = inverse.get(&package.id) { *index @@ -129,42 +96,16 @@ impl<'env> TreeDisplay<'env> { }; // Add an edge between the package and the dependency. - if invert { - graph.add_edge( - dependency_node, - package_node, - Edge::Optional( - extra, - Cow::Owned(Dependency { - package_id: package.id.clone(), - extra: dependency.extra.clone(), - simplified_marker: dependency.simplified_marker.clone(), - complexified_marker: dependency.complexified_marker.clone(), - }), - ), - ); - } else { - graph.add_edge( - package_node, - dependency_node, - Edge::Optional(extra, Cow::Borrowed(dependency)), - ); - } + graph.add_edge( + package_node, + dependency_node, + Edge::Optional(extra, Cow::Borrowed(dependency)), + ); } } for (group, dependencies) in &package.dev_dependencies { for dependency in dependencies { - // Skip dependencies that don't apply to the current environment. - if let Some(environment_markers) = markers { - if !dependency - .complexified_marker - .evaluate(environment_markers, &[]) - { - continue; - } - } - // Insert the package into the graph. let package_node = if let Some(index) = inverse.get(&package.id) { *index @@ -184,47 +125,44 @@ impl<'env> TreeDisplay<'env> { }; // Add an edge between the package and the dependency. - if invert { - graph.add_edge( - dependency_node, - package_node, - Edge::Dev( - group, - Cow::Owned(Dependency { - package_id: package.id.clone(), - extra: dependency.extra.clone(), - simplified_marker: dependency.simplified_marker.clone(), - complexified_marker: dependency.complexified_marker.clone(), - }), - ), - ); - } else { - graph.add_edge( - package_node, - dependency_node, - Edge::Dev(group, Cow::Borrowed(dependency)), - ); - } + graph.add_edge( + package_node, + dependency_node, + Edge::Dev(group, Cow::Borrowed(dependency)), + ); } } } let mut modified = false; - // Filter the graph to those nodes reachable from the root nodes. - if !packages.is_empty() { + // Step 1: Filter out packages that aren't reachable on this platform. + if let Some(environment_markers) = markers { let mut reachable = FxHashSet::default(); - // Perform a DFS from the root nodes to find the reachable nodes. - let mut dfs = Dfs { - stack: graph - .node_indices() - .filter(|index| packages.contains(&graph[*index].name)) - .collect::>(), - ..Dfs::empty(&graph) - }; - while let Some(node) = dfs.next(&graph) { + // Perform a DFS from the root nodes to find the reachable nodes, following only the + // production edges. + let mut stack = graph + .node_indices() + .filter(|index| { + graph + .edges_directed(*index, Direction::Incoming) + .next() + .is_none() + }) + .collect::>(); + while let Some(node) = stack.pop_front() { reachable.insert(node); + for edge in graph.edges_directed(node, Direction::Outgoing) { + if edge + .weight() + .dependency() + .complexified_marker + .evaluate(environment_markers, &[]) + { + stack.push_back(edge.target()); + } + } } // Remove the unreachable nodes from the graph. @@ -232,8 +170,8 @@ impl<'env> TreeDisplay<'env> { modified = true; } - // Filter the graph to those that are reachable from production nodes, if `--no-dev` or - // `--only-dev` was specified. + // Step 2: Filter the graph to those that are reachable in production or development, if + // `--no-dev` or `--only-dev` were specified, respectively. if dev != DevMode::Include { let mut reachable = FxHashSet::default(); @@ -268,6 +206,33 @@ impl<'env> TreeDisplay<'env> { modified = true; } + // Step 3: Reverse the graph. + if invert { + graph.reverse(); + modified = true; + } + + // Step 4: Filter the graph to those nodes reachable from the target packages. + if !packages.is_empty() { + let mut reachable = FxHashSet::default(); + + // Perform a DFS from the root nodes to find the reachable nodes. + let mut dfs = Dfs { + stack: graph + .node_indices() + .filter(|index| packages.contains(&graph[*index].name)) + .collect::>(), + ..Dfs::empty(&graph) + }; + while let Some(node) = dfs.next(&graph) { + reachable.insert(node); + } + + // Remove the unreachable nodes from the graph. + graph.retain_nodes(|_, index| reachable.contains(&index)); + modified = true; + } + // If the graph was modified, re-create the inverse map. if modified { inverse.clear(); @@ -307,9 +272,9 @@ impl<'env> TreeDisplay<'env> { match node { Node::Root(_) => line, - Node::Dependency(_) => line, - Node::OptionalDependency(extra, _) => format!("{line} (extra: {extra})"), - Node::DevDependency(group, _) => format!("{line} (group: {group})"), + Node::Dependency(_, _) => line, + Node::OptionalDependency(extra, _, _) => format!("{line} (extra: {extra})"), + Node::DevDependency(group, _, _) => format!("{line} (group: {group})"), } }; @@ -330,9 +295,13 @@ impl<'env> TreeDisplay<'env> { .graph .edges_directed(self.inverse[node.package_id()], Direction::Outgoing) .map(|edge| match edge.weight() { - Edge::Prod(dependency) => Node::Dependency(dependency), - Edge::Optional(extra, dependency) => Node::OptionalDependency(extra, dependency), - Edge::Dev(group, dependency) => Node::DevDependency(group, dependency), + Edge::Prod(dependency) => Node::Dependency(self.graph[edge.target()], dependency), + Edge::Optional(extra, dependency) => { + Node::OptionalDependency(self.graph[edge.target()], dependency, extra) + } + Edge::Dev(group, dependency) => { + Node::DevDependency(self.graph[edge.target()], dependency, group) + } }) .collect::>(); dependencies.sort_unstable(); @@ -424,30 +393,40 @@ enum Edge<'env> { Dev(&'env GroupName, Cow<'env, Dependency>), } +impl<'env> Edge<'env> { + fn dependency(&self) -> &Dependency { + match self { + Self::Prod(dependency) => dependency, + Self::Optional(_, dependency) => dependency, + Self::Dev(_, dependency) => dependency, + } + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)] enum Node<'env> { Root(&'env PackageId), - Dependency(&'env Dependency), - OptionalDependency(&'env ExtraName, &'env Dependency), - DevDependency(&'env GroupName, &'env Dependency), + Dependency(&'env PackageId, &'env Dependency), + OptionalDependency(&'env PackageId, &'env Dependency, &'env ExtraName), + DevDependency(&'env PackageId, &'env Dependency, &'env GroupName), } impl<'env> Node<'env> { fn package_id(&self) -> &'env PackageId { match self { Self::Root(id) => id, - Self::Dependency(dep) => &dep.package_id, - Self::OptionalDependency(_, dep) => &dep.package_id, - Self::DevDependency(_, dep) => &dep.package_id, + Self::Dependency(id, _) => id, + Self::OptionalDependency(id, _, _) => id, + Self::DevDependency(id, _, _) => id, } } fn extras(&self) -> Option<&BTreeSet> { match self { Self::Root(_) => None, - Self::Dependency(dep) => Some(&dep.extra), - Self::OptionalDependency(_, dep) => Some(&dep.extra), - Self::DevDependency(_, dep) => Some(&dep.extra), + Self::Dependency(_, dep) => Some(&dep.extra), + Self::OptionalDependency(_, dep, _) => Some(&dep.extra), + Self::DevDependency(_, dep, _) => Some(&dep.extra), } } } diff --git a/crates/uv/tests/it/tree.rs b/crates/uv/tests/it/tree.rs index 6a8b7f3b2cb8f..25091dff50e17 100644 --- a/crates/uv/tests/it/tree.rs +++ b/crates/uv/tests/it/tree.rs @@ -45,6 +45,73 @@ fn nested_dependencies() -> Result<()> { Ok(()) } +#[test] +fn nested_platform_dependencies() -> 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 = [ + "jupyter-client" + ] + "#, + )?; + + uv_snapshot!(context.filters(), context.tree().arg("--python-platform").arg("linux"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + project v0.1.0 + └── jupyter-client v8.6.1 + ├── jupyter-core v5.7.2 + │ ├── platformdirs v4.2.0 + │ └── traitlets v5.14.2 + ├── python-dateutil v2.9.0.post0 + │ └── six v1.16.0 + ├── pyzmq v25.1.2 + ├── tornado v6.4 + └── traitlets v5.14.2 + + ----- stderr ----- + Resolved 12 packages in [TIME] + "### + ); + + uv_snapshot!(context.filters(), context.tree().arg("--universal"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + project v0.1.0 + └── jupyter-client v8.6.1 + ├── jupyter-core v5.7.2 + │ ├── platformdirs v4.2.0 + │ ├── pywin32 v306 + │ └── traitlets v5.14.2 + ├── python-dateutil v2.9.0.post0 + │ └── six v1.16.0 + ├── pyzmq v25.1.2 + │ └── cffi v1.16.0 + │ └── pycparser v2.21 + ├── tornado v6.4 + └── traitlets v5.14.2 + + ----- stderr ----- + Resolved 12 packages in [TIME] + "### + ); + + // `uv tree` should update the lockfile + let lock = context.read("uv.lock"); + assert!(!lock.is_empty()); + + Ok(()) +} + #[test] fn invert() -> Result<()> { let context = TestContext::new("3.12"); @@ -267,8 +334,7 @@ fn platform_dependencies_inverted() -> Result<()> { )?; // When `--universal` is _not_ provided, `colorama` should _not_ be included. - #[cfg(not(windows))] - uv_snapshot!(context.filters(), context.tree().arg("--invert"), @r###" + uv_snapshot!(context.filters(), context.tree().arg("--invert").arg("--python-platform").arg("linux"), @r###" success: true exit_code: 0 ----- stdout -----