fix: roundtrip of arch/platform in lock files (#1124)

Author: wolfv

crates/rattler_conda_types/src/repo_data/mod.rs

@@ -26,7 +26,7 @@ use crate::{
         serde::{sort_map_alphabetically, DeserializeFromStrUnchecked},
         UrlWithTrailingSlash,
     },
-    Channel, MatchSpec, Matches, NoArchType, PackageName, PackageUrl, ParseMatchSpecError,
+    Arch, Channel, MatchSpec, Matches, NoArchType, PackageName, PackageUrl, ParseMatchSpecError,
     ParseStrictness, Platform, RepoDataRecord, VersionWithSource,
 };
 
@@ -93,7 +93,10 @@ pub trait RecordFromPath {
 #[sorted]
 #[derive(Debug, Deserialize, Serialize, Eq, PartialEq, Clone, Hash)]
 pub struct PackageRecord {
-    /// Optionally the architecture the package supports
+    /// Optionally the architecture the package supports. This is almost
+    /// always the second part of the `subdir` string. Except for `64` which
+    /// maps to `x86_64` and `32` which maps to `x86`. This will be `None` if
+    /// the package is `noarch`.
     pub arch: Option<String>,
 
     /// The build string of the package
@@ -152,8 +155,11 @@ pub struct PackageRecord {
     #[serde(skip_serializing_if = "NoArchType::is_none")]
     pub noarch: NoArchType,
 
-    /// Optionally the platform the package supports
-    pub platform: Option<String>, // Note that this does not match the [`Platform`] enum..
+    /// Optionally the platform the package supports.
+    /// Note that this does not match the [`Platform`] enum, but is only the first
+    /// part of the platform (e.g. `linux`, `osx`, `win`, ...).
+    /// The `subdir` field contains the `Platform` enum.
+    pub platform: Option<String>,
 
     /// Package identifiers of packages that are equivalent to this package but
     /// from other ecosystems.
@@ -457,33 +463,17 @@ fn determine_subdir(
     let platform = platform.ok_or(ConvertSubdirError::PlatformEmpty)?;
     let arch = arch.ok_or(ConvertSubdirError::ArchEmpty)?;
 
-    let plat = match platform.as_ref() {
-        "linux" => match arch.as_ref() {
-            "x86" => Ok(Platform::Linux32),
-            "x86_64" => Ok(Platform::Linux64),
-            "aarch64" => Ok(Platform::LinuxAarch64),
-            "armv61" => Ok(Platform::LinuxArmV6l),
-            "armv71" => Ok(Platform::LinuxArmV7l),
-            "ppc64le" => Ok(Platform::LinuxPpc64le),
-            "ppc64" => Ok(Platform::LinuxPpc64),
-            "s390x" => Ok(Platform::LinuxS390X),
-            _ => Err(ConvertSubdirError::NoKnownCombination { platform, arch }),
-        },
-        "osx" => match arch.as_ref() {
-            "x86_64" => Ok(Platform::Osx64),
-            "arm64" => Ok(Platform::OsxArm64),
-            _ => Err(ConvertSubdirError::NoKnownCombination { platform, arch }),
-        },
-        "windows" => match arch.as_ref() {
-            "x86" => Ok(Platform::Win32),
-            "x86_64" => Ok(Platform::Win64),
-            "arm64" => Ok(Platform::WinArm64),
-            _ => Err(ConvertSubdirError::NoKnownCombination { platform, arch }),
-        },
-        _ => Err(ConvertSubdirError::NoKnownCombination { platform, arch }),
-    }?;
-    // Convert back to Platform string which should correspond to known subdirs
-    Ok(plat.to_string())
+    match arch.parse::<Arch>() {
+        Ok(arch) => {
+            let arch_str = match arch {
+                Arch::X86 => "32",
+                Arch::X86_64 => "64",
+                _ => arch.as_str(),
+            };
+            Ok(format!("{}-{}", platform, arch_str))
+        }
+        Err(_) => Err(ConvertSubdirError::NoKnownCombination { platform, arch }),
+    }
 }
 
 impl PackageRecord {

crates/rattler_lock/Cargo.toml

@@ -31,5 +31,6 @@ typed-path = { workspace = true }
 
 [dev-dependencies]
 insta = { workspace = true, features = ["yaml"] }
+serde_json = { workspace = true }
 similar-asserts = { workspace = true }
-rstest = { workspace = true }
+rstest = { workspace = true }

crates/rattler_lock/src/lib.rs

@@ -527,7 +527,7 @@ mod test {
         str::FromStr,
     };
 
-    use rattler_conda_types::Platform;
+    use rattler_conda_types::{Platform, RepoDataRecord};
     use rstest::*;
 
     use super::{LockFile, DEFAULT_ENVIRONMENT_NAME};
@@ -661,4 +661,54 @@ mod test {
         let conda_lock = LockFile::from_path(&path).unwrap();
         assert!(!conda_lock.is_empty());
     }
+
+    #[test]
+    fn solve_roundtrip() {
+        // load repodata from JSON
+        let path = Path::new(env!("CARGO_MANIFEST_DIR"))
+            .join("../../test-data/repodata-records/_libgcc_mutex-0.1-conda_forge.json");
+        let content = std::fs::read_to_string(&path).unwrap();
+        let repodata_record: RepoDataRecord = serde_json::from_str(&content).unwrap();
+
+        // check that the repodata record is as expected
+        assert_eq!(repodata_record.package_record.arch, None);
+        assert_eq!(repodata_record.package_record.platform, None);
+
+        // create a lockfile with the repodata record
+        let lock_file = LockFile::builder()
+            .with_conda_package(
+                DEFAULT_ENVIRONMENT_NAME,
+                Platform::Linux64,
+                repodata_record.clone().into(),
+            )
+            .finish();
+
+        // serialize the lockfile
+        let rendered_lock_file = lock_file.render_to_string().unwrap();
+
+        // parse the lockfile
+        let parsed_lock_file = LockFile::from_str(&rendered_lock_file).unwrap();
+        // get repodata record from parsed lockfile
+        let repodata_records = parsed_lock_file
+            .environment(DEFAULT_ENVIRONMENT_NAME)
+            .unwrap()
+            .conda_repodata_records(Platform::Linux64)
+            .unwrap()
+            .unwrap();
+
+        // These are not equal because the one from `repodata_records[0]` contains arch and platform.
+        let repodata_record_two = repodata_records[0].clone();
+        assert_eq!(
+            repodata_record_two.package_record.arch,
+            Some("x86_64".to_string())
+        );
+        assert_eq!(
+            repodata_record_two.package_record.platform,
+            Some("linux".to_string())
+        );
+
+        // But if we render it again, the lockfile should look the same at least
+        let rerendered_lock_file_two = parsed_lock_file.render_to_string().unwrap();
+        assert_eq!(rendered_lock_file, rerendered_lock_file_two);
+    }
 }

crates/rattler_lock/src/parse/models/v6/conda_package_data.rs

@@ -80,11 +80,6 @@ pub(crate) struct CondaPackageDataModel<'a> {
     pub extra_depends: Cow<'a, BTreeMap<String, Vec<String>>>,
 
     // Additional properties (in semi alphabetic order but grouped by commonality)
-    #[serde(default, skip_serializing_if = "Option::is_none")]
-    pub arch: Option<Cow<'a, Option<String>>>,
-    #[serde(default, skip_serializing_if = "Option::is_none")]
-    pub platform: Option<Cow<'a, Option<String>>>,
-
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub channel: Option<Cow<'a, Option<Url>>>,
 
@@ -175,8 +170,8 @@ impl<'a> TryFrom<CondaPackageDataModel<'a>> for CondaPackageData {
                 .or(derived.name)
                 .ok_or_else(|| ConversionError::Missing("name".to_string()))?,
             noarch,
-            arch: value.arch.map_or(derived_arch, Cow::into_owned),
-            platform: value.platform.map_or(derived_platform, Cow::into_owned),
+            arch: derived_arch,
+            platform: derived_platform,
             purls: value.purls.into_owned(),
             sha256: value.sha256,
             size: value.size.into_owned(),
@@ -241,13 +236,6 @@ impl<'a> From<&'a CondaPackageData> for CondaPackageDataModel<'a> {
             derived.subdir.as_deref().unwrap_or(&package_record.subdir),
             derived.build.as_deref().unwrap_or(&package_record.build),
         );
-        let (derived_arch, derived_platform) = derived_fields::derive_arch_and_platform(
-            derived.subdir.as_deref().unwrap_or(&package_record.subdir),
-        );
-
-        // Polyfill the arch and platform values if they are not present.
-        let arch = package_record.arch.clone().or(derived_arch);
-        let platform = package_record.platform.clone().or(derived_platform);
 
         let channel = value.as_binary().and_then(|binary| binary.channel.as_ref());
         let file_name = value.as_binary().map(|binary| binary.file_name.as_str());
@@ -282,8 +270,6 @@ impl<'a> From<&'a CondaPackageData> for CondaPackageDataModel<'a> {
             depends: Cow::Borrowed(&package_record.depends),
             constrains: Cow::Borrowed(&package_record.constrains),
             extra_depends: Cow::Borrowed(&package_record.extra_depends),
-            arch: (package_record.arch != arch).then_some(Cow::Owned(arch)),
-            platform: (package_record.platform != platform).then_some(Cow::Owned(platform)),
             md5: package_record.md5,
             legacy_bz2_md5: package_record.legacy_bz2_md5,
             sha256: package_record.sha256,

crates/rattler_lock/src/snapshots/rattler_lock__builder__test__merge_records_and_purls.snap

@@ -16,8 +16,6 @@ environments:
       - conda: https://prefix.dev/example/linux-64/foobar-1.0.0-build.tar.bz2
 packages:
 - conda: https://prefix.dev/example/linux-64/foobar-1.0.0-build.tar.bz2
-  arch: x86_64
-  platform: linux
   channel: null
   purls:
   - pkg:pypi/foobar@1.0.0

test-data/repodata-records/_libgcc_mutex-0.1-conda_forge.json

@@ -0,0 +1,16 @@
+{
+  "build": "conda_forge",
+  "build_number": 0,
+  "depends": [],
+  "license": "None",
+  "md5": "d7c89558ba9fa0495403155b64376d81",
+  "name": "_libgcc_mutex",
+  "sha256": "fe51de6107f9edc7aa4f786a70f4a883943bc9d39b3bb7307c04c41410990726",
+  "size": 2562,
+  "subdir": "linux-64",
+  "timestamp": 1578324546067,
+  "version": "0.1",
+  "fn": "_libgcc_mutex-0.1-conda_forge.tar.bz2",
+  "url": "https://conda.anaconda.org/conda-forge/linux-64/_libgcc_mutex-0.1-conda_forge.tar.bz2",
+  "channel": "https://conda.anaconda.org/conda-forge/"
+}