Handle cycles when propagating markers (#4595)

## Summary

It turns out that `Topo` only works on graphs without cycles. If a graph
has a cycle, it seems to bail early. So we were losing markers for trees
that contain cycles (like Poetry, which depends on
`poetry-plugin-export`, which depends on Poetry).

Now, we remove cycles beforehand and re-add those edges afterwards.

It's a bit hard for me to reason about the implications of this. The way
that marker propagation works is that we do visit the nodes in-order and
propagate the markers from any incoming to any outgoing edges. We only
do this at a single depth (rather than recursively) because we visit the
nodes in-order anyway. But if you have a cycle... then in theory you
might need to propagate the markers recursively? Or maybe not?

As an example:

`A -> B -> C -> D -> B`

If `A -> B` has `sys_platform == 'darwin'`, and then `D -> B` has
`python_version >= '3.7`... then we don't need to propagate
`python_version >= '3.7'` back to `B` or any of its dependencies,
because the condition would be `(sys_platform == 'darwin' or
python_version >= '3.7) or sys_platform == 'darwin'`, which is
equivalent to `sys_platform == 'darwin'`.

Closes #4584.

crates/uv-resolver/src/resolution/display.rs

@@ -1,6 +1,7 @@
 use std::collections::BTreeSet;
 
 use owo_colors::OwoColorize;
+use petgraph::algo::greedy_feedback_arc_set;
 use petgraph::visit::{EdgeRef, Topo};
 use petgraph::Direction;
 use rustc_hash::{FxBuildHasher, FxHashMap};
@@ -348,6 +349,28 @@ fn to_requirements_txt_graph(graph: &ResolutionPetGraph) -> IntermediatePetGraph
 /// The graph is directed, so if any edge contains a marker, we need to propagate it to all
 /// downstream nodes.
 fn propagate_markers(mut graph: IntermediatePetGraph) -> IntermediatePetGraph {
+    // Remove any cycles. By absorption, it should be fine to ignore cycles.
+    //
+    // Imagine a graph: `A -> B -> C -> A`. Assume that `A` has weight `1`, `B` has weight `2`,
+    // and `C` has weight `3`. The weights are the marker trees.
+    //
+    // When propagating, we'd return to `A` when we hit the cycle, to create `1 or (1 and 2 and 3)`,
+    // which resolves to `1`.
+    //
+    // TODO(charlie): The above reasoning could be incorrect. Consider using a graph algorithm that
+    // can handle weight propagation with cycles.
+    let edges = {
+        let fas = greedy_feedback_arc_set(&graph)
+            .map(|edge| edge.id())
+            .collect::<Vec<_>>();
+        let mut edges = Vec::with_capacity(fas.len());
+        for edge_id in fas {
+            edges.push(graph.edge_endpoints(edge_id).unwrap());
+            graph.remove_edge(edge_id);
+        }
+        edges
+    };
+
     let mut topo = Topo::new(&graph);
     while let Some(index) = topo.next(&graph) {
         let marker_tree: Option<MarkerTree> = {
@@ -386,6 +409,12 @@ fn propagate_markers(mut graph: IntermediatePetGraph) -> IntermediatePetGraph {
         };
     }
 
+    // Re-add the removed edges. We no longer care about the edge _weights_, but we do want the
+    // edges to be present, to power the `# via` annotations.
+    for (source, target) in edges {
+        graph.add_edge(source, target, None);
+    }
+
     graph
 }
 

crates/uv/tests/pip_compile.rs

@@ -6300,7 +6300,7 @@ fn universal() -> Result<()> {
         trio ; sys_platform == 'win32'
     "})?;
 
-    uv_snapshot!(context.pip_compile()
+    uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile()
             .arg("requirements.in")
             .arg("--universal"), @r###"
     success: true
@@ -6345,7 +6345,7 @@ fn universal_conflicting() -> Result<()> {
         trio==0.10.0 ; sys_platform == 'win32'
     "})?;
 
-    uv_snapshot!(context.pip_compile()
+    uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile()
             .arg("requirements.in")
             .arg("--universal"), @r###"
     success: true
@@ -6384,6 +6384,137 @@ fn universal_conflicting() -> Result<()> {
     Ok(())
 }
 
+/// Perform a universal resolution with a package that contains cycles in its dependency graph.
+#[test]
+fn universal_cycles() -> Result<()> {
+    let context = TestContext::new("3.12");
+    let requirements_in = context.temp_dir.child("requirements.in");
+    requirements_in.write_str(indoc::indoc! {r"
+        poetry
+    "})?;
+
+    uv_snapshot!(context.filters(), windows_filters=false, context.pip_compile()
+            .arg("requirements.in")
+            .arg("--universal"), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+    # This file was autogenerated by uv via the following command:
+    #    uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal
+    build==1.1.1
+        # via poetry
+    cachecontrol==0.14.0
+        # via poetry
+    certifi==2024.2.2
+        # via requests
+    cffi==1.16.0 ; sys_platform == 'darwin' or (platform_python_implementation != 'PyPy' and sys_platform == 'linux')
+        # via
+        #   cryptography
+        #   xattr
+    charset-normalizer==3.3.2
+        # via requests
+    cleo==2.1.0
+        # via poetry
+    colorama==0.4.6 ; os_name == 'nt'
+        # via build
+    crashtest==0.4.1
+        # via
+        #   cleo
+        #   poetry
+    cryptography==42.0.5 ; sys_platform == 'linux'
+        # via secretstorage
+    distlib==0.3.8
+        # via virtualenv
+    dulwich==0.21.7
+        # via poetry
+    fastjsonschema==2.19.1
+        # via poetry
+    filelock==3.13.1
+        # via
+        #   cachecontrol
+        #   virtualenv
+    idna==3.6
+        # via requests
+    installer==0.7.0
+        # via poetry
+    jaraco-classes==3.3.1
+        # via keyring
+    jeepney==0.8.0 ; sys_platform == 'linux'
+        # via
+        #   keyring
+        #   secretstorage
+    keyring==24.3.1
+        # via poetry
+    more-itertools==10.2.0
+        # via jaraco-classes
+    msgpack==1.0.8
+        # via cachecontrol
+    packaging==24.0
+        # via
+        #   build
+        #   poetry
+    pexpect==4.9.0
+        # via poetry
+    pkginfo==1.10.0
+        # via poetry
+    platformdirs==4.2.0
+        # via
+        #   poetry
+        #   virtualenv
+    poetry==1.8.2
+        # via
+        #   -r requirements.in
+        #   poetry-plugin-export
+    poetry-core==1.9.0
+        # via
+        #   poetry
+        #   poetry-plugin-export
+    poetry-plugin-export==1.7.1
+        # via poetry
+    ptyprocess==0.7.0
+        # via pexpect
+    pycparser==2.21 ; sys_platform == 'darwin' or (platform_python_implementation != 'PyPy' and sys_platform == 'linux')
+        # via cffi
+    pyproject-hooks==1.0.0
+        # via
+        #   build
+        #   poetry
+    pywin32-ctypes==0.2.2 ; sys_platform == 'win32'
+        # via keyring
+    rapidfuzz==3.7.0
+        # via cleo
+    requests==2.31.0
+        # via
+        #   cachecontrol
+        #   poetry
+        #   requests-toolbelt
+    requests-toolbelt==1.0.0
+        # via poetry
+    secretstorage==3.3.3 ; sys_platform == 'linux'
+        # via keyring
+    shellingham==1.5.4
+        # via poetry
+    tomlkit==0.12.4
+        # via poetry
+    trove-classifiers==2024.3.3
+        # via poetry
+    urllib3==2.2.1
+        # via
+        #   dulwich
+        #   requests
+    virtualenv==20.25.1
+        # via poetry
+    xattr==1.1.0 ; sys_platform == 'darwin'
+        # via poetry
+
+    ----- stderr -----
+    Resolved 41 packages in [TIME]
+    "###
+    );
+
+    Ok(())
+}
+
 /// Resolve a package from a `requirements.in` file, with a `constraints.txt` file pinning one of
 /// its transitive dependencies to a specific version.
 #[test]