Skip to content

Commit cae1b17

Browse files
charliermarshcharliermarsh
committed
Fix dangling non-platform extras in uv tree
1 parent 98523e2 commit cae1b17

File tree

2 files changed

+165
-120
lines changed

2 files changed

+165
-120
lines changed

‎crates/uv-resolver/src/lock/tree.rs‎

Lines changed: 97 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,6 @@ impl<'env> TreeDisplay<'env> {
4949
}
5050

5151
for dependency in &package.dependencies {
52-
// Skip dependencies that don't apply to the current environment.
53-
if let Some(environment_markers) = markers {
54-
if !dependency
55-
.complexified_marker
56-
.evaluate(environment_markers, &[])
57-
{
58-
continue;
59-
}
60-
}
61-
6252
// Insert the package into the graph.
6353
let package_node = if let Some(index) = inverse.get(&package.id) {
6454
*index
@@ -78,38 +68,15 @@ impl<'env> TreeDisplay<'env> {
7868
};
7969

8070
// Add an edge between the package and the dependency.
81-
if invert {
82-
graph.add_edge(
83-
dependency_node,
84-
package_node,
85-
Edge::Prod(Cow::Owned(Dependency {
86-
package_id: package.id.clone(),
87-
extra: dependency.extra.clone(),
88-
simplified_marker: dependency.simplified_marker.clone(),
89-
complexified_marker: dependency.complexified_marker.clone(),
90-
})),
91-
);
92-
} else {
93-
graph.add_edge(
94-
package_node,
95-
dependency_node,
96-
Edge::Prod(Cow::Borrowed(dependency)),
97-
);
98-
}
71+
graph.add_edge(
72+
package_node,
73+
dependency_node,
74+
Edge::Prod(Cow::Borrowed(dependency)),
75+
);
9976
}
10077

10178
for (extra, dependencies) in &package.optional_dependencies {
10279
for dependency in dependencies {
103-
// Skip dependencies that don't apply to the current environment.
104-
if let Some(environment_markers) = markers {
105-
if !dependency
106-
.complexified_marker
107-
.evaluate(environment_markers, &[])
108-
{
109-
continue;
110-
}
111-
}
112-
11380
// Insert the package into the graph.
11481
let package_node = if let Some(index) = inverse.get(&package.id) {
11582
*index
@@ -129,42 +96,16 @@ impl<'env> TreeDisplay<'env> {
12996
};
13097

13198
// Add an edge between the package and the dependency.
132-
if invert {
133-
graph.add_edge(
134-
dependency_node,
135-
package_node,
136-
Edge::Optional(
137-
extra,
138-
Cow::Owned(Dependency {
139-
package_id: package.id.clone(),
140-
extra: dependency.extra.clone(),
141-
simplified_marker: dependency.simplified_marker.clone(),
142-
complexified_marker: dependency.complexified_marker.clone(),
143-
}),
144-
),
145-
);
146-
} else {
147-
graph.add_edge(
148-
package_node,
149-
dependency_node,
150-
Edge::Optional(extra, Cow::Borrowed(dependency)),
151-
);
152-
}
99+
graph.add_edge(
100+
package_node,
101+
dependency_node,
102+
Edge::Optional(extra, Cow::Borrowed(dependency)),
103+
);
153104
}
154105
}
155106

156107
for (group, dependencies) in &package.dev_dependencies {
157108
for dependency in dependencies {
158-
// Skip dependencies that don't apply to the current environment.
159-
if let Some(environment_markers) = markers {
160-
if !dependency
161-
.complexified_marker
162-
.evaluate(environment_markers, &[])
163-
{
164-
continue;
165-
}
166-
}
167-
168109
// Insert the package into the graph.
169110
let package_node = if let Some(index) = inverse.get(&package.id) {
170111
*index
@@ -184,56 +125,53 @@ impl<'env> TreeDisplay<'env> {
184125
};
185126

186127
// Add an edge between the package and the dependency.
187-
if invert {
188-
graph.add_edge(
189-
dependency_node,
190-
package_node,
191-
Edge::Dev(
192-
group,
193-
Cow::Owned(Dependency {
194-
package_id: package.id.clone(),
195-
extra: dependency.extra.clone(),
196-
simplified_marker: dependency.simplified_marker.clone(),
197-
complexified_marker: dependency.complexified_marker.clone(),
198-
}),
199-
),
200-
);
201-
} else {
202-
graph.add_edge(
203-
package_node,
204-
dependency_node,
205-
Edge::Dev(group, Cow::Borrowed(dependency)),
206-
);
207-
}
128+
graph.add_edge(
129+
package_node,
130+
dependency_node,
131+
Edge::Dev(group, Cow::Borrowed(dependency)),
132+
);
208133
}
209134
}
210135
}
211136

212137
let mut modified = false;
213138

214-
// Filter the graph to those nodes reachable from the root nodes.
215-
if !packages.is_empty() {
139+
// Step 1: Filter out packages that aren't reachable on this platform.
140+
if let Some(environment_markers) = markers {
216141
let mut reachable = FxHashSet::default();
217142

218-
// Perform a DFS from the root nodes to find the reachable nodes.
219-
let mut dfs = Dfs {
220-
stack: graph
221-
.node_indices()
222-
.filter(|index| packages.contains(&graph[*index].name))
223-
.collect::<Vec<_>>(),
224-
..Dfs::empty(&graph)
225-
};
226-
while let Some(node) = dfs.next(&graph) {
143+
// Perform a DFS from the root nodes to find the reachable nodes, following only the
144+
// production edges.
145+
let mut stack = graph
146+
.node_indices()
147+
.filter(|index| {
148+
graph
149+
.edges_directed(*index, Direction::Incoming)
150+
.next()
151+
.is_none()
152+
})
153+
.collect::<VecDeque<_>>();
154+
while let Some(node) = stack.pop_front() {
227155
reachable.insert(node);
156+
for edge in graph.edges_directed(node, Direction::Outgoing) {
157+
if edge
158+
.weight()
159+
.dependency()
160+
.complexified_marker
161+
.evaluate(environment_markers, &[])
162+
{
163+
stack.push_back(edge.target());
164+
}
165+
}
228166
}
229167

230168
// Remove the unreachable nodes from the graph.
231169
graph.retain_nodes(|_, index| reachable.contains(&index));
232170
modified = true;
233171
}
234172

235-
// Filter the graph to those that are reachable from production nodes, if `--no-dev` or
236-
// `--only-dev` was specified.
173+
// Step 2: Filter the graph to those that are reachable in production or development, if
174+
// `--no-dev` or `--only-dev` were specified, respectively.
237175
if dev != DevMode::Include {
238176
let mut reachable = FxHashSet::default();
239177

@@ -268,6 +206,33 @@ impl<'env> TreeDisplay<'env> {
268206
modified = true;
269207
}
270208

209+
// Step 3: Reverse the graph.
210+
if invert {
211+
graph.reverse();
212+
modified = true;
213+
}
214+
215+
// Step 4: Filter the graph to those nodes reachable from the target packages.
216+
if !packages.is_empty() {
217+
let mut reachable = FxHashSet::default();
218+
219+
// Perform a DFS from the root nodes to find the reachable nodes.
220+
let mut dfs = Dfs {
221+
stack: graph
222+
.node_indices()
223+
.filter(|index| packages.contains(&graph[*index].name))
224+
.collect::<Vec<_>>(),
225+
..Dfs::empty(&graph)
226+
};
227+
while let Some(node) = dfs.next(&graph) {
228+
reachable.insert(node);
229+
}
230+
231+
// Remove the unreachable nodes from the graph.
232+
graph.retain_nodes(|_, index| reachable.contains(&index));
233+
modified = true;
234+
}
235+
271236
// If the graph was modified, re-create the inverse map.
272237
if modified {
273238
inverse.clear();
@@ -307,9 +272,9 @@ impl<'env> TreeDisplay<'env> {
307272

308273
match node {
309274
Node::Root(_) => line,
310-
Node::Dependency(_) => line,
311-
Node::OptionalDependency(extra, _) => format!("{line} (extra: {extra})"),
312-
Node::DevDependency(group, _) => format!("{line} (group: {group})"),
275+
Node::Dependency(_, _) => line,
276+
Node::OptionalDependency(extra, _, _) => format!("{line} (extra: {extra})"),
277+
Node::DevDependency(group, _, _) => format!("{line} (group: {group})"),
313278
}
314279
};
315280

@@ -330,9 +295,13 @@ impl<'env> TreeDisplay<'env> {
330295
.graph
331296
.edges_directed(self.inverse[node.package_id()], Direction::Outgoing)
332297
.map(|edge| match edge.weight() {
333-
Edge::Prod(dependency) => Node::Dependency(dependency),
334-
Edge::Optional(extra, dependency) => Node::OptionalDependency(extra, dependency),
335-
Edge::Dev(group, dependency) => Node::DevDependency(group, dependency),
298+
Edge::Prod(dependency) => Node::Dependency(self.graph[edge.target()], dependency),
299+
Edge::Optional(extra, dependency) => {
300+
Node::OptionalDependency(self.graph[edge.target()], dependency, extra)
301+
}
302+
Edge::Dev(group, dependency) => {
303+
Node::DevDependency(self.graph[edge.target()], dependency, group)
304+
}
336305
})
337306
.collect::<Vec<_>>();
338307
dependencies.sort_unstable();
@@ -424,30 +393,40 @@ enum Edge<'env> {
424393
Dev(&'env GroupName, Cow<'env, Dependency>),
425394
}
426395

396+
impl<'env> Edge<'env> {
397+
fn dependency(&self) -> &Dependency {
398+
match self {
399+
Self::Prod(dependency) => dependency,
400+
Self::Optional(_, dependency) => dependency,
401+
Self::Dev(_, dependency) => dependency,
402+
}
403+
}
404+
}
405+
427406
#[derive(Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd)]
428407
enum Node<'env> {
429408
Root(&'env PackageId),
430-
Dependency(&'env Dependency),
431-
OptionalDependency(&'env ExtraName, &'env Dependency),
432-
DevDependency(&'env GroupName, &'env Dependency),
409+
Dependency(&'env PackageId, &'env Dependency),
410+
OptionalDependency(&'env PackageId, &'env Dependency, &'env ExtraName),
411+
DevDependency(&'env PackageId, &'env Dependency, &'env GroupName),
433412
}
434413

435414
impl<'env> Node<'env> {
436415
fn package_id(&self) -> &'env PackageId {
437416
match self {
438417
Self::Root(id) => id,
439-
Self::Dependency(dep) => &dep.package_id,
440-
Self::OptionalDependency(_, dep) => &dep.package_id,
441-
Self::DevDependency(_, dep) => &dep.package_id,
418+
Self::Dependency(id, _) => id,
419+
Self::OptionalDependency(id, _, _) => id,
420+
Self::DevDependency(id, _, _) => id,
442421
}
443422
}
444423

445424
fn extras(&self) -> Option<&BTreeSet<ExtraName>> {
446425
match self {
447426
Self::Root(_) => None,
448-
Self::Dependency(dep) => Some(&dep.extra),
449-
Self::OptionalDependency(_, dep) => Some(&dep.extra),
450-
Self::DevDependency(_, dep) => Some(&dep.extra),
427+
Self::Dependency(_, dep) => Some(&dep.extra),
428+
Self::OptionalDependency(_, dep, _) => Some(&dep.extra),
429+
Self::DevDependency(_, dep, _) => Some(&dep.extra),
451430
}
452431
}
453432
}

‎crates/uv/tests/it/tree.rs‎

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,73 @@ fn nested_dependencies() -> Result<()> {
4545
Ok(())
4646
}
4747

48+
#[test]
49+
fn nested_platform_dependencies() -> Result<()> {
50+
let context = TestContext::new("3.12");
51+
52+
let pyproject_toml = context.temp_dir.child("pyproject.toml");
53+
pyproject_toml.write_str(
54+
r#"
55+
[project]
56+
name = "project"
57+
version = "0.1.0"
58+
requires-python = ">=3.12"
59+
dependencies = [
60+
"jupyter-client"
61+
]
62+
"#,
63+
)?;
64+
65+
uv_snapshot!(context.filters(), context.tree().arg("--python-platform").arg("linux"), @r###"
66+
success: true
67+
exit_code: 0
68+
----- stdout -----
69+
project v0.1.0
70+
└── jupyter-client v8.6.1
71+
├── jupyter-core v5.7.2
72+
│ ├── platformdirs v4.2.0
73+
│ └── traitlets v5.14.2
74+
├── python-dateutil v2.9.0.post0
75+
│ └── six v1.16.0
76+
├── pyzmq v25.1.2
77+
├── tornado v6.4
78+
└── traitlets v5.14.2
79+
80+
----- stderr -----
81+
Resolved 12 packages in [TIME]
82+
"###
83+
);
84+
85+
uv_snapshot!(context.filters(), context.tree().arg("--universal"), @r###"
86+
success: true
87+
exit_code: 0
88+
----- stdout -----
89+
project v0.1.0
90+
└── jupyter-client v8.6.1
91+
├── jupyter-core v5.7.2
92+
│ ├── platformdirs v4.2.0
93+
│ ├── pywin32 v306
94+
│ └── traitlets v5.14.2
95+
├── python-dateutil v2.9.0.post0
96+
│ └── six v1.16.0
97+
├── pyzmq v25.1.2
98+
│ └── cffi v1.16.0
99+
│ └── pycparser v2.21
100+
├── tornado v6.4
101+
└── traitlets v5.14.2
102+
103+
----- stderr -----
104+
Resolved 12 packages in [TIME]
105+
"###
106+
);
107+
108+
// `uv tree` should update the lockfile
109+
let lock = context.read("uv.lock");
110+
assert!(!lock.is_empty());
111+
112+
Ok(())
113+
}
114+
48115
#[test]
49116
fn invert() -> Result<()> {
50117
let context = TestContext::new("3.12");
@@ -267,8 +334,7 @@ fn platform_dependencies_inverted() -> Result<()> {
267334
)?;
268335

269336
// When `--universal` is _not_ provided, `colorama` should _not_ be included.
270-
#[cfg(not(windows))]
271-
uv_snapshot!(context.filters(), context.tree().arg("--invert"), @r###"
337+
uv_snapshot!(context.filters(), context.tree().arg("--invert").arg("--python-platform").arg("linux"), @r###"
272338
success: true
273339
exit_code: 0
274340
----- stdout -----

0 commit comments

Comments
 (0)