From 9ad9513605b26f37ad9650b84b62beec3f8a9fa1 Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Fri, 13 Mar 2026 09:22:25 +0000 Subject: [PATCH 1/4] refactor: Wrap all PypiPackageData into UnresolvedPypiRecord ... so we can introduce a ResolvedPypiRecord later. --- Cargo.lock | 1 + crates/pixi_api/Cargo.toml | 1 + crates/pixi_api/src/workspace/list/mod.rs | 2 +- crates/pixi_api/src/workspace/list/package.rs | 51 +++++++---- crates/pixi_core/src/lock_file/mod.rs | 8 +- .../src/lock_file/records_by_name.rs | 60 ++++++------- .../pixi_core/src/lock_file/resolve/pypi.rs | 23 +++-- .../src/lock_file/satisfiability/mod.rs | 88 +++++++++++-------- crates/pixi_core/src/lock_file/update.rs | 25 ++++-- crates/pixi_core/src/workspace/mod.rs | 1 - crates/pixi_install_pypi/src/conversions.rs | 12 +-- crates/pixi_install_pypi/src/lib.rs | 25 ++++-- .../src/plan/required_dists.rs | 21 ++--- .../src/plan/test/harness.rs | 14 +-- .../pixi_install_pypi/src/plan/validation.rs | 5 +- 15 files changed, 201 insertions(+), 136 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4eeb19d9ba..b5da943967 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5662,6 +5662,7 @@ dependencies = [ "pixi_config", "pixi_consts", "pixi_core", + "pixi_install_pypi", "pixi_manifest", "pixi_pypi_spec", "pixi_spec", diff --git a/crates/pixi_api/Cargo.toml b/crates/pixi_api/Cargo.toml index fd7cc7038a..0876e8ad77 100644 --- a/crates/pixi_api/Cargo.toml +++ b/crates/pixi_api/Cargo.toml @@ -21,6 +21,7 @@ pep508_rs = { workspace = true } pixi_config = { workspace = true } pixi_consts = { workspace = true } pixi_core = { workspace = true } +pixi_install_pypi = { workspace = true } pixi_manifest = { workspace = true } pixi_pypi_spec = { workspace = true } pixi_spec = { workspace = true } diff --git a/crates/pixi_api/src/workspace/list/mod.rs b/crates/pixi_api/src/workspace/list/mod.rs index 3c61e80d32..5b22936599 100644 --- a/crates/pixi_api/src/workspace/list/mod.rs +++ b/crates/pixi_api/src/workspace/list/mod.rs @@ -60,7 +60,7 @@ pub async fn list( .map(|p| match p { LockedPackageRef::Pypi(pypi_data) => { let name = to_uv_normalize(&pypi_data.name)?; - Ok(PackageExt::PyPI(pypi_data.clone(), name)) + Ok(PackageExt::PyPI(pypi_data.clone().into(), name)) } LockedPackageRef::Conda(c) => Ok(PackageExt::Conda(c.clone())), }) diff --git a/crates/pixi_api/src/workspace/list/package.rs b/crates/pixi_api/src/workspace/list/package.rs index 4f29bca646..587a7e7d62 100644 --- a/crates/pixi_api/src/workspace/list/package.rs +++ b/crates/pixi_api/src/workspace/list/package.rs @@ -1,6 +1,8 @@ use pixi_consts::consts; +use pixi_core::lock_file::HasNameVersion; +use pixi_install_pypi::UnresolvedPypiRecord; use pixi_uv_conversions::to_uv_version; -use rattler_lock::{CondaPackageData, PypiPackageData, UrlOrPath}; +use rattler_lock::{CondaPackageData, UrlOrPath}; use serde::Serialize; use std::str::FromStr; use std::{borrow::Cow, collections::HashMap}; @@ -77,7 +79,8 @@ impl Package { .map(|c| c.to_string().trim_end_matches('/').to_string()), }, ), - PackageExt::PyPI(p, name) => { + PackageExt::PyPI(record, name) => { + let p = record.as_package_data(); if p.hash.is_some() { let url = p .index_url @@ -126,7 +129,8 @@ impl Package { let md5 = match package { PackageExt::Conda(pkg) => pkg.record().and_then(|r| r.md5.map(|h| format!("{h:x}"))), - PackageExt::PyPI(p, _) => p + PackageExt::PyPI(record, _) => record + .as_package_data() .hash .as_ref() .and_then(|h| h.md5().map(|m| format!("{m:x}"))), @@ -136,7 +140,8 @@ impl Package { PackageExt::Conda(pkg) => pkg .record() .and_then(|r| r.sha256.map(|h| format!("{h:x}"))), - PackageExt::PyPI(p, _) => p + PackageExt::PyPI(record, _) => record + .as_package_data() .hash .as_ref() .and_then(|h| h.sha256().map(|s| format!("{s:x}"))), @@ -186,18 +191,25 @@ impl Package { ), CondaPackageData::Source(source) => (None, Some(source.location.to_string())), }, - PackageExt::PyPI(p, _) => ( - None, - Some( - p.location - .given() - .map_or_else(|| p.location.to_string(), ToOwned::to_owned), - ), - ), + PackageExt::PyPI(record, _) => { + let p = record.as_package_data(); + ( + None, + Some( + p.location + .given() + .map_or_else(|| p.location.to_string(), ToOwned::to_owned), + ), + ) + } }; let index_url = match package { - PackageExt::PyPI(p, _) => p.index_url.as_ref().map(|u| u.to_string()), + PackageExt::PyPI(record, _) => record + .as_package_data() + .index_url + .as_ref() + .map(|u| u.to_string()), PackageExt::Conda(_) => None, }; @@ -222,7 +234,12 @@ impl Package { let depends = match package { PackageExt::Conda(pkg) => pkg.record().map(|r| r.depends.clone()).unwrap_or_default(), - PackageExt::PyPI(p, _) => p.requires_dist.iter().map(|r| r.to_string()).collect(), + PackageExt::PyPI(record, _) => record + .as_package_data() + .requires_dist + .iter() + .map(|r| r.to_string()) + .collect(), }; let track_features = match package { @@ -303,7 +320,7 @@ fn serde_skip_is_editable(editable: &bool) -> bool { /// Associate with a uv_normalize::PackageName #[allow(clippy::large_enum_variant)] pub(crate) enum PackageExt { - PyPI(PypiPackageData, uv_normalize::PackageName), + PyPI(UnresolvedPypiRecord, uv_normalize::PackageName), Conda(CondaPackageData), } @@ -328,7 +345,7 @@ impl PackageExt { pub fn name(&self) -> Cow<'_, str> { match self { Self::Conda(value) => value.name().as_normalized().into(), - Self::PyPI(value, _) => value.name.as_dist_info_name(), + Self::PyPI(value, _) => value.name().as_dist_info_name(), } } @@ -336,7 +353,7 @@ impl PackageExt { pub fn version(&self) -> Option { match self { Self::Conda(value) => value.record().map(|r| r.version.to_string()), - Self::PyPI(value, _) => value.version.as_ref().map(|v| v.to_string()), + Self::PyPI(value, _) => value.version().map(|v| v.to_string()), } } } diff --git a/crates/pixi_core/src/lock_file/mod.rs b/crates/pixi_core/src/lock_file/mod.rs index 5226852680..7606b7e0ea 100644 --- a/crates/pixi_core/src/lock_file/mod.rs +++ b/crates/pixi_core/src/lock_file/mod.rs @@ -12,11 +12,13 @@ pub mod virtual_packages; pub use crate::environment::CondaPrefixUpdater; pub use install_subset::{FilteredPackages, InstallSubset}; pub use package_identifier::PypiPackageIdentifier; +use pixi_install_pypi::UnresolvedPypiRecord; use pixi_record::PixiRecord; pub use pixi_uv_context::UvResolutionContext; -use rattler_lock::PypiPackageData; pub use rattler_lock::Verbatim; -pub use records_by_name::{PixiRecordsByName, PypiRecordsByName, UnresolvedPixiRecordsByName}; +pub use records_by_name::{ + HasNameVersion, PixiRecordsByName, PypiRecordsByName, UnresolvedPixiRecordsByName, +}; pub use resolve::pypi::resolve_pypi; pub use satisfiability::{ Dependency, EnvironmentUnsat, PlatformUnsat, resolve_dev_dependencies, @@ -34,7 +36,7 @@ pub use utils::IoConcurrencyLimit; pub type LockedCondaPackages = Vec; /// A list of Pypi packages that are locked for a specific platform. -pub type LockedPypiPackages = Vec; +pub type LockedPypiPackages = Vec; #[cfg(test)] mod tests { diff --git a/crates/pixi_core/src/lock_file/records_by_name.rs b/crates/pixi_core/src/lock_file/records_by_name.rs index cdd5a037af..00c763caab 100644 --- a/crates/pixi_core/src/lock_file/records_by_name.rs +++ b/crates/pixi_core/src/lock_file/records_by_name.rs @@ -1,5 +1,6 @@ use super::package_identifier::ConversionError; -use crate::lock_file::{PypiPackageData, PypiPackageIdentifier}; +use crate::lock_file::PypiPackageIdentifier; +use pixi_install_pypi::UnresolvedPypiRecord; use pixi_record::{PixiRecord, UnresolvedPixiRecord}; use pixi_uv_conversions::to_uv_normalize; use pypi_modifiers::pypi_tags::is_python_record; @@ -8,7 +9,7 @@ use std::collections::HashMap; use std::collections::hash_map::Entry; use std::hash::Hash; -pub type PypiRecordsByName = DependencyRecordsByName; +pub type PypiRecordsByName = DependencyRecordsByName; pub type PixiRecordsByName = DependencyRecordsByName; pub type UnresolvedPixiRecordsByName = DependencyRecordsByName; @@ -26,15 +27,15 @@ pub trait HasNameVersion { fn version(&self) -> Option<&Self::V>; } -impl HasNameVersion for PypiPackageData { +impl HasNameVersion for UnresolvedPypiRecord { type N = pep508_rs::PackageName; type V = pep440_rs::Version; fn name(&self) -> &pep508_rs::PackageName { - &self.name + &self.as_package_data().name } fn version(&self) -> Option<&Self::V> { - self.version.as_ref() + self.as_package_data().version.as_ref() } } @@ -253,8 +254,7 @@ impl UnresolvedPixiRecordsByName { #[cfg(test)] mod tests { use super::*; - use crate::lock_file::PypiPackageData; - use rattler_lock::{UrlOrPath, Verbatim}; + use rattler_lock::{PypiPackageData, UrlOrPath, Verbatim}; use std::str::FromStr; fn make_pypi_package(name: &str, version: Option<&str>) -> PypiPackageData { @@ -272,22 +272,22 @@ mod tests { #[test] fn from_iter_with_none_version_does_not_panic() { // A single package with no version should work fine. - let records = vec![make_pypi_package("dynamic-dep", None)]; + let records = vec![make_pypi_package("dynamic-dep", None).into()]; let by_name = PypiRecordsByName::from_iter(records); assert_eq!(by_name.len(), 1); - assert!(by_name.records[0].version.is_none()); + assert!(by_name.records[0].version().is_none()); } #[test] fn from_iter_dedup_keeps_first_when_both_versions_none() { // Two packages with the same name and no version — should keep the first. let records = vec![ - make_pypi_package("dynamic-dep", None), - make_pypi_package("dynamic-dep", None), + make_pypi_package("dynamic-dep", None).into(), + make_pypi_package("dynamic-dep", None).into(), ]; let by_name = PypiRecordsByName::from_iter(records); assert_eq!(by_name.len(), 1); - assert!(by_name.records[0].version.is_none()); + assert!(by_name.records[0].version().is_none()); } #[test] @@ -295,65 +295,59 @@ mod tests { // First entry has no version, second has a version — keeps the first // because we can't compare None to Some. let records = vec![ - make_pypi_package("pkg", None), - make_pypi_package("pkg", Some("1.0.0")), + make_pypi_package("pkg", None).into(), + make_pypi_package("pkg", Some("1.0.0")).into(), ]; let by_name = PypiRecordsByName::from_iter(records); assert_eq!(by_name.len(), 1); - assert!(by_name.records[0].version.is_none()); + assert!(by_name.records[0].version().is_none()); } #[test] fn from_iter_dedup_keeps_first_when_new_has_no_version() { // First entry has a version, second has no version — keeps the first. let records = vec![ - make_pypi_package("pkg", Some("1.0.0")), - make_pypi_package("pkg", None), + make_pypi_package("pkg", Some("1.0.0")).into(), + make_pypi_package("pkg", None).into(), ]; let by_name = PypiRecordsByName::from_iter(records); assert_eq!(by_name.len(), 1); - assert_eq!( - by_name.records[0].version.as_ref().unwrap().to_string(), - "1.0.0" - ); + assert_eq!(by_name.records[0].version().unwrap().to_string(), "1.0.0"); } #[test] fn from_iter_dedup_picks_higher_version() { let records = vec![ - make_pypi_package("pkg", Some("1.0.0")), - make_pypi_package("pkg", Some("2.0.0")), + make_pypi_package("pkg", Some("1.0.0")).into(), + make_pypi_package("pkg", Some("2.0.0")).into(), ]; let by_name = PypiRecordsByName::from_iter(records); assert_eq!(by_name.len(), 1); - assert_eq!( - by_name.records[0].version.as_ref().unwrap().to_string(), - "2.0.0" - ); + assert_eq!(by_name.records[0].version().unwrap().to_string(), "2.0.0"); } #[test] fn from_unique_iter_with_none_version() { // from_unique_iter should work fine with None version (it doesn't compare versions). - let records = vec![make_pypi_package("dynamic-dep", None)]; + let records = vec![make_pypi_package("dynamic-dep", None).into()]; let by_name = PypiRecordsByName::from_unique_iter(records).unwrap(); assert_eq!(by_name.len(), 1); - assert!(by_name.records[0].version.is_none()); + assert!(by_name.records[0].version().is_none()); } #[test] fn mixed_versioned_and_dynamic_packages() { let records = vec![ - make_pypi_package("versioned-pkg", Some("1.0.0")), - make_pypi_package("dynamic-pkg", None), + make_pypi_package("versioned-pkg", Some("1.0.0")).into(), + make_pypi_package("dynamic-pkg", None).into(), ]; let by_name = PypiRecordsByName::from_iter(records); assert_eq!(by_name.len(), 2); let versioned = by_name.by_name(&"versioned-pkg".parse().unwrap()).unwrap(); - assert_eq!(versioned.version.as_ref().unwrap().to_string(), "1.0.0"); + assert_eq!(versioned.version().unwrap().to_string(), "1.0.0"); let dynamic = by_name.by_name(&"dynamic-pkg".parse().unwrap()).unwrap(); - assert!(dynamic.version.is_none()); + assert!(dynamic.version().is_none()); } } diff --git a/crates/pixi_core/src/lock_file/resolve/pypi.rs b/crates/pixi_core/src/lock_file/resolve/pypi.rs index 6ba3d07b6c..b0cb9ce12d 100644 --- a/crates/pixi_core/src/lock_file/resolve/pypi.rs +++ b/crates/pixi_core/src/lock_file/resolve/pypi.rs @@ -20,6 +20,7 @@ use indicatif::ProgressBar; use itertools::{Either, Itertools}; use miette::{Context, IntoDiagnostic}; use pixi_consts::consts; +use pixi_install_pypi::UnresolvedPypiRecord; use pixi_manifest::{ EnvironmentName, SolveStrategy, SystemRequirements, pypi::pypi_options::PypiOptions, }; @@ -285,7 +286,7 @@ pub async fn resolve_pypi( dependencies: IndexMap>, system_requirements: SystemRequirements, locked_pixi_records: &[PixiRecord], - locked_pypi_packages: &[PypiPackageData], + locked_pypi_packages: &[UnresolvedPypiRecord], platform: rattler_conda_types::Platform, pb: &ProgressBar, project_root: &Path, @@ -365,7 +366,7 @@ pub async fn resolve_pypi( // This ensures that when uv resolves git dependencies, it will find the cached commit // and not panic in `url_to_precise` function. for package_data in locked_pypi_packages { - if let Some(location) = package_data.location.as_url() + if let Some(location) = package_data.as_package_data().location.as_url() && LockedGitUrl::is_locked_git_url(location) { let locked_url = LockedGitUrl::new(location.clone()); @@ -621,11 +622,11 @@ pub async fn resolve_pypi( let preferences = locked_pypi_packages .iter() .map(|record| { - let Some(version) = &record.version else { + let Some(version) = record.version() else { return Ok(None); }; let requirement = uv_pep508::Requirement { - name: to_uv_normalize(&record.name)?, + name: to_uv_normalize(record.name())?, extras: Vec::new().into(), version_or_url: Some(uv_pep508::VersionOrUrl::VersionSpecifier( uv_pep440::VersionSpecifiers::from( @@ -640,7 +641,7 @@ pub async fn resolve_pypi( // because they are resolved based on the reference (branch/tag/rev) in the manifest. // This matches how uv handles git dependencies - it doesn't try to pin them via preferences. // The git resolver cache (pre-populated above) ensures the locked commit is preferred. - if let Some(location) = record.location.as_url() + if let Some(location) = record.as_package_data().location.as_url() && LockedGitUrl::is_locked_git_url(location) { // Skip git packages - they'll be resolved based on manifest reference @@ -794,7 +795,13 @@ pub async fn resolve_pypi( // We try to distinguish between build dispatch panics and any other panics that occur let (locked_packages, conda_task) = match resolution_future.catch_unwind().await { - Ok(result) => result?, + Ok(result) => { + let result = result?; + ( + result.0.iter().map(|r| r.clone().into()).collect(), + result.1, + ) + } Err(panic_payload) => { // Try to get the stored initialization error from the last_error holder if let Some(stored_error) = last_error.get() { @@ -947,7 +954,7 @@ async fn lock_pypi_packages( concurrent_downloads: usize, abs_project_root: &Path, original_git_references: &HashMap, -) -> miette::Result> { +) -> miette::Result { let mut locked_packages = LockedPypiPackages::with_capacity(resolution.len()); let database = DistributionDatabase::new(registry_client, pixi_build_dispatch, concurrent_downloads); @@ -1171,7 +1178,7 @@ async fn lock_pypi_packages( }, }; - locked_packages.push(pypi_package_data); + locked_packages.push(pypi_package_data.into()); } Ok(locked_packages) diff --git a/crates/pixi_core/src/lock_file/satisfiability/mod.rs b/crates/pixi_core/src/lock_file/satisfiability/mod.rs index 6ae1668382..e42a5fc592 100644 --- a/crates/pixi_core/src/lock_file/satisfiability/mod.rs +++ b/crates/pixi_core/src/lock_file/satisfiability/mod.rs @@ -7,6 +7,7 @@ use std::{ str::FromStr, }; +use crate::lock_file::records_by_name::HasNameVersion; use futures::TryStreamExt; use itertools::{Either, Itertools}; use miette::Diagnostic; @@ -19,6 +20,7 @@ use pixi_command_dispatcher::{ executor::CancellationAwareFutures, }; use pixi_git::url::RepositoryUrl; +use pixi_install_pypi::UnresolvedPypiRecord; use pixi_manifest::{ FeaturesExt, pypi::pypi_options::{NoBuild, PrereleaseMode}, @@ -42,8 +44,7 @@ use rattler_conda_types::{ PackageRecord, ParseChannelError, ParseMatchSpecError, ParseStrictness::Lenient, Platform, }; use rattler_lock::{ - LockedPackageRef, PackageHashes, PypiIndexes, PypiPackageData, PypiSourceTreeHashable, - UrlOrPath, + LockedPackageRef, PackageHashes, PypiIndexes, PypiSourceTreeHashable, UrlOrPath, }; use thiserror::Error; use typed_path::Utf8TypedPathBuf; @@ -587,12 +588,13 @@ pub fn verify_environment_satisfiability( for (lock_platform, package_it) in locked_environment.pypi_packages_by_platform() { let platform = lock_platform.subdir(); for package_data in package_it { + let record = UnresolvedPypiRecord::from(package_data.clone()); let pypi_source = pypi_dependencies - .get(&package_data.name) + .get(record.name()) .and_then(|specs| specs.last()) .map(|spec| &spec.source); - no_build_check.check(package_data, pypi_source)?; - pypi_wheel_tags_check.check(platform, package_data)?; + no_build_check.check(&record, pypi_source)?; + pypi_wheel_tags_check.check(platform, &record)?; } } } @@ -693,8 +695,9 @@ impl PypiWheelTagsCheck { pub fn check( &self, platform: Platform, - package_data: &PypiPackageData, + package_data: &UnresolvedPypiRecord, ) -> Result<(), EnvironmentUnsat> { + let package_data = package_data.as_package_data(); let Some(package_file_name) = package_data.location.file_name() else { return Ok(()); }; @@ -747,9 +750,10 @@ impl PypiNoBuildCheck { pub fn check( &self, - package_data: &PypiPackageData, + package_data: &UnresolvedPypiRecord, source: Option<&PixiPypiSource>, ) -> Result<(), EnvironmentUnsat> { + let package_data = package_data.as_package_data(); let Some(check) = &self.check else { return Ok(()); }; @@ -882,7 +886,7 @@ pub async fn verify_platform_satisfiability( // Convert the lock file into a list of conda and pypi packages. // Read as UnresolvedPixiRecord first, then resolve any partial source records. let mut unresolved_records: Vec = Vec::new(); - let mut pypi_packages: Vec = Vec::new(); + let mut pypi_packages: Vec = Vec::new(); let lock_platform = locked_environment .lock_file() .platform(&platform.to_string()); @@ -901,7 +905,7 @@ pub async fn verify_platform_satisfiability( ); } LockedPackageRef::Pypi(pypi) => { - pypi_packages.push(pypi.clone()); + pypi_packages.push(pypi.clone().into()); } } } @@ -1043,7 +1047,7 @@ pub async fn verify_platform_satisfiability( Ok(pypi_packages) => pypi_packages, Err(duplicate) => { return Err(CommandDispatcherError::Failed(Box::new( - PlatformUnsat::DuplicateEntry(duplicate.name.to_string()), + PlatformUnsat::DuplicateEntry(duplicate.name().to_string()), ))); } }; @@ -1086,9 +1090,10 @@ impl Dependency { /// references pub(crate) fn pypi_satisfies_editable( spec: &uv_distribution_types::Requirement, - locked_data: &PypiPackageData, + locked_data: &UnresolvedPypiRecord, project_root: &Path, ) -> Result<(), Box> { + let locked_data = locked_data.as_package_data(); // We dont match on spec.is_editable() != locked_data.editable // as it will happen later in verify_package_platform_satisfiability // TODO: could be a potential refactoring opportunity @@ -1142,9 +1147,10 @@ pub(crate) fn pypi_satisfies_editable( /// references pub(crate) fn pypi_satisfies_requirement( spec: &uv_distribution_types::Requirement, - locked_data: &PypiPackageData, + locked_data: &UnresolvedPypiRecord, project_root: &Path, ) -> Result<(), Box> { + let locked_data = locked_data.as_package_data(); if spec.name.to_string() != locked_data.name.to_string() { return Err(PlatformUnsat::LockedPyPINamesMismatch { expected: spec.name.to_string(), @@ -2380,6 +2386,7 @@ pub(crate) async fn verify_package_platform_satisfiability( } FoundPackage::PyPi(idx, extras) => { let record = &locked_pypi_environment.records[idx.0]; + let pkg = record.as_package_data(); // If there is no marker environment there is no python version let Some(marker_environment) = marker_environment.as_ref() else { @@ -2391,7 +2398,7 @@ pub(crate) async fn verify_package_platform_satisfiability( if pypi_packages_visited.insert(idx) { // If this is path based package we need to check if the source tree hash still // matches. and if it is a directory - if let UrlOrPath::Path(path) = &*record.location { + if let UrlOrPath::Path(path) = &*pkg.location { let absolute_path = if path.is_absolute() { Cow::Borrowed(Path::new(path.as_str())) } else { @@ -2402,13 +2409,13 @@ pub(crate) async fn verify_package_platform_satisfiability( match PypiSourceTreeHashable::from_directory(&absolute_path) .map(|hashable| hashable.hash()) { - Ok(hashable) if Some(&hashable) != record.hash.as_ref() => { + Ok(hashable) if Some(&hashable) != pkg.hash.as_ref() => { delayed_pypi_error.get_or_insert_with(|| { Box::new(PlatformUnsat::SourceTreeHashMismatch( - record.name.clone(), + pkg.name.clone(), SourceTreeHashMismatch { computed: hashable, - locked: record.hash.clone(), + locked: pkg.hash.clone(), }, )) }); @@ -2417,7 +2424,7 @@ pub(crate) async fn verify_package_platform_satisfiability( Err(err) => { delayed_pypi_error.get_or_insert_with(|| { Box::new(PlatformUnsat::FailedToDetermineSourceTreeHash( - record.name.clone(), + pkg.name.clone(), err, )) }); @@ -2427,7 +2434,7 @@ pub(crate) async fn verify_package_platform_satisfiability( } // Ensure that the record matches the currently selected interpreter. - if let Some(requires_python) = &record.requires_python { + if let Some(requires_python) = &pkg.requires_python { let uv_specifier_requires_python = to_uv_specifiers(requires_python) .expect("pep440 conversion should never fail"); @@ -2445,7 +2452,7 @@ pub(crate) async fn verify_package_platform_satisfiability( if !marker_requires_python.is_contained_by(&uv_specifier_requires_python) { delayed_pypi_error.get_or_insert_with(|| { Box::new(PlatformUnsat::PythonVersionMismatch( - record.name.clone(), + pkg.name.clone(), requires_python.clone(), marker_version.into(), )) @@ -2455,7 +2462,7 @@ pub(crate) async fn verify_package_platform_satisfiability( } // Add all the requirements of the package to the queue. - for requirement in &record.requires_dist { + for requirement in &pkg.requires_dist { let requirement = match pep508_requirement_to_uv_requirement(requirement.clone()) { Ok(requirement) => requirement, @@ -2479,7 +2486,7 @@ pub(crate) async fn verify_package_platform_satisfiability( pypi_queue.push(Dependency::PyPi( requirement.clone(), - record.name.as_ref().to_string().into(), + pkg.name.as_ref().to_string().into(), )); } } @@ -2823,7 +2830,7 @@ mod tests { use pixi_build_backend_passthrough::PassthroughBackend; use pixi_build_frontend::BackendOverride; use pixi_command_dispatcher::CacheDirs; - use rattler_lock::{LockFile, Verbatim}; + use rattler_lock::{LockFile, PypiPackageData, Verbatim}; use rstest::rstest; use tracing_test::traced_test; @@ -3052,7 +3059,7 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + }.into(); let spec = pep508_requirement_to_uv_requirement( pep508_rs::Requirement::from_str("mypkg @ git+https://github.com/mypkg@2993").unwrap(), ) @@ -3072,7 +3079,7 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + }.into(); let spec = pep508_requirement_to_uv_requirement( pep508_rs::Requirement::from_str( "mypkg @ git+https://github.com/mypkg.git@29932f3915935d773dc8d52c292cadd81c81071d", @@ -3111,7 +3118,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); pypi_satisfies_requirement( &spec_without_rev, &locked_data_default_branch, @@ -3134,7 +3142,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = pep508_rs::Requirement::from_str("mypkg @ file:///home/username/mypkg.tar.gz").unwrap(); @@ -3154,7 +3163,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = pep508_rs::Requirement::from_str("mypkg @ file:///C:\\Users\\username\\mypkg.tar.gz") @@ -3195,7 +3205,8 @@ mod tests { hash: None, requires_dist: vec![], requires_python: None, - }, + } + .into(), Some(&PixiPypiSource::Path { path: PathBuf::from("").into(), editable: Some(true), @@ -3218,7 +3229,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = pep508_requirement_to_uv_requirement( pep508_rs::Requirement::from_str("dynamic-dep @ file:///home/user/project/dynamic-dep") @@ -3245,7 +3257,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = pep508_requirement_to_uv_requirement( pep508_rs::Requirement::from_str( @@ -3274,7 +3287,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = pep508_requirement_to_uv_requirement( pep508_rs::Requirement::from_str("mypkg @ git+https://github.com/mypkg").unwrap(), @@ -3304,7 +3318,8 @@ mod tests { index_url: Some(Url::parse("https://custom.example.com/simple").unwrap()), requires_dist: vec![], requires_python: None, - }; + } + .into(); // Requirement: no index specified (user removed the `index` field). let spec = pep508_requirement_to_uv_requirement( @@ -3363,7 +3378,8 @@ mod tests { index_url: Some(Url::parse("https://old-index.example.com/simple").unwrap()), requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = registry_requirement_with_index( "my-dep", @@ -3393,7 +3409,8 @@ mod tests { index_url: Some(Url::parse(index_url).unwrap()), requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = registry_requirement_with_index("my-dep", ">=1.0", index_url); @@ -3420,7 +3437,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); let spec = registry_requirement_with_index("my-dep", ">=1.0", "https://custom.example.com/simple"); diff --git a/crates/pixi_core/src/lock_file/update.rs b/crates/pixi_core/src/lock_file/update.rs index a4bb963df9..542969a5f1 100644 --- a/crates/pixi_core/src/lock_file/update.rs +++ b/crates/pixi_core/src/lock_file/update.rs @@ -10,6 +10,7 @@ use std::{ time::{Duration, Instant}, }; +use crate::lock_file::records_by_name::HasNameVersion; use barrier_cell::BarrierCell; use dashmap::DashMap; use fancy_display::FancyDisplay; @@ -28,7 +29,7 @@ use pixi_consts::consts; use pixi_glob::GlobHashCache; use pixi_install_pypi::{ LazyEnvironmentVariables, PyPIBuildConfig, PyPIContextConfig, PyPIEnvironmentUpdater, - PyPIUpdateConfig, + PyPIUpdateConfig, UnresolvedPypiRecord, }; use pixi_manifest::{ChannelPriority, EnvironmentName, FeaturesExt}; use pixi_progress::global_multi_progress; @@ -68,7 +69,7 @@ use crate::{ read_environment_file, write_environment_file, }, lock_file::{ - self, PypiPackageData, reporter::SolveProgressBar, + self, reporter::SolveProgressBar, virtual_packages::validate_system_meets_environment_requirements, }, workspace::{ @@ -747,7 +748,7 @@ impl<'p> LockFileDerivedData<'p> { .filter_map(LockedPackageRef::as_pypi) .map(move |data| { ( - data.clone(), + Into::::into(data.clone()), pixi_install_pypi::ManifestData { editable: is_editable_from_manifest( &manifest_pypi_deps, @@ -783,7 +784,7 @@ impl<'p> LockFileDerivedData<'p> { let pypi_lock_file_names = pypi_records .iter() - .filter_map(|(data, _)| to_uv_normalize(&data.name).ok()) + .filter_map(|(data, _)| to_uv_normalize(data.name()).ok()) .collect::>(); // Figure out uv reinstall @@ -1650,7 +1651,9 @@ impl<'p> UpdateContextBuilder<'p> { .map(|(lock_platform, records)| { ( lock_platform.subdir(), - Arc::new(PypiRecordsByName::from_iter(records.cloned())), + Arc::new(PypiRecordsByName::from_iter( + records.map(|r| r.clone().into()), + )), ) }) .collect(), @@ -2369,7 +2372,11 @@ impl<'p> UpdateContext<'p> { if let Some(records) = self.take_latest_pypi_records(&environment, platform) { for pkg_data in records.into_inner() { builder - .add_pypi_package(&environment_name, &platform_str, pkg_data) + .add_pypi_package( + &environment_name, + &platform_str, + pkg_data.as_package_data().clone(), + ) .expect("platform was registered"); has_pypi_records = true; } @@ -2685,7 +2692,7 @@ async fn spawn_extract_environment_task( enum PackageRecord<'a> { Conda(&'a PixiRecord), - Pypi((&'a PypiPackageData, Option)), + Pypi((&'a UnresolvedPypiRecord, Option)), } // Determine the conda packages we need. @@ -2833,7 +2840,7 @@ async fn spawn_extract_environment_task( .into_diagnostic()? .unwrap_or_default(); - for req in record.requires_dist.iter() { + for req in record.as_package_data().requires_dist.iter() { // Evaluate the marker environment with the given extras if let Some(marker_env) = &marker_environment { // let marker_str = marker_env.to_string(); @@ -2866,7 +2873,7 @@ async fn spawn_extract_environment_task( } // Insert the record if it is not already present - pypi_records.entry(record.name.clone()).or_insert(record); + pypi_records.entry(record.name().clone()).or_insert(record); } } } diff --git a/crates/pixi_core/src/workspace/mod.rs b/crates/pixi_core/src/workspace/mod.rs index f02049f070..2a2a9384ea 100644 --- a/crates/pixi_core/src/workspace/mod.rs +++ b/crates/pixi_core/src/workspace/mod.rs @@ -938,7 +938,6 @@ mod tests { use pixi_manifest::{FeatureName, FeaturesExt}; use rattler_conda_types::{Platform, Version}; use rattler_virtual_packages::{LibC, VirtualPackage}; - use std::env; use super::*; diff --git a/crates/pixi_install_pypi/src/conversions.rs b/crates/pixi_install_pypi/src/conversions.rs index dc4e0f0e16..0dfb8da521 100644 --- a/crates/pixi_install_pypi/src/conversions.rs +++ b/crates/pixi_install_pypi/src/conversions.rs @@ -6,7 +6,7 @@ use pixi_record::LockedGitUrl; use pixi_uv_conversions::{ ConversionError, to_parsed_git_url, to_uv_normalize, to_uv_version, to_uv_version_specifiers, }; -use rattler_lock::{PackageHashes, PypiPackageData, UrlOrPath}; +use rattler_lock::{PackageHashes, UrlOrPath}; use url::Url; use uv_distribution_filename::DistExtension; use uv_distribution_filename::{ExtensionError, SourceDistExtension, WheelFilename}; @@ -100,12 +100,13 @@ pub enum ConvertToUvDistError { UvPepTypes(#[from] ConversionError), } -/// Convert from a PypiPackageData to a uv [`distribution_types::Dist`] +/// Convert from an [`UnresolvedPypiRecord`] to a uv [`distribution_types::Dist`] pub fn convert_to_dist( - pkg: &PypiPackageData, + record: &crate::UnresolvedPypiRecord, manifest_data: &crate::ManifestData, lock_file_dir: &Path, ) -> Result { + let pkg = record.as_package_data(); // Figure out if it is a url from the registry or a direct url let dist = match &*pkg.location { UrlOrPath::Url(url) if is_direct_url(url.scheme()) => { @@ -249,7 +250,7 @@ mod tests { let wheel = "torch-2.3.0%2Bcu121-cp312-cp312-win_amd64.whl"; let url = format!("https://example.com/{wheel}").parse().unwrap(); // Pass into locked data - let locked = PypiPackageData { + let locked: crate::UnresolvedPypiRecord = PypiPackageData { name: "torch".parse().unwrap(), version: Some(Version::from_str("2.3.0+cu121").unwrap()), location: UrlOrPath::Url(url).into(), @@ -257,7 +258,8 @@ mod tests { index_url: None, requires_dist: vec![], requires_python: None, - }; + } + .into(); // Convert the locked data to a uv dist // check if it does not panic diff --git a/crates/pixi_install_pypi/src/lib.rs b/crates/pixi_install_pypi/src/lib.rs index 9b0f6daf6e..06be05e788 100644 --- a/crates/pixi_install_pypi/src/lib.rs +++ b/crates/pixi_install_pypi/src/lib.rs @@ -56,7 +56,22 @@ pub struct ManifestData { pub editable: bool, } -pub type PyPIRecords = (PypiPackageData, ManifestData); +#[derive(Clone, Debug)] +pub struct UnresolvedPypiRecord(PypiPackageData); + +impl From for UnresolvedPypiRecord { + fn from(value: PypiPackageData) -> Self { + UnresolvedPypiRecord(value) + } +} + +impl UnresolvedPypiRecord { + pub fn as_package_data(&self) -> &PypiPackageData { + &self.0 + } +} + +pub type PypiRecords = (UnresolvedPypiRecord, ManifestData); pub(crate) mod conda_pypi_clobber; pub(crate) mod conversions; @@ -131,7 +146,7 @@ async fn uninstall_outdated_site_packages(site_packages: &Path) -> miette::Resul pub async fn on_python_interpreter_change<'a>( status: &'a PythonStatus, prefix: &Prefix, - pypi_records: &[PyPIRecords], + pypi_records: &[PypiRecords], ) -> miette::Result> { match status { PythonStatus::Removed { old } => { @@ -277,7 +292,7 @@ impl<'a> PyPIEnvironmentUpdater<'a> { &self, python_status: &PythonStatus, pixi_records: &[PixiRecord], - pypi_records: &[PyPIRecords], + pypi_records: &[PypiRecords], ) -> miette::Result<()> { // Initialize UV flags from environment variables and pypi-options before any operations initialize_uv_flags(self.build_config.skip_wheel_filename_check); @@ -311,7 +326,7 @@ impl<'a> PyPIEnvironmentUpdater<'a> { async fn execute_update( &self, pixi_records: &[PixiRecord], - pypi_records: &[PyPIRecords], + pypi_records: &[PypiRecords], python_info: &rattler::install::PythonInfo, ) -> miette::Result<()> { // Cheap planning setup (no network I/O) @@ -479,7 +494,7 @@ impl<'a> PyPIEnvironmentUpdater<'a> { /// Create the installation plan by analyzing current state vs requirements async fn create_installation_plan( &self, - pypi_records: &[crate::PyPIRecords], + pypi_records: &[crate::PypiRecords], planner_config: &UvInstallerPlannerConfig, ) -> miette::Result { // Create required distributions with pre-created Dist objects diff --git a/crates/pixi_install_pypi/src/plan/required_dists.rs b/crates/pixi_install_pypi/src/plan/required_dists.rs index 983ccc40f4..df8843c504 100644 --- a/crates/pixi_install_pypi/src/plan/required_dists.rs +++ b/crates/pixi_install_pypi/src/plan/required_dists.rs @@ -1,27 +1,27 @@ //! Utilities for creating and managing required distribution mappings for PyPI packages. //! -//! This module provides functionality to convert PypiPackageData into Dist objects +//! This module provides functionality to convert UnresolvedPypiRecord into Dist objects //! and manage them in a way that satisfies lifetime requirements for the install planner. use std::path::Path; use std::str::FromStr; use std::{collections::HashMap, ops::Deref}; -use rattler_lock::PypiPackageData; use uv_distribution_types::Dist; use uv_normalize::PackageName; use crate::conversions::{ConvertToUvDistError, convert_to_dist}; +use crate::{ManifestData, UnresolvedPypiRecord}; /// A collection of required distributions with their associated package data. /// This struct owns the Dist objects to ensure proper lifetimes for the install planner. pub struct RequiredDists( - /// Map from normalized package name to (PypiPackageData, Dist) - HashMap, + /// Map from normalized package name to (UnresolvedPypiRecord, Dist) + HashMap, ); impl RequiredDists { - /// Create a new RequiredDists from a slice of PypiPackageData and a lock file directory. + /// Create a new RequiredDists from a slice of UnresolvedPypiRecord and a lock file directory. /// /// # Arguments /// * `packages` - The PyPI package data to convert @@ -30,14 +30,15 @@ impl RequiredDists { /// # Returns /// A RequiredDists instance or an error if conversion fails pub fn from_packages( - packages: &[(&PypiPackageData, &crate::ManifestData)], + packages: &[(&UnresolvedPypiRecord, &ManifestData)], lock_file_dir: impl AsRef, ) -> Result { let mut dists = HashMap::new(); for (pkg, manifest_data) in packages { - let uv_name = PackageName::from_str(pkg.name.as_ref()).map_err(|_| { - ConvertToUvDistError::InvalidPackageName(pkg.name.as_ref().to_string()) + let pkg_data = pkg.as_package_data(); + let uv_name = PackageName::from_str(pkg_data.name.as_ref()).map_err(|_| { + ConvertToUvDistError::InvalidPackageName(pkg_data.name.as_ref().to_string()) })?; let dist = convert_to_dist(pkg, manifest_data, lock_file_dir.as_ref())?; dists.insert(uv_name, ((*pkg).clone(), dist)); @@ -48,7 +49,7 @@ impl RequiredDists { /// Get a reference map suitable for passing to InstallPlanner::plan(). /// Returns a map where the values are references to the owned data. - pub fn as_ref_map(&self) -> HashMap { + pub fn as_ref_map(&self) -> HashMap { self.0 .iter() .map(|(name, (pkg, dist))| (name.clone(), (pkg, dist))) @@ -62,7 +63,7 @@ impl RequiredDists { } impl Deref for RequiredDists { - type Target = HashMap; + type Target = HashMap; fn deref(&self) -> &Self::Target { &self.0 diff --git a/crates/pixi_install_pypi/src/plan/test/harness.rs b/crates/pixi_install_pypi/src/plan/test/harness.rs index d7925a1073..3a426fc0a1 100644 --- a/crates/pixi_install_pypi/src/plan/test/harness.rs +++ b/crates/pixi_install_pypi/src/plan/test/harness.rs @@ -1,7 +1,7 @@ -use crate::ManifestData; use crate::plan::InstallPlanner; use crate::plan::cache::DistCache; use crate::plan::installed_dists::InstalledDists; +use crate::{ManifestData, UnresolvedPypiRecord}; use pixi_consts::consts; use pixi_uv_conversions::GitUrlWithPrefix; use rattler_lock::{PypiPackageData, UrlOrPath}; @@ -480,7 +480,7 @@ impl<'a> DistCache<'a> for AllCached { /// Struct to create the required packages map #[derive(Default)] pub struct RequiredPackages { - required: HashMap, + required: HashMap, } impl RequiredPackages { @@ -492,7 +492,7 @@ impl RequiredPackages { pub fn add_registry>(mut self, name: S, version: S) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::registry(name, version); + let data = PyPIPackageDataBuilder::registry(name, version).into(); self.required .insert(package_name, (data, ManifestData { editable: false })); self @@ -508,7 +508,7 @@ impl RequiredPackages { ) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::path(name, version, path); + let data = PyPIPackageDataBuilder::path(name, version, path).into(); self.required .insert(package_name, (data, ManifestData { editable })); self @@ -517,7 +517,7 @@ impl RequiredPackages { pub fn add_local_wheel>(mut self, name: S, version: S, path: PathBuf) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::path(name, version, path); + let data = PyPIPackageDataBuilder::path(name, version, path).into(); self.required .insert(package_name, (data, ManifestData { editable: false })); self @@ -526,7 +526,7 @@ impl RequiredPackages { pub fn add_archive>(mut self, name: S, version: S, url: Url) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::url(name, version, url, UrlType::Direct); + let data = PyPIPackageDataBuilder::url(name, version, url, UrlType::Direct).into(); self.required .insert(package_name, (data, ManifestData { editable: false })); self @@ -535,7 +535,7 @@ impl RequiredPackages { pub fn add_git>(mut self, name: S, version: S, url: Url) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::url(name, version, url, UrlType::Other); + let data = PyPIPackageDataBuilder::url(name, version, url, UrlType::Other).into(); self.required .insert(package_name, (data, ManifestData { editable: false })); self diff --git a/crates/pixi_install_pypi/src/plan/validation.rs b/crates/pixi_install_pypi/src/plan/validation.rs index 60e4256364..b7c0f89d47 100644 --- a/crates/pixi_install_pypi/src/plan/validation.rs +++ b/crates/pixi_install_pypi/src/plan/validation.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use pixi_git::url::RepositoryUrl; use pixi_record::LockedGitUrl; use pixi_uv_conversions::{to_parsed_git_url, to_uv_version}; -use rattler_lock::{PypiPackageData, UrlOrPath}; +use rattler_lock::UrlOrPath; use url::Url; use uv_cache_info::CacheInfoError; use uv_distribution_types::{Dist, InstalledDist, InstalledDistKind}; @@ -31,10 +31,11 @@ pub enum NeedsReinstallError { /// Check if a package needs to be reinstalled pub(crate) fn need_reinstall( installed_dist: &InstalledDist, - required_pkg: &PypiPackageData, + required_record: &crate::UnresolvedPypiRecord, required_dist: &Dist, lock_file_dir: &Path, ) -> Result { + let required_pkg = required_record.as_package_data(); // Check if the installed version is the same as the required version match &installed_dist.kind { InstalledDistKind::Registry(reg) => { From 65a1a282f1bbd7dc2db57f03a0f33b93a2d21e7f Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Tue, 17 Mar 2026 09:28:43 +0000 Subject: [PATCH 2/4] refactor: Introduce InstallablePypiRecord ... with just the data needed to install a pypi package. Avoids some needless copied of the LockFile data -- which is incomplete for some use cases anyway. --- crates/pixi_core/src/lock_file/update.rs | 12 +++- crates/pixi_install_pypi/src/conversions.rs | 64 +++++++++--------- crates/pixi_install_pypi/src/lib.rs | 45 ++++++++++--- crates/pixi_install_pypi/src/plan/planner.rs | 19 ++++-- .../src/plan/required_dists.rs | 36 ++++++---- .../src/plan/test/harness.rs | 66 +++++++++++++------ .../pixi_install_pypi/src/plan/validation.rs | 20 ++---- 7 files changed, 161 insertions(+), 101 deletions(-) diff --git a/crates/pixi_core/src/lock_file/update.rs b/crates/pixi_core/src/lock_file/update.rs index 542969a5f1..3b1bd642e1 100644 --- a/crates/pixi_core/src/lock_file/update.rs +++ b/crates/pixi_core/src/lock_file/update.rs @@ -747,14 +747,20 @@ impl<'p> LockFileDerivedData<'p> { .into_iter() .filter_map(LockedPackageRef::as_pypi) .map(move |data| { - ( - Into::::into(data.clone()), + // Use the version requested in the lock file or be happy with *any* version + let version = data + .version + .clone() + .unwrap_or_else(|| pep440_rs::MIN_VERSION.clone()); + pixi_install_pypi::InstallablePypiRecord::new( + data, pixi_install_pypi::ManifestData { editable: is_editable_from_manifest( &manifest_pypi_deps, &data.name, ), }, + version, ) }) .collect::>(); @@ -784,7 +790,7 @@ impl<'p> LockFileDerivedData<'p> { let pypi_lock_file_names = pypi_records .iter() - .filter_map(|(data, _)| to_uv_normalize(data.name()).ok()) + .filter_map(|r| to_uv_normalize(&r.name).ok()) .collect::>(); // Figure out uv reinstall diff --git a/crates/pixi_install_pypi/src/conversions.rs b/crates/pixi_install_pypi/src/conversions.rs index 0dfb8da521..27932c6187 100644 --- a/crates/pixi_install_pypi/src/conversions.rs +++ b/crates/pixi_install_pypi/src/conversions.rs @@ -16,6 +16,8 @@ use uv_distribution_types::{ }; use uv_pypi_types::{HashAlgorithm, HashDigest, ParsedUrl, ParsedUrlError, VerbatimParsedUrl}; +use crate::InstallablePypiRecord; + use super::utils::{is_direct_url, strip_direct_scheme}; /// Build an [`IndexUrl`] from the lock-file's optional `index_url`. @@ -102,16 +104,14 @@ pub enum ConvertToUvDistError { /// Convert from an [`UnresolvedPypiRecord`] to a uv [`distribution_types::Dist`] pub fn convert_to_dist( - record: &crate::UnresolvedPypiRecord, - manifest_data: &crate::ManifestData, + record: &InstallablePypiRecord, lock_file_dir: &Path, ) -> Result { - let pkg = record.as_package_data(); // Figure out if it is a url from the registry or a direct url - let dist = match &*pkg.location { + let dist = match &record.location { UrlOrPath::Url(url) if is_direct_url(url.scheme()) => { let url_without_direct = strip_direct_scheme(url); - let pkg_name = to_uv_normalize(&pkg.name)?; + let pkg_name = to_uv_normalize(&record.name)?; if LockedGitUrl::is_locked_git_url(&url_without_direct) { let locked_git_url = LockedGitUrl::new(url_without_direct.clone().into_owned()); @@ -160,9 +160,9 @@ pub fn convert_to_dist( // which is essentially the file information for a wheel or sdist let file = locked_data_to_file( url, - pkg.hash.as_ref(), + record.hash.as_ref(), filename_decoded.as_ref(), - pkg.requires_python.clone(), + record.requires_python.clone(), )?; // Recreate the filename from the extracted last component // If this errors this is not a valid wheel filename @@ -173,23 +173,19 @@ pub fn convert_to_dist( wheels: vec![RegistryBuiltWheel { filename, file: Box::new(file), - index: index_url_from_lock(pkg.index_url.as_ref()), + index: index_url_from_lock(record.index_url.as_ref()), }], best_wheel_index: 0, sdist: None, })) } else { - let pkg_name = to_uv_normalize(&pkg.name)?; - let pkg_version = to_uv_version( - pkg.version - .as_ref() - .expect("registry source dists always have a version"), - )?; + let pkg_name = to_uv_normalize(&record.name)?; + let pkg_version = to_uv_version(&record.version)?; Dist::Source(SourceDist::Registry(RegistrySourceDist { name: pkg_name, version: pkg_version, file: Box::new(file), - index: index_url_from_lock(pkg.index_url.as_ref()), + index: index_url_from_lock(record.index_url.as_ref()), // I don't think this really matters for the install wheels: vec![], ext: SourceDistExtension::from_path(Path::new(filename_raw)).map_err(|e| { @@ -207,14 +203,14 @@ pub fn convert_to_dist( }; let absolute_url = uv_pep508::VerbatimUrl::from_absolute_path(&abs_path)?; - let pkg_name = - uv_normalize::PackageName::from_str(pkg.name.as_ref()).expect("should be correct"); + let pkg_name = uv_normalize::PackageName::from_str(record.name.as_ref()) + .expect("should be correct"); if abs_path.is_dir() { Dist::from_directory_url( pkg_name, absolute_url, &abs_path, - Some(manifest_data.editable), + Some(record.manifest_data.editable), Some(false), )? } else { @@ -249,26 +245,26 @@ mod tests { // Create url with special characters let wheel = "torch-2.3.0%2Bcu121-cp312-cp312-win_amd64.whl"; let url = format!("https://example.com/{wheel}").parse().unwrap(); + let version = Version::from_str("2.3.0+cu121").unwrap(); // Pass into locked data - let locked: crate::UnresolvedPypiRecord = PypiPackageData { - name: "torch".parse().unwrap(), - version: Some(Version::from_str("2.3.0+cu121").unwrap()), - location: UrlOrPath::Url(url).into(), - hash: None, - index_url: None, - requires_dist: vec![], - requires_python: None, - } - .into(); + let locked = crate::InstallablePypiRecord::new( + &PypiPackageData { + name: "torch".parse().unwrap(), + version: Some(version.clone()), + location: UrlOrPath::Url(url).into(), + hash: None, + index_url: None, + requires_dist: vec![], + requires_python: None, + }, + crate::ManifestData { editable: false }, + version, + ); // Convert the locked data to a uv dist // check if it does not panic - let dist = convert_to_dist( - &locked, - &crate::ManifestData { editable: false }, - &PathBuf::new(), - ) - .expect("could not convert wheel with special chars to dist"); + let dist = convert_to_dist(&locked, &PathBuf::new()) + .expect("could not convert wheel with special chars to dist"); // Check if the dist is a built dist assert!(!dist.filename().unwrap().contains("%2B")); diff --git a/crates/pixi_install_pypi/src/lib.rs b/crates/pixi_install_pypi/src/lib.rs index 06be05e788..5b1bf10fd0 100644 --- a/crates/pixi_install_pypi/src/lib.rs +++ b/crates/pixi_install_pypi/src/lib.rs @@ -10,6 +10,7 @@ use conda_pypi_clobber::PypiCondaClobberRegistry; use fancy_display::FancyDisplay; use itertools::Itertools; use miette::{IntoDiagnostic, WrapErr}; +use pep440_rs::VersionSpecifiers; use pixi_consts::consts; use pixi_manifest::{ EnvironmentName, SystemRequirements, @@ -31,7 +32,7 @@ use pypi_modifiers::{ pypi_tags::{get_pypi_tags, is_python_record}, }; use rattler_conda_types::Platform; -use rattler_lock::{PypiIndexes, PypiPackageData}; +use rattler_lock::{PypiIndexes, PypiPackageData, UrlOrPath}; use rayon::prelude::*; use utils::elapsed; use uv_auth::store_credentials_from_url; @@ -52,6 +53,7 @@ use uv_resolver::{ExcludeNewer, FlatIndex}; use crate::plan::{CachedWheels, RequiredDists}; /// Extra data available from the manifest, not the lockfile +#[derive(Clone)] pub struct ManifestData { pub editable: bool, } @@ -71,7 +73,34 @@ impl UnresolvedPypiRecord { } } -pub type PypiRecords = (UnresolvedPypiRecord, ManifestData); +#[derive(Clone)] +pub struct InstallablePypiRecord { + pub manifest_data: ManifestData, + pub name: pep508_rs::PackageName, + pub index_url: Option, + pub hash: Option, + pub location: UrlOrPath, + pub version: pep440_rs::Version, + pub requires_python: Option, +} + +impl InstallablePypiRecord { + pub fn new( + data: &PypiPackageData, + manifest_data: ManifestData, + version_override: pep440_rs::Version, + ) -> Self { + Self { + manifest_data, + name: data.name.clone(), + location: data.location.inner().clone(), + hash: data.hash.clone(), + index_url: data.index_url.clone(), + requires_python: data.requires_python.clone(), + version: version_override, + } + } +} pub(crate) mod conda_pypi_clobber; pub(crate) mod conversions; @@ -146,7 +175,7 @@ async fn uninstall_outdated_site_packages(site_packages: &Path) -> miette::Resul pub async fn on_python_interpreter_change<'a>( status: &'a PythonStatus, prefix: &Prefix, - pypi_records: &[PypiRecords], + pypi_records: &[InstallablePypiRecord], ) -> miette::Result> { match status { PythonStatus::Removed { old } => { @@ -292,7 +321,7 @@ impl<'a> PyPIEnvironmentUpdater<'a> { &self, python_status: &PythonStatus, pixi_records: &[PixiRecord], - pypi_records: &[PypiRecords], + pypi_records: &[InstallablePypiRecord], ) -> miette::Result<()> { // Initialize UV flags from environment variables and pypi-options before any operations initialize_uv_flags(self.build_config.skip_wheel_filename_check); @@ -326,7 +355,7 @@ impl<'a> PyPIEnvironmentUpdater<'a> { async fn execute_update( &self, pixi_records: &[PixiRecord], - pypi_records: &[PypiRecords], + pypi_records: &[InstallablePypiRecord], python_info: &rattler::install::PythonInfo, ) -> miette::Result<()> { // Cheap planning setup (no network I/O) @@ -494,14 +523,12 @@ impl<'a> PyPIEnvironmentUpdater<'a> { /// Create the installation plan by analyzing current state vs requirements async fn create_installation_plan( &self, - pypi_records: &[crate::PypiRecords], + pypi_records: &[crate::InstallablePypiRecord], planner_config: &UvInstallerPlannerConfig, ) -> miette::Result { // Create required distributions with pre-created Dist objects - let required_packages: Vec<_> = - pypi_records.iter().map(|(pkg, spec)| (pkg, spec)).collect(); let required_dists = - RequiredDists::from_packages(&required_packages, self.config.lock_file_dir) + RequiredDists::from_packages(pypi_records.iter(), self.config.lock_file_dir) .into_diagnostic() .context("Failed to create required distributions")?; diff --git a/crates/pixi_install_pypi/src/plan/planner.rs b/crates/pixi_install_pypi/src/plan/planner.rs index 87d6425068..2b79e538ba 100644 --- a/crates/pixi_install_pypi/src/plan/planner.rs +++ b/crates/pixi_install_pypi/src/plan/planner.rs @@ -120,7 +120,7 @@ impl InstallPlanner { // Empty string if no installer or any other error .map_or(String::new(), |f| f.unwrap_or_default()); - if let Some((required_pkg, required_dist)) = pkg_and_dist { + if let Some(required) = pkg_and_dist { // Add to the list of previously installed packages prev_installed_packages.insert(dist.name()); // Check if we need this package installed but it is not currently installed by us @@ -135,7 +135,12 @@ impl InstallPlanner { )); } else { // Check if we need to reinstall - match need_reinstall(dist, required_pkg, required_dist, &self.lock_file_dir)? { + match need_reinstall( + dist, + &required.record, + &required.dist, + &self.lock_file_dir, + )? { ValidateCurrentInstall::Keep => { if self.uv_cache.must_revalidate_package(dist.name()) { reinstalls @@ -155,7 +160,7 @@ impl InstallPlanner { // let's see if we need the remote or local version let installation_sources = installation_source::decide_installation_source( &self.uv_cache, - required_dist, + &required.dist, &mut dist_cache, Operation::Reinstall, build_options, @@ -168,7 +173,7 @@ impl InstallPlanner { } // Now we need to check if we have any packages left in the required_map - for (_name, (_pkg, dist)) in required_dists_map + for (_name, required) in required_dists_map .iter() // Only check the packages that have not been previously installed .filter(|(name, _)| !prev_installed_packages.contains(name)) @@ -178,7 +183,7 @@ impl InstallPlanner { // let's see if we need the remote or local version let installation_sources = installation_source::decide_installation_source( &self.uv_cache, - dist, + &required.dist, &mut dist_cache, Operation::Install, build_options, @@ -198,8 +203,8 @@ impl InstallPlanner { // Walk over all installed packages and check if they are required let mut extraneous = HashMap::new(); for dist in site_packages.iter() { - let pkg_and_dist = required_dists_map.get(dist.name()); - let pkg = pkg_and_dist.map(|(pkg, _dist)| *pkg); + let dist_data = required_dists_map.get(dist.name()); + let pkg = dist_data.map(|r| &r.record); let installer = dist .read_installer() .map_or(String::new(), |f| f.unwrap_or_default()); diff --git a/crates/pixi_install_pypi/src/plan/required_dists.rs b/crates/pixi_install_pypi/src/plan/required_dists.rs index df8843c504..7d851c4670 100644 --- a/crates/pixi_install_pypi/src/plan/required_dists.rs +++ b/crates/pixi_install_pypi/src/plan/required_dists.rs @@ -10,14 +10,18 @@ use std::{collections::HashMap, ops::Deref}; use uv_distribution_types::Dist; use uv_normalize::PackageName; +use crate::InstallablePypiRecord; use crate::conversions::{ConvertToUvDistError, convert_to_dist}; -use crate::{ManifestData, UnresolvedPypiRecord}; +pub struct RequiredDistData { + pub record: InstallablePypiRecord, + pub dist: Dist, +} /// A collection of required distributions with their associated package data. /// This struct owns the Dist objects to ensure proper lifetimes for the install planner. pub struct RequiredDists( /// Map from normalized package name to (UnresolvedPypiRecord, Dist) - HashMap, + HashMap, ); impl RequiredDists { @@ -29,19 +33,23 @@ impl RequiredDists { /// /// # Returns /// A RequiredDists instance or an error if conversion fails - pub fn from_packages( - packages: &[(&UnresolvedPypiRecord, &ManifestData)], + pub fn from_packages<'a>( + packages: impl Iterator, lock_file_dir: impl AsRef, ) -> Result { let mut dists = HashMap::new(); - for (pkg, manifest_data) in packages { - let pkg_data = pkg.as_package_data(); - let uv_name = PackageName::from_str(pkg_data.name.as_ref()).map_err(|_| { - ConvertToUvDistError::InvalidPackageName(pkg_data.name.as_ref().to_string()) - })?; - let dist = convert_to_dist(pkg, manifest_data, lock_file_dir.as_ref())?; - dists.insert(uv_name, ((*pkg).clone(), dist)); + for p in packages { + let uv_name = PackageName::from_str(p.name.as_ref()) + .map_err(|_| ConvertToUvDistError::InvalidPackageName(p.name.to_string()))?; + let dist = convert_to_dist(p, lock_file_dir.as_ref())?; + dists.insert( + uv_name, + RequiredDistData { + record: p.clone(), + dist, + }, + ); } Ok(Self(dists)) @@ -49,10 +57,10 @@ impl RequiredDists { /// Get a reference map suitable for passing to InstallPlanner::plan(). /// Returns a map where the values are references to the owned data. - pub fn as_ref_map(&self) -> HashMap { + pub fn as_ref_map(&self) -> HashMap { self.0 .iter() - .map(|(name, (pkg, dist))| (name.clone(), (pkg, dist))) + .map(|(name, data)| (name.clone(), data)) .collect() } @@ -63,7 +71,7 @@ impl RequiredDists { } impl Deref for RequiredDists { - type Target = HashMap; + type Target = HashMap; fn deref(&self) -> &Self::Target { &self.0 diff --git a/crates/pixi_install_pypi/src/plan/test/harness.rs b/crates/pixi_install_pypi/src/plan/test/harness.rs index 3a426fc0a1..e89c2bff3a 100644 --- a/crates/pixi_install_pypi/src/plan/test/harness.rs +++ b/crates/pixi_install_pypi/src/plan/test/harness.rs @@ -1,7 +1,7 @@ use crate::plan::InstallPlanner; use crate::plan::cache::DistCache; use crate::plan::installed_dists::InstalledDists; -use crate::{ManifestData, UnresolvedPypiRecord}; +use crate::{InstallablePypiRecord, ManifestData}; use pixi_consts::consts; use pixi_uv_conversions::GitUrlWithPrefix; use rattler_lock::{PypiPackageData, UrlOrPath}; @@ -480,7 +480,7 @@ impl<'a> DistCache<'a> for AllCached { /// Struct to create the required packages map #[derive(Default)] pub struct RequiredPackages { - required: HashMap, + required: HashMap, } impl RequiredPackages { @@ -488,13 +488,31 @@ impl RequiredPackages { Self::default() } + fn add( + &mut self, + package_name: uv_normalize::PackageName, + data: &PypiPackageData, + editable: bool, + ) { + let version_override = data + .version + .clone() + .unwrap_or_else(|| pep440_rs::MIN_VERSION.clone()); + self.required.insert( + package_name, + InstallablePypiRecord::new(data, ManifestData { editable }, version_override), + ); + } + /// Add a registry package to the required packages pub fn add_registry>(mut self, name: S, version: S) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::registry(name, version).into(); - self.required - .insert(package_name, (data, ManifestData { editable: false })); + self.add( + package_name, + &PyPIPackageDataBuilder::registry(name, version), + false, + ); self } @@ -508,44 +526,51 @@ impl RequiredPackages { ) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::path(name, version, path).into(); - self.required - .insert(package_name, (data, ManifestData { editable })); + self.add( + package_name, + &PyPIPackageDataBuilder::path(name, version, path), + editable, + ); self } pub fn add_local_wheel>(mut self, name: S, version: S, path: PathBuf) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::path(name, version, path).into(); - self.required - .insert(package_name, (data, ManifestData { editable: false })); + self.add( + package_name, + &PyPIPackageDataBuilder::path(name, version, path), + false, + ); self } pub fn add_archive>(mut self, name: S, version: S, url: Url) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::url(name, version, url, UrlType::Direct).into(); - self.required - .insert(package_name, (data, ManifestData { editable: false })); + self.add( + package_name, + &PyPIPackageDataBuilder::url(name, version, url, UrlType::Direct), + false, + ); self } pub fn add_git>(mut self, name: S, version: S, url: Url) -> Self { let package_name = uv_normalize::PackageName::from_owned(name.as_ref().to_owned()) .expect("should be correct"); - let data = PyPIPackageDataBuilder::url(name, version, url, UrlType::Other).into(); - self.required - .insert(package_name, (data, ManifestData { editable: false })); + self.add( + package_name, + &PyPIPackageDataBuilder::url(name, version, url, UrlType::Other), + false, + ); self } /// Convert to RequiredDists for the new install planner API /// Uses the default lock file directory from the test setup pub fn to_required_dists(&self) -> super::super::RequiredDists { - let packages: Vec<_> = self.required.values().map(|(p, m)| (p, m)).collect(); - super::super::RequiredDists::from_packages(&packages, default_lock_file_dir()) + super::super::RequiredDists::from_packages(self.required.values(), default_lock_file_dir()) .expect("Failed to create RequiredDists in test") } @@ -554,8 +579,7 @@ impl RequiredPackages { &self, lock_dir: impl AsRef, ) -> super::super::RequiredDists { - let packages: Vec<_> = self.required.values().map(|(p, m)| (p, m)).collect(); - super::super::RequiredDists::from_packages(&packages, lock_dir) + super::super::RequiredDists::from_packages(self.required.values(), lock_dir) .expect("Failed to create RequiredDists in test") } } diff --git a/crates/pixi_install_pypi/src/plan/validation.rs b/crates/pixi_install_pypi/src/plan/validation.rs index b7c0f89d47..b7de58027c 100644 --- a/crates/pixi_install_pypi/src/plan/validation.rs +++ b/crates/pixi_install_pypi/src/plan/validation.rs @@ -31,15 +31,14 @@ pub enum NeedsReinstallError { /// Check if a package needs to be reinstalled pub(crate) fn need_reinstall( installed_dist: &InstalledDist, - required_record: &crate::UnresolvedPypiRecord, + required_pkg: &crate::InstallablePypiRecord, required_dist: &Dist, lock_file_dir: &Path, ) -> Result { - let required_pkg = required_record.as_package_data(); // Check if the installed version is the same as the required version match &installed_dist.kind { InstalledDistKind::Registry(reg) => { - if !matches!(*required_pkg.location, UrlOrPath::Url(_)) { + if !matches!(required_pkg.location, UrlOrPath::Url(_)) { return Ok(ValidateCurrentInstall::Reinstall( NeedReinstall::SourceMismatch { locked_location: required_pkg.location.to_string(), @@ -48,18 +47,13 @@ pub(crate) fn need_reinstall( )); } - let specifier = to_uv_version( - required_pkg - .version - .as_ref() - .expect("registry packages always have a version"), - )?; + let specifier = to_uv_version(&required_pkg.version)?; if reg.version != specifier { return Ok(ValidateCurrentInstall::Reinstall( NeedReinstall::VersionMismatch { installed_version: reg.version.clone(), - locked_version: required_pkg.version_string(), + locked_version: required_pkg.version.to_string(), }, )); } @@ -92,7 +86,7 @@ pub(crate) fn need_reinstall( match result { Ok(url) => { // Convert the locked location, which can be a path or a url, to a url - let locked_url = match &*required_pkg.location { + let locked_url = match &required_pkg.location { // Fine if it is already a url UrlOrPath::Url(url) => url.clone(), // Do some path mangling if it is actually a path to get it into a url @@ -165,7 +159,7 @@ pub(crate) fn need_reinstall( let lock_file_dir = typed_path::Utf8TypedPathBuf::from( lock_file_dir.to_string_lossy().as_ref(), ); - let locked_url = match &*required_pkg.location { + let locked_url = match &required_pkg.location { // Remove `direct+` scheme if it is there so we can compare the required to // the installed url UrlOrPath::Url(url) => strip_direct_scheme(url).into_owned(), @@ -231,7 +225,7 @@ pub(crate) fn need_reinstall( // Try to parse the locked git url, this can be any url, so this may fail // in practice it always seems to succeed, even with a non-git url - let locked_git_url = match &*required_pkg.location { + let locked_git_url = match &required_pkg.location { UrlOrPath::Url(url) => { // is it a git url? if LockedGitUrl::is_locked_git_url(url) { From e331393f9153dbf18e24e26321558127803d50ea Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Mon, 16 Mar 2026 13:37:33 +0000 Subject: [PATCH 3/4] refactor: Introduce LockedPypiRecord --- .../pixi/tests/integration_rust/pypi_tests.rs | 69 +++++++++++++++++++ crates/pixi_core/src/lock_file/mod.rs | 4 +- .../src/lock_file/records_by_name.rs | 15 +++- .../pixi_core/src/lock_file/resolve/pypi.rs | 60 ++++++++-------- crates/pixi_core/src/lock_file/update.rs | 53 ++++++++------ crates/pixi_install_pypi/src/lib.rs | 21 ++++++ 6 files changed, 171 insertions(+), 51 deletions(-) diff --git a/crates/pixi/tests/integration_rust/pypi_tests.rs b/crates/pixi/tests/integration_rust/pypi_tests.rs index 757148c232..d3b035dbf2 100644 --- a/crates/pixi/tests/integration_rust/pypi_tests.rs +++ b/crates/pixi/tests/integration_rust/pypi_tests.rs @@ -335,6 +335,75 @@ setup(version="42.23.12") _ => panic!("expected a pypi package"), } + // Make the version static: remove `dynamic = ["version"]` and set an + // explicit version. The lock file should still store version as None + // because the package is a local source dependency. + fs_err::write( + pixi.workspace_path().join("dynamic-dep/pyproject.toml"), + r#"[build-system] +requires = ["setuptools"] +build-backend = "setuptools.build_meta" + +[project] +name = "dynamic-dep" +version = "42.23.12" +"#, + ) + .unwrap(); + + let lock = pixi.update_lock_file().await.unwrap(); + + match lock + .get_pypi_package("default", platform, "dynamic-dep") + .expect("dynamic-dep should be in the lock file after making version static") + { + rattler_lock::LockedPackageRef::Pypi(data) => { + assert!( + data.version.is_none(), + "version should remain None for local source dependency even after making version static, got {:?}", + data.version + ); + } + _ => panic!("expected a pypi package"), + } + + // Switch back to a dynamic version and re-resolve. + fs_err::write( + pixi.workspace_path().join("dynamic-dep/pyproject.toml"), + r#"[build-system] +requires = ["setuptools"] +build-backend = "setuptools.build_meta" + +[project] +name = "dynamic-dep" +dynamic = ["version"] +"#, + ) + .unwrap(); + fs_err::write( + pixi.workspace_path().join("dynamic-dep/setup.py"), + r#"from setuptools import setup +setup(version="99.0.0") +"#, + ) + .unwrap(); + + let lock = pixi.update_lock_file().await.unwrap(); + + match lock + .get_pypi_package("default", platform, "dynamic-dep") + .expect("dynamic-dep should be in the lock file after switching back to dynamic version") + { + rattler_lock::LockedPackageRef::Pypi(data) => { + assert!( + data.version.is_none(), + "version should be None after switching back to dynamic version, got {:?}", + data.version + ); + } + _ => panic!("expected a pypi package"), + } + // Round-trip: serialize and parse the lock file, then verify the version is still None let lock_str = lock.render_to_string().unwrap(); let lock2 = rattler_lock::LockFile::from_str_with_base_directory(&lock_str, None).unwrap(); diff --git a/crates/pixi_core/src/lock_file/mod.rs b/crates/pixi_core/src/lock_file/mod.rs index 7606b7e0ea..b75f6718f7 100644 --- a/crates/pixi_core/src/lock_file/mod.rs +++ b/crates/pixi_core/src/lock_file/mod.rs @@ -12,7 +12,7 @@ pub mod virtual_packages; pub use crate::environment::CondaPrefixUpdater; pub use install_subset::{FilteredPackages, InstallSubset}; pub use package_identifier::PypiPackageIdentifier; -use pixi_install_pypi::UnresolvedPypiRecord; +use pixi_install_pypi::LockedPypiRecord; use pixi_record::PixiRecord; pub use pixi_uv_context::UvResolutionContext; pub use rattler_lock::Verbatim; @@ -36,7 +36,7 @@ pub use utils::IoConcurrencyLimit; pub type LockedCondaPackages = Vec; /// A list of Pypi packages that are locked for a specific platform. -pub type LockedPypiPackages = Vec; +pub type LockedPypiRecords = Vec; #[cfg(test)] mod tests { diff --git a/crates/pixi_core/src/lock_file/records_by_name.rs b/crates/pixi_core/src/lock_file/records_by_name.rs index 00c763caab..980880a985 100644 --- a/crates/pixi_core/src/lock_file/records_by_name.rs +++ b/crates/pixi_core/src/lock_file/records_by_name.rs @@ -1,5 +1,5 @@ use super::package_identifier::ConversionError; -use crate::lock_file::PypiPackageIdentifier; +use crate::lock_file::{LockedPypiRecord, PypiPackageIdentifier}; use pixi_install_pypi::UnresolvedPypiRecord; use pixi_record::{PixiRecord, UnresolvedPixiRecord}; use pixi_uv_conversions::to_uv_normalize; @@ -10,6 +10,7 @@ use std::collections::hash_map::Entry; use std::hash::Hash; pub type PypiRecordsByName = DependencyRecordsByName; +pub type LockedPypiRecordsByName = DependencyRecordsByName; pub type PixiRecordsByName = DependencyRecordsByName; pub type UnresolvedPixiRecordsByName = DependencyRecordsByName; @@ -27,6 +28,18 @@ pub trait HasNameVersion { fn version(&self) -> Option<&Self::V>; } +impl HasNameVersion for LockedPypiRecord { + type N = pep508_rs::PackageName; + type V = pep440_rs::Version; + + fn name(&self) -> &pep508_rs::PackageName { + &self.data.name + } + fn version(&self) -> Option<&Self::V> { + Some(&self.locked_version) + } +} + impl HasNameVersion for UnresolvedPypiRecord { type N = pep508_rs::PackageName; type V = pep440_rs::Version; diff --git a/crates/pixi_core/src/lock_file/resolve/pypi.rs b/crates/pixi_core/src/lock_file/resolve/pypi.rs index b0cb9ce12d..443e921c43 100644 --- a/crates/pixi_core/src/lock_file/resolve/pypi.rs +++ b/crates/pixi_core/src/lock_file/resolve/pypi.rs @@ -64,7 +64,7 @@ use uv_types::EmptyInstalledPackages; use crate::{ environment::CondaPrefixUpdated, lock_file::{ - CondaPrefixUpdater, LockedPypiPackages, PixiRecordsByName, PypiPackageIdentifier, + CondaPrefixUpdater, LockedPypiRecords, PixiRecordsByName, PypiPackageIdentifier, records_by_name::HasNameVersion, resolve::{ build_dispatch::{ @@ -297,7 +297,7 @@ pub async fn resolve_pypi( disallow_install_conda_prefix: bool, exclude_newer: Option>, solve_strategy: SolveStrategy, -) -> miette::Result<(LockedPypiPackages, Option)> { +) -> miette::Result<(LockedPypiRecords, Option)> { // Solve python packages pb.set_message("resolving pypi dependencies"); @@ -795,13 +795,7 @@ pub async fn resolve_pypi( // We try to distinguish between build dispatch panics and any other panics that occur let (locked_packages, conda_task) = match resolution_future.catch_unwind().await { - Ok(result) => { - let result = result?; - ( - result.0.iter().map(|r| r.clone().into()).collect(), - result.1, - ) - } + Ok(result) => result?, Err(panic_payload) => { // Try to get the stored initialization error from the last_error holder if let Some(stored_error) = last_error.get() { @@ -954,8 +948,8 @@ async fn lock_pypi_packages( concurrent_downloads: usize, abs_project_root: &Path, original_git_references: &HashMap, -) -> miette::Result { - let mut locked_packages = LockedPypiPackages::with_capacity(resolution.len()); +) -> miette::Result { + let mut locked_packages = Vec::with_capacity(resolution.len()); let database = DistributionDatabase::new(registry_client, pixi_build_dispatch, concurrent_downloads); for dist in resolution.distributions() { @@ -964,7 +958,7 @@ async fn lock_pypi_packages( continue; } - let pypi_package_data = match dist { + let locked_pypi_package_data = match dist { // Ignore installed distributions ResolvedDist::Installed { .. } => { continue; @@ -1014,15 +1008,17 @@ async fn lock_pypi_packages( .await .into_diagnostic() .wrap_err("cannot get wheel metadata")?; - PypiPackageData { + + let locked_version = + pep440_rs::Version::from_str(&metadata.version.to_string()) + .into_diagnostic() + .context("cannot convert version")?; + + UnresolvedPypiRecord::from(PypiPackageData { name: pep508_rs::PackageName::new(metadata.name.to_string()) .into_diagnostic() .context("cannot convert name")?, - version: Some( - pep440_rs::Version::from_str(&metadata.version.to_string()) - .into_diagnostic() - .context("cannot convert version")?, - ), + version: None, requires_python: metadata .requires_python .map(|r| to_version_specifiers(&r)) @@ -1035,7 +1031,11 @@ async fn lock_pypi_packages( location: Verbatim::new(location), hash, index_url, - } + }) + .lock( + locked_version, + metadata.dynamic || matches!(dist, BuiltDist::Path(_)), + ) } Dist::Source(source) => { // Handle new hash stuff @@ -1155,14 +1155,13 @@ async fn lock_pypi_packages( } }; - PypiPackageData { + let locked_version = + pep440_rs::Version::from_str(&metadata.version.to_string()) + .into_diagnostic()?; + + UnresolvedPypiRecord::from(PypiPackageData { name: to_normalize(&metadata.name).into_diagnostic()?, - version: (!metadata.dynamic) - .then(|| { - pep440_rs::Version::from_str(&metadata.version.to_string()) - .into_diagnostic() - }) - .transpose()?, + version: None, requires_python: metadata .requires_python .map(|r| to_version_specifiers(&r)) @@ -1173,12 +1172,17 @@ async fn lock_pypi_packages( .into_diagnostic()?, hash, index_url, - } + }) + .lock( + locked_version, + metadata.dynamic + || matches!(source, SourceDist::Path(_) | SourceDist::Directory(_)), + ) } }, }; - locked_packages.push(pypi_package_data.into()); + locked_packages.push(locked_pypi_package_data); } Ok(locked_packages) diff --git a/crates/pixi_core/src/lock_file/update.rs b/crates/pixi_core/src/lock_file/update.rs index 3b1bd642e1..7dc5c61afc 100644 --- a/crates/pixi_core/src/lock_file/update.rs +++ b/crates/pixi_core/src/lock_file/update.rs @@ -10,7 +10,10 @@ use std::{ time::{Duration, Instant}, }; -use crate::lock_file::records_by_name::HasNameVersion; +use crate::lock_file::{ + LockedPypiRecord, + records_by_name::{HasNameVersion, LockedPypiRecordsByName}, +}; use barrier_cell::BarrierCell; use dashmap::DashMap; use fancy_display::FancyDisplay; @@ -29,7 +32,7 @@ use pixi_consts::consts; use pixi_glob::GlobHashCache; use pixi_install_pypi::{ LazyEnvironmentVariables, PyPIBuildConfig, PyPIContextConfig, PyPIEnvironmentUpdater, - PyPIUpdateConfig, UnresolvedPypiRecord, + PyPIUpdateConfig, }; use pixi_manifest::{ChannelPriority, EnvironmentName, FeaturesExt}; use pixi_progress::global_multi_progress; @@ -1276,10 +1279,10 @@ pub struct UpdateContext<'p> { /// mapping contains a [`BarrierCell`] that will eventually contain the /// solved records computed by another task. This allows tasks to wait /// for the records to be solved before proceeding. - solved_pypi_records: PerEnvironmentAndPlatform<'p, Arc>>, + solved_pypi_records: PerEnvironmentAndPlatform<'p, Arc>>, /// Keeps track of all pending grouped pypi targets that are being solved. - grouped_solved_pypi_records: PerGroupAndPlatform<'p, Arc>>, + grouped_solved_pypi_records: PerGroupAndPlatform<'p, Arc>>, /// The package cache to use when instantiating prefixes. package_cache: PackageCache, @@ -1351,7 +1354,7 @@ impl<'p> UpdateContext<'p> { &self, group: &GroupedEnvironment<'p>, platform: Platform, - ) -> Option> + use<>> { + ) -> Option> + use<>> { // Check if there is a pending operation for this group and platform if let Some(pending_records) = self .grouped_solved_pypi_records @@ -1410,7 +1413,7 @@ impl<'p> UpdateContext<'p> { &mut self, environment: &Environment<'p>, platform: Platform, - ) -> Option { + ) -> Option { self.solved_pypi_records .get_mut(environment) .and_then(|records| records.remove(&platform)) @@ -1424,6 +1427,20 @@ impl<'p> UpdateContext<'p> { self.locked_pypi_records .get_mut(environment) .and_then(|records| records.remove(&platform)) + .map(|unresolved| { + Arc::new(LockedPypiRecordsByName::from_iter( + unresolved.records.iter().map(|ur| { + ur.lock( + ur.as_package_data() + .version + .as_ref() + .unwrap_or(&pep440_rs::MIN_VERSION) + .clone(), + ur.as_package_data().version.is_some(), + ) + }), + )) + }) }) .map(|records| Arc::try_unwrap(records).unwrap_or_else(|arc| (*arc).clone())) } @@ -2376,13 +2393,9 @@ impl<'p> UpdateContext<'p> { } } if let Some(records) = self.take_latest_pypi_records(&environment, platform) { - for pkg_data in records.into_inner() { + for r in records.into_inner() { builder - .add_pypi_package( - &environment_name, - &platform_str, - pkg_data.as_package_data().clone(), - ) + .add_pypi_package(&environment_name, &platform_str, r.data.clone()) .expect("platform was registered"); has_pypi_records = true; } @@ -2478,7 +2491,7 @@ pub enum TaskResult { PypiGroupSolved( GroupedEnvironmentName, Platform, - PypiRecordsByName, + LockedPypiRecordsByName, Duration, Option, ), @@ -2489,7 +2502,7 @@ pub enum TaskResult { EnvironmentName, Platform, Arc, - Arc, + Arc, ), } @@ -2667,7 +2680,7 @@ async fn spawn_extract_environment_task( environment: Environment<'_>, platform: Platform, grouped_repodata_records: impl Future>, - grouped_pypi_records: impl Future>, + grouped_pypi_records: impl Future>, command_dispatcher: CommandDispatcher, ) -> miette::Result { let group = GroupedEnvironment::from(environment.clone()); @@ -2698,7 +2711,7 @@ async fn spawn_extract_environment_task( enum PackageRecord<'a> { Conda(&'a PixiRecord), - Pypi((&'a UnresolvedPypiRecord, Option)), + Pypi((&'a LockedPypiRecord, Option)), } // Determine the conda packages we need. @@ -2846,7 +2859,7 @@ async fn spawn_extract_environment_task( .into_diagnostic()? .unwrap_or_default(); - for req in record.as_package_data().requires_dist.iter() { + for req in record.data.requires_dist.iter() { // Evaluate the marker environment with the given extras if let Some(marker_env) = &marker_environment { // let marker_str = marker_env.to_string(); @@ -2890,7 +2903,7 @@ async fn spawn_extract_environment_task( Arc::new(PixiRecordsByName::from_iter( pixi_records.into_iter().cloned(), )), - Arc::new(PypiRecordsByName::from_iter( + Arc::new(LockedPypiRecordsByName::from_iter( pypi_records.into_values().cloned(), )), )) @@ -2918,7 +2931,7 @@ async fn spawn_solve_pypi_task<'p>( return Ok(TaskResult::PypiGroupSolved( grouped_environment.name().clone(), platform, - PypiRecordsByName::default(), + LockedPypiRecordsByName::default(), Duration::from_millis(0), None, )); @@ -3002,7 +3015,7 @@ async fn spawn_solve_pypi_task<'p>( pb.finish(); Ok::<(_, _, _), miette::Report>(( - PypiRecordsByName::from_iter(records), + LockedPypiRecordsByName::from_iter(records), end - start, prefix_task_result, )) diff --git a/crates/pixi_install_pypi/src/lib.rs b/crates/pixi_install_pypi/src/lib.rs index 5b1bf10fd0..0b4ff70dcc 100644 --- a/crates/pixi_install_pypi/src/lib.rs +++ b/crates/pixi_install_pypi/src/lib.rs @@ -58,6 +58,12 @@ pub struct ManifestData { pub editable: bool, } +#[derive(Clone)] +pub struct LockedPypiRecord { + pub data: PypiPackageData, + pub locked_version: pep440_rs::Version, +} + #[derive(Clone, Debug)] pub struct UnresolvedPypiRecord(PypiPackageData); @@ -71,6 +77,21 @@ impl UnresolvedPypiRecord { pub fn as_package_data(&self) -> &PypiPackageData { &self.0 } + + pub fn lock( + &self, + locked_version: pep440_rs::Version, + version_is_dynamic: bool, + ) -> LockedPypiRecord { + let mut data = self.0.clone(); + + data.version = (!version_is_dynamic).then_some(locked_version.clone()); + + LockedPypiRecord { + data, + locked_version, + } + } } #[derive(Clone)] From ea801b03d21195c9fcb9f38fff3eda01504a6291 Mon Sep 17 00:00:00 2001 From: Tobias Hunger Date: Tue, 17 Mar 2026 16:21:34 +0000 Subject: [PATCH 4/4] Add more tests that need the newest rattler We no longer store `editable` for pypi projects, so we do not need to check whether the editable state from the projects wins over it. --- .../pixi/tests/integration_rust/pypi_tests.rs | 227 +++++------------- 1 file changed, 65 insertions(+), 162 deletions(-) diff --git a/crates/pixi/tests/integration_rust/pypi_tests.rs b/crates/pixi/tests/integration_rust/pypi_tests.rs index d3b035dbf2..7a6a3b76a8 100644 --- a/crates/pixi/tests/integration_rust/pypi_tests.rs +++ b/crates/pixi/tests/integration_rust/pypi_tests.rs @@ -1,4 +1,4 @@ -use std::{fs::File, io::Write, path::Path, str::FromStr}; +use std::{fs::File, io::Write, str::FromStr}; use pep508_rs::Requirement; use rattler_conda_types::Platform; @@ -10,60 +10,6 @@ use crate::common::{LockFileExt, PixiControl}; use crate::setup_tracing; use pixi_test_utils::{MockRepoData, Package}; -/// Helper to check if a pypi package is installed as editable by looking for a .pth file. -/// Editable installations create a .pth file in site-packages that points to the source directory. -/// For most backends, make sure to use the old style editables when using this function -fn has_editable_pth_file(prefix: &Path, package_name: &str) -> bool { - let site_packages = if cfg!(target_os = "windows") { - prefix.join("Lib").join("site-packages") - } else { - // Find the python version directory - let lib_dir = prefix.join("lib"); - if let Ok(entries) = fs_err::read_dir(&lib_dir) { - let entry = entries - .filter_map(|e| e.ok()) - // Find the directory that starts with "python" - .find(|e| e.file_name().to_string_lossy().starts_with("python")) - .map(|e| e.path().join("site-packages")); - if let Some(entry) = entry { - entry - } else { - panic!("expected site-packages folder"); - } - } else { - panic!("expected lib folder"); - } - }; - - // Look for editable .pth files - different build backends use different naming: - // - hatchling: _{package_name}.pth (e.g., _editable_test.pth) - // - setuptools: __editable__.{package_name}-{version}.pth - let mut count = 0u32; - let normalized_name = package_name.replace('-', "_"); - if let Ok(entries) = fs_err::read_dir(&site_packages) { - for entry in entries.flatten() { - count += 1; - let name = entry.file_name(); - let name_str = name.to_string_lossy(); - if name_str.ends_with(".pth") { - // Check for hatchling style: _{package_name}.pth - if name_str == format!("_{}.pth", normalized_name) { - return true; - } - // Check for setuptools style: __editable__.{package_name}-*.pth - if name_str.starts_with(&format!("__editable__.{}", normalized_name)) { - return true; - } - } - } - } - if count == 0 { - panic!("expected folders in directory"); - } - - false -} - /// This tests if we can resolve pyproject optional dependencies recursively /// before when running `pixi list -e all`, this would have not included numpy /// we are now explicitly testing that this works @@ -474,6 +420,70 @@ version = "1.0.0" } _ => panic!("expected a pypi package"), } + + // Trigger a re-resolve so that update_lock_file writes a new lock file + // that includes another-dep. Then verify the lock file can be loaded + // again — this catches URL mismatches between the environment reference + // (e.g. "./another-dep") and the packages section (e.g. + // "file:///tmp/.../another-dep"). + fs_err::write( + pixi.workspace_path().join("dynamic-dep/pyproject.toml"), + r#"[build-system] +requires = ["setuptools"] +build-backend = "setuptools.build_meta" + +[project] +name = "dynamic-dep" +version = "50.0.0" +"#, + ) + .unwrap(); + + let lock = pixi.update_lock_file().await.unwrap(); + + // The lock file written by the re-resolve must be loadable. + let lock_reloaded = pixi + .lock_file() + .await + .expect("lock file written by update_lock_file should be loadable"); + + // Both packages should be present after the round-trip through disk. + assert!( + lock_reloaded.contains_pypi_package("default", platform, "dynamic-dep"), + "dynamic-dep should be present after reload" + ); + assert!( + lock_reloaded.contains_pypi_package("default", platform, "another-dep"), + "another-dep should be present after reload" + ); + + // Verify the in-memory lock also has both packages with correct properties. + match lock + .get_pypi_package("default", platform, "dynamic-dep") + .expect("dynamic-dep should be in the re-resolved lock") + { + rattler_lock::LockedPackageRef::Pypi(data) => { + assert!( + data.version.is_none(), + "dynamic-dep version should be None, got {:?}", + data.version + ); + } + _ => panic!("expected a pypi package"), + } + match lock + .get_pypi_package("default", platform, "another-dep") + .expect("another-dep should be in the re-resolved lock") + { + rattler_lock::LockedPackageRef::Pypi(data) => { + assert!( + data.version.is_none(), + "another-dep version should be None for local source dep, got {:?}", + data.version + ); + } + _ => panic!("expected a pypi package"), + } } #[tokio::test] @@ -1687,113 +1697,6 @@ test-project = {{ path = "." }} } } -/// Test that when a lock file has editable: true but the manifest doesn't specify editable, -/// the package is installed as non-editable (manifest takes precedence). -/// -/// This tests the fix for the bug where old lock files with editable: true would cause -/// packages to be installed as editable even when the manifest didn't specify it. -#[tokio::test] -#[cfg_attr( - any(not(feature = "online_tests"), not(feature = "slow_integration_tests")), - ignore -)] -async fn test_editable_from_manifest_not_lockfile() { - use rattler_lock::LockFile; - - setup_tracing(); - - let platform = Platform::current(); - - // Create a project with a path dependency WITHOUT editable specified - // Use conda-forge directly since we need a real Python - let pixi = PixiControl::from_manifest(&format!( - r#" - [workspace] - name = "editable-test" - platforms = ["{platform}"] - channels = ["https://prefix.dev/conda-forge"] - - [dependencies] - python = "~=3.12.0" - - [pypi-dependencies] - editable-test = {{ path = "." }} - "#, - platform = platform, - )) - .unwrap(); - - // Create a minimal pyproject.toml for the package - let pyproject = r#" -[build-system] -requires = ["hatchling"] -build-backend = "hatchling.build" - -[project] -name = "editable-test" -version = "0.1.0" -"#; - fs_err::write(pixi.workspace_path().join("pyproject.toml"), pyproject).unwrap(); - - // Create the package source - let src_dir = pixi.workspace_path().join("editable_test"); - fs_err::create_dir_all(&src_dir).unwrap(); - fs_err::write(src_dir.join("__init__.py"), "").unwrap(); - - // First, update the lock file (this won't have editable field since we don't record it) - let lock = pixi.update_lock_file().await.unwrap(); - - // Path-based source package should not have index_url - match lock - .get_pypi_package("default", platform, "editable-test") - .expect("editable-test should be in the lock file") - { - rattler_lock::LockedPackageRef::Pypi(data) => { - assert!( - data.index_url.is_none(), - "path-based source package should not have index_url, got: {:?}", - data.index_url - ); - } - _ => panic!("expected a pypi package"), - } - - // Manually modify the lock file to add editable: true, simulating an old lock file - let lock_file_str = lock.render_to_string().unwrap(); - - // Add editable: true after the package name line - let modified_lock_file_str = lock_file_str.replace( - "name: editable-test\n", - "name: editable-test\n editable: true\n", - ); - - assert!( - modified_lock_file_str.contains("editable: true"), - "Failed to add editable: true to lock file" - ); - - // Parse and write the modified lock file back - let modified_lockfile = - LockFile::from_str_with_base_directory(&modified_lock_file_str, None).unwrap(); - let workspace = pixi.workspace().unwrap(); - modified_lockfile - .to_path(&workspace.lock_file_path()) - .unwrap(); - - // Now install with --locked (uses the modified lock file without re-resolving) - // The fix should ensure that the package is installed as NON-editable - // because the manifest doesn't specify editable = true - pixi.install().with_locked().await.unwrap(); - - let prefix_path = pixi.default_env_path().unwrap(); - - // The package should NOT be installed as editable because the manifest doesn't specify editable - assert!( - !has_editable_pth_file(&prefix_path, "editable_test"), - "Package should NOT be installed as editable when manifest doesn't specify editable = true (even if lock file has editable: true)" - ); -} - /// Test that packages from different indexes get distinct `index_url` values /// recorded in the lock file. #[tokio::test]