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.

Sign up for GitHub

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

Fix dangling non-platform dependencies in uv tree #8532

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Fix dangling non-platform dependencies in uv tree #8532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
215 changes: 97 additions & 118 deletions crates/uv-resolver/src/lock/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -184,56 +125,53 @@ 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::<Vec<_>>(),
..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::<VecDeque<_>>();
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.
graph.retain_nodes(|_, index| reachable.contains(&index));
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();

Expand Down Expand Up @@ -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::<Vec<_>>(),
..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();
Expand Down Expand Up @@ -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})"),
}
};

Expand All @@ -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::<Vec<_>>();
dependencies.sort_unstable();
Expand Down Expand Up @@ -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<ExtraName>> {
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),
}
}
}
Expand Down
70 changes: 68 additions & 2 deletions crates/uv/tests/it/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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 -----
Expand Down
Loading