feat: total ordering for the dependency provider (#892)

This should fix the total ordering issues by rewriting how ordering works for the case where the name, version, build number are the same, lets call this set of candidates X. The old method violated total ordering, with specifically that though `a > b`, ` b > c`, actually `a < c` which was incorrect. The new method compares the dependencies alphabetically and early-outs on the first version that wins, this seems to work consistently enough. But we shouldn't rely on this behavior. fixes: #888 --------- Co-authored-by: Bas Zalmstra <[email protected]>
conda · Oct 7, 2024 · 6032945 · 6032945
1 parent bc48aaa
commit 6032945
Show file tree

Hide file tree

Showing 15 changed files with 583 additions and 535 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -61,7 +61,7 @@ digest = "0.10.7"
 dirs = "5.0.1"
 dunce = "1.0.4"
 enum_dispatch = "0.3.13"
-fs-err = {  version = "2.11.0", features = ["tokio"] }
+fs-err = { version = "2.11.0", features = ["tokio"] }
 fslock = "0.2.1"
 futures = "0.3.30"
 futures-util = "0.3.30"
@@ -110,7 +110,7 @@ regex = "1.10.4"
 reqwest = { version = "0.12.3", default-features = false }
 reqwest-middleware = "0.3.0"
 reqwest-retry = "0.6.0"
-resolvo = { version = "0.8.1" }
+resolvo = { version = "0.8.2" }
 retry-policies = { version = "0.4.0", default-features = false }
 rmp-serde = { version = "1.2.0" }
 rstest = { version = "0.21.0" }
@@ -150,7 +150,7 @@ tracing = "0.1.40"
 tracing-subscriber = { version = "0.3.18", default-features = false }
 tracing-test = { version = "0.2.4" }
 trybuild = { version = "1.0.91" }
-typed-path = { version = "0.9.0" }
+typed-path = { version = "0.9.2" }
 url = { version = "2.5.0" }
 uuid = { version = "1.8.0", default-features = false }
 walkdir = "2.5.0"

diff --git a/crates/rattler_conda_types/src/utils/serde.rs b/crates/rattler_conda_types/src/utils/serde.rs
@@ -1,14 +1,10 @@
 use chrono::{DateTime, Utc};
 use fxhash::FxHashMap;
-use itertools::Itertools;
 use serde::de::Error as _;
 use serde::ser::Error;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
-use serde_with::de::DeserializeAsWrap;
-use serde_with::ser::SerializeAsWrap;
 use serde_with::{DeserializeAs, SerializeAs};
-use std::collections::{BTreeMap, HashSet};
-use std::hash::{BuildHasher, Hash};
+use std::collections::BTreeMap;
 use std::marker::PhantomData;
 use std::path::{Path, PathBuf};
 use url::Url;
@@ -143,62 +139,6 @@ impl SerializeAs<chrono::DateTime<chrono::Utc>> for Timestamp {
     }
 }
 
-/// Used with `serde_with` to serialize a collection as a sorted collection.
-#[derive(Default)]
-pub(crate) struct Ordered<T>(PhantomData<T>);
-
-impl<'de, T: Eq + Hash, S: BuildHasher + Default, TAs> DeserializeAs<'de, HashSet<T, S>>
-    for Ordered<TAs>
-where
-    TAs: DeserializeAs<'de, T>,
-{
-    fn deserialize_as<D>(deserializer: D) -> Result<HashSet<T, S>, D::Error>
-    where
-        D: Deserializer<'de>,
-    {
-        let content =
-            DeserializeAsWrap::<Vec<T>, Vec<TAs>>::deserialize(deserializer)?.into_inner();
-        Ok(content.into_iter().collect())
-    }
-}
-
-impl<T: Ord, HS, TAs: SerializeAs<T>> SerializeAs<HashSet<T, HS>> for Ordered<TAs> {
-    fn serialize_as<S>(source: &HashSet<T, HS>, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        let mut elements = source.iter().collect_vec();
-        elements.sort();
-        SerializeAsWrap::<Vec<&T>, Vec<&TAs>>::new(&elements).serialize(serializer)
-    }
-}
-
-impl<'de, T: Ord, TAs> DeserializeAs<'de, Vec<T>> for Ordered<TAs>
-where
-    TAs: DeserializeAs<'de, T>,
-{
-    fn deserialize_as<D>(deserializer: D) -> Result<Vec<T>, D::Error>
-    where
-        D: Deserializer<'de>,
-    {
-        let mut content =
-            DeserializeAsWrap::<Vec<T>, Vec<TAs>>::deserialize(deserializer)?.into_inner();
-        content.sort();
-        Ok(content)
-    }
-}
-
-impl<T: Ord, TAs: SerializeAs<T>> SerializeAs<Vec<T>> for Ordered<TAs> {
-    fn serialize_as<S>(source: &Vec<T>, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        let mut elements = source.iter().collect_vec();
-        elements.sort();
-        SerializeAsWrap::<Vec<&T>, Vec<&TAs>>::new(&elements).serialize(serializer)
-    }
-}
-
 /// A helper struct to deserialize types from a string without checking the string.
 pub struct DeserializeFromStrUnchecked;
 

diff --git a/crates/rattler_conda_types/src/version/mod.rs b/crates/rattler_conda_types/src/version/mod.rs
@@ -1037,7 +1037,7 @@ mod test {
 
     use crate::version::StrictVersion;
 
-    use super::Version;
+    use super::{Component, Version};
 
     // Tests are inspired by: https://github.com/conda/conda/blob/33a142c16530fcdada6c377486f1c1a385738a96/tests/models/test_version.py
 
@@ -1049,7 +1049,7 @@ mod test {
             Restart,
         }
 
-        let versions = [
+        let versions_str = [
             "   0.4",
             "== 0.4.0",
             " < 0.4.1.rc",
@@ -1079,13 +1079,13 @@ mod test {
             " < 2!0.4.1", // epoch increased again
         ];
 
-        let ops = versions.iter().map(|&v| {
-            let (op, version) = if let Some((op, version)) = v.trim().split_once(' ') {
+        let ops = versions_str.iter().map(|&v| {
+            let (op, version_str) = if let Some((op, version)) = v.trim().split_once(' ') {
                 (op, version.trim())
             } else {
                 ("", v.trim())
             };
-            let version: Version = version.parse().unwrap();
+            let version: Version = version_str.parse().unwrap();
             let op = match op {
                 "<" => CmpOp::Less,
                 "==" => CmpOp::Equal,
@@ -1397,4 +1397,51 @@ mod test {
             expected
         );
     }
+
+    #[test]
+    fn test_component_total_order() {
+        // Create instances of each variant
+        let components = vec![
+            Component::Dev,
+            Component::UnderscoreOrDash { is_dash: false },
+            Component::Iden(Box::from("alpha")),
+            Component::Iden(Box::from("beta")),
+            Component::Numeral(1),
+            Component::Numeral(2),
+            Component::Post,
+        ];
+
+        // Check that each component equals itself
+        for a in &components {
+            assert_eq!(a.cmp(a), Ordering::Equal);
+        }
+
+        for (i, a) in components.iter().enumerate() {
+            for b in components[i + 1..].iter() {
+                let ord = a.cmp(b);
+                assert_eq!(
+                    ord,
+                    Ordering::Less,
+                    "Expected {:?} < {:?}, but found {:?}",
+                    a,
+                    b,
+                    ord
+                );
+            }
+            // Check the reverse ordering as well
+            // I think this should automatically check transitivity
+            // If a <= b and b <= c, then a <= c
+            for b in components[..i].iter() {
+                let ord = a.cmp(b);
+                assert_eq!(
+                    ord,
+                    Ordering::Greater,
+                    "Expected {:?} > {:?}, but found {:?}",
+                    a,
+                    b,
+                    ord
+                );
+            }
+        }
+    }
 }