Skip to content

Commit

Permalink
Fix dangling non-platform extras in uv tree
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Oct 24, 2024
1 parent 98523e2 commit a245799
Show file tree
Hide file tree
Showing 2 changed files with 165 additions and 120 deletions.
215 changes: 97 additions & 118 deletions crates/uv-resolver/src/lock/tree.rs
View file
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
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

0 comments on commit a245799

Please sign in to comment.