diff --git a/Cargo.lock b/Cargo.lock index 727e3e3801..d32d76f6fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5487,6 +5487,7 @@ dependencies = [ "url", "uv-build-frontend", "uv-cache", + "uv-cache-key", "uv-client", "uv-configuration", "uv-dispatch", diff --git a/Cargo.toml b/Cargo.toml index 6ddf11bc44..fef1488ecc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -190,6 +190,7 @@ uv-auth = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } uv-build-frontend = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } uv-cache = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } uv-cache-info = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } +uv-cache-key = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } uv-client = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } uv-configuration = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } uv-dispatch = { git = "https://github.com/astral-sh/uv", tag = "0.9.5" } diff --git a/crates/pixi/tests/integration_rust/add_tests.rs b/crates/pixi/tests/integration_rust/add_tests.rs index 64599a9b8f..d8337c8ba6 100644 --- a/crates/pixi/tests/integration_rust/add_tests.rs +++ b/crates/pixi/tests/integration_rust/add_tests.rs @@ -1095,6 +1095,131 @@ platforms = ["{platform}"] }); } +/// Test that git branch references are correctly preserved in lock files. +/// This tests issue #5185: git references should be preserved across lock file updates. +#[tokio::test] +#[cfg_attr(not(feature = "online_tests"), ignore)] +async fn test_pypi_git_branch_preservation() { + setup_tracing(); + + let pixi = PixiControl::from_manifest( + format!( + r#" +[workspace] +name = "test-git-branch-preservation" +channels = ["https://prefix.dev/conda-forge"] +platforms = ["{platform}"] + +[dependencies] +python = ">=3.13" +"#, + platform = Platform::current() + ) + .as_str(), + ) + .unwrap(); + + // Step 1: Add git dependency WITHOUT branch + pixi.add("boltons") + .set_pypi(true) + .with_git_url(Url::parse("https://github.com/mahmoud/boltons.git").unwrap()) + .await + .unwrap(); + + // Verify: lock file should NOT have ?branch= when no branch specified + let lock = pixi.lock_file().await.unwrap(); + let url1 = lock + .default_environment() + .unwrap() + .pypi_packages(Platform::current()) + .unwrap() + .find(|(p, _)| p.name.to_string() == "boltons") + .map(|(p, _)| p.location.to_string()) + .unwrap(); + assert!( + !url1.contains("branch="), + "Should not have branch= without manifest branch, got: {url1}" + ); + + // Step 2: Update manifest to ADD branch = "master" (boltons uses master, not main) + pixi.update_manifest( + format!( + r#" +[workspace] +name = "test-git-branch-preservation" +channels = ["https://prefix.dev/conda-forge"] +platforms = ["{platform}"] + +[dependencies] +python = ">=3.13" + +[pypi-dependencies] +boltons = {{ git = "https://github.com/mahmoud/boltons.git", branch = "master" }} +"#, + platform = Platform::current() + ) + .as_str(), + ) + .unwrap(); + + // Re-lock to update + pixi.lock().await.unwrap(); + + // Verify: lock file should have ?branch=master + let lock = pixi.lock_file().await.unwrap(); + let url2 = lock + .default_environment() + .unwrap() + .pypi_packages(Platform::current()) + .unwrap() + .find(|(p, _)| p.name.to_string() == "boltons") + .map(|(p, _)| p.location.to_string()) + .unwrap(); + assert!( + url2.contains("branch=master"), + "Should have branch=master after adding to manifest, got: {url2}" + ); + + // Step 3: Update manifest to REMOVE branch + pixi.update_manifest( + format!( + r#" +[workspace] +name = "test-git-branch-preservation" +channels = ["https://prefix.dev/conda-forge"] +platforms = ["{platform}"] + +[dependencies] +python = ">=3.13" + +[pypi-dependencies] +boltons = {{ git = "https://github.com/mahmoud/boltons.git" }} +"#, + platform = Platform::current() + ) + .as_str(), + ) + .unwrap(); + + // Re-lock to update + pixi.lock().await.unwrap(); + + // Verify: lock file should NOT have branch=main anymore + let lock = pixi.lock_file().await.unwrap(); + let url3 = lock + .default_environment() + .unwrap() + .pypi_packages(Platform::current()) + .unwrap() + .find(|(p, _)| p.name.to_string() == "boltons") + .map(|(p, _)| p.location.to_string()) + .unwrap(); + assert!( + !url3.contains("branch="), + "Should not have branch= after removing from manifest, got: {url3}" + ); +} + #[tokio::test] async fn add_git_dependency_without_preview_feature_fails() { setup_tracing(); diff --git a/crates/pixi_core/Cargo.toml b/crates/pixi_core/Cargo.toml index f55d26176b..e67a764571 100644 --- a/crates/pixi_core/Cargo.toml +++ b/crates/pixi_core/Cargo.toml @@ -79,6 +79,7 @@ typed-path = { workspace = true } url = { workspace = true } uv-build-frontend = { workspace = true } uv-cache = { workspace = true } +uv-cache-key = { workspace = true } uv-client = { workspace = true } uv-configuration = { workspace = true } uv-dispatch = { workspace = true } diff --git a/crates/pixi_core/src/lock_file/resolve/pypi.rs b/crates/pixi_core/src/lock_file/resolve/pypi.rs index 991412bf62..0674f3adf3 100644 --- a/crates/pixi_core/src/lock_file/resolve/pypi.rs +++ b/crates/pixi_core/src/lock_file/resolve/pypi.rs @@ -18,7 +18,6 @@ use indicatif::ProgressBar; use itertools::{Either, Itertools}; use miette::{Context, IntoDiagnostic}; use pixi_consts::consts; -use pixi_git::git::GitReference; use pixi_manifest::{ EnvironmentName, SolveStrategy, SystemRequirements, pypi::pypi_options::PypiOptions, }; @@ -27,9 +26,10 @@ use pixi_record::{LockedGitUrl, PixiRecord}; use pixi_reporters::{UvReporter, UvReporterOptions}; use pixi_uv_conversions::{ ConversionError, as_uv_req, configure_insecure_hosts_for_tls_bypass, - convert_uv_requirements_to_pep508, into_pinned_git_spec, pypi_options_to_build_options, - pypi_options_to_index_locations, to_exclude_newer, to_index_strategy, to_normalize, - to_prerelease_mode, to_requirements, to_uv_normalize, to_uv_version, to_version_specifiers, + convert_uv_requirements_to_pep508, into_pinned_git_spec, into_uv_git_reference, + into_uv_git_sha, pypi_options_to_build_options, pypi_options_to_index_locations, + to_exclude_newer, to_index_strategy, to_normalize, to_prerelease_mode, to_requirements, + to_uv_normalize, to_uv_version, to_version_specifiers, }; use pypi_modifiers::{ pypi_marker_env::determine_marker_environment, @@ -41,6 +41,7 @@ use rattler_lock::{ }; use typed_path::Utf8TypedPathBuf; use url::Url; +use uv_cache_key::RepositoryUrl; use uv_client::{ BaseClientBuilder, Connectivity, FlatIndexClient, RegistryClient, RegistryClientBuilder, }; @@ -51,10 +52,8 @@ use uv_distribution_types::{ IndexCapabilities, IndexUrl, Name, RequirementSource, RequiresPython, Resolution, ResolvedDist, SourceDist, ToUrlError, }; -use uv_git_types::GitUrl; -use uv_pep508::VerbatimUrl; +use uv_git::RepositoryReference; use uv_pypi_types::{Conflicts, HashAlgorithm, HashDigests}; -use uv_redacted::DisplaySafeUrl; use uv_requirements::LookaheadResolver; use uv_resolver::{ AllowedYanks, DefaultResolverProvider, FlatIndex, InMemoryIndex, Manifest, Options, Preference, @@ -347,7 +346,51 @@ pub async fn resolve_pypi( tracing::info!("there are no python packages installed by conda"); } - let mut requirements = dependencies + // Build a lookup map of original git references before consuming dependencies. + // This is used later to preserve branch/tag info in the lock file that uv normalizes away. + let original_git_references = dependencies + .iter() + .filter_map(|(name, specs)| { + specs.iter().find_map(|spec| { + spec.source + .as_git() + .and_then(|git_spec| git_spec.rev.clone()) + .map(|rev| (name.clone(), rev)) + }) + }) + .collect(); + + // Pre-populate the git resolver with locked git references. + // 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() + && LockedGitUrl::is_locked_git_url(location) + { + let locked_url = LockedGitUrl::new(location.clone()); + if let Ok(pinned_git_spec) = locked_url.to_pinned_git_spec() { + // Convert pixi types to uv types and insert into the git resolver + // pixi_spec::GitReference -> pixi_git::git::GitReference -> uv_git_types::GitReference + let pixi_git_ref = pinned_git_spec.source.reference.clone().into(); + + let uv_reference = into_uv_git_reference(pixi_git_ref); + let uv_sha = into_uv_git_sha(pinned_git_spec.source.commit); + + let display_safe_url = pinned_git_spec.git.clone().into(); + + let repository_url = RepositoryUrl::new(&display_safe_url); + let reference = RepositoryReference { + url: repository_url, + reference: uv_reference, + }; + + tracing::debug!("Pre-populating git resolver: {:?} -> {}", reference, uv_sha); + context.shared_state.git().insert(reference, uv_sha); + } + } + } + + let requirements = dependencies .into_iter() .flat_map(|(name, req)| { req.into_iter() @@ -572,9 +615,6 @@ pub async fn resolve_pypi( #[error(transparent)] GitUrlParse(#[from] uv_git_types::GitUrlParseError), - - #[error("{0}")] - Other(miette::ErrReport), } // Create preferences from the locked pypi packages @@ -597,69 +637,26 @@ pub async fn resolve_pypi( origin: None, }; - // When iterating over locked packages, - // instead of adding git requirements as preferences - // we enrich previous defined requirements in the `requirements` list - // as they have been pinned to a precise commit - // This will help the resolver to pick the commit that we already have locked - // instead of updating to a newer commit that also matches the requirement. - if let Some(location) = package_data.location.as_url() { - // now check if it's a git url - if LockedGitUrl::is_locked_git_url(location) { - // we need to parse back `LockedGitUrl` in order to get the `PinnedGitSpec` - // then we will precise commit to set for the `GitUrl` - // that will be used in the `RequirementSource::Git` below - let git_locked_url = LockedGitUrl::from(location.clone()); - let pinned_git_spec = git_locked_url - .to_pinned_git_spec() - .map_err(PixiPreferencesError::Other)?; - // we need to create VerbatimUrl from the original location - let verbatim_url = VerbatimUrl::from(location.clone()); - - // but the display safe url should come from the `PinnedGitSpec` url - // which don't have anymore git+ prefix - let display_safe = DisplaySafeUrl::from(pinned_git_spec.git.clone()); - - let git_oid = - uv_git_types::GitOid::from_str(&pinned_git_spec.source.commit.to_string())?; - - let git_url = GitUrl::try_from(display_safe)?.with_precise(git_oid); - - let constraint_source = RequirementSource::Git { - git: git_url, - subdirectory: None, - url: verbatim_url, - }; + // For git packages, we don't add them as preferences. + // 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) = package_data.location.as_url() + && LockedGitUrl::is_locked_git_url(location) + { + // Skip git packages - they'll be resolved based on manifest reference + // with the cached commit from the git resolver + return Ok(None); + } - // find this requirements in dependencies and skip adding it as preference - let req_from_dep = requirements.iter_mut().find(|r| r.name == requirement.name); - if let Some(req) = req_from_dep { - // we need to update the requirement source in the requirements list - // to use the precise git commit - // only if the requirements do not already have a source set with something specific - if let RequirementSource::Git { git, .. } = &req.source { - // only update if the git url does not already have a precise commit - if git.precise().is_none() && !GitReference::looks_like_commit_hash(git.reference().as_rev()) { - tracing::debug!( - "updating requirement source to precise git commit for requirement: {:?}", - &req - ); - req.source = constraint_source.clone(); - } - - } - } - } - Ok(None) - } else { - let named = uv_requirements_txt::RequirementsTxtRequirement::Named(requirement); - let entry = uv_requirements_txt::RequirementEntry { - requirement: named, - hashes: Default::default(), - }; + // Create preference for registry and URL packages + let named = uv_requirements_txt::RequirementsTxtRequirement::Named(requirement); + let entry = uv_requirements_txt::RequirementEntry { + requirement: named, + hashes: Default::default(), + }; - Ok(Preference::from_entry(entry)?) - } + Ok(Preference::from_entry(entry)?) }) .filter_map(|pref| pref.transpose()) .collect::, PixiPreferencesError>>() @@ -815,6 +812,7 @@ pub async fn resolve_pypi( &context.capabilities, context.concurrency.downloads, project_root, + &original_git_references, ) .await?; @@ -937,6 +935,7 @@ fn get_url_or_path( } /// Create a vector of locked packages from a resolution +#[allow(clippy::too_many_arguments)] async fn lock_pypi_packages( conda_python_packages: CondaPythonPackages, pixi_build_dispatch: &LazyBuildDispatch<'_>, @@ -945,6 +944,7 @@ async fn lock_pypi_packages( index_capabilities: &IndexCapabilities, concurrent_downloads: usize, abs_project_root: &Path, + original_git_references: &HashMap, ) -> miette::Result> { let mut locked_packages = LockedPypiPackages::with_capacity(resolution.len()); let database = @@ -1064,8 +1064,15 @@ async fn lock_pypi_packages( (direct_url.into(), hash, false) } SourceDist::Git(git) => { + // Look up the original git reference from the manifest dependencies + // to preserve branch/tag info that uv normalizes away + let package_name = git.name.clone(); + let original_reference = + original_git_references.get(&package_name).cloned(); + // convert resolved source dist into a pinned git spec - let pinned_git_spec = into_pinned_git_spec(git.clone()); + let pinned_git_spec = + into_pinned_git_spec(git.clone(), original_reference); ( pinned_git_spec.into_locked_git_url().to_url().into(), hash, diff --git a/crates/pixi_core/src/lock_file/satisfiability/mod.rs b/crates/pixi_core/src/lock_file/satisfiability/mod.rs index b9afeb6774..1a6e507f3a 100644 --- a/crates/pixi_core/src/lock_file/satisfiability/mod.rs +++ b/crates/pixi_core/src/lock_file/satisfiability/mod.rs @@ -970,10 +970,28 @@ pub(crate) fn pypi_satifisfies_requirement( } .into()); } - // If the spec does not specify a revision than any will do - // E.g `git.com/user/repo` is the same as `git.com/user/repo@adbdd` + // If the spec uses DefaultBranch, we need to check what the lock has + // DefaultBranch in it + // otherwise any explicit ref in lock is not satisfiable if *reference == GitReference::DefaultBranch { - return Ok(()); + match &pinned_git_spec.source.reference { + // Any explicit reference in lock is not satisfiable + // when manifest has DefaultBranch (user removed the explicit ref) + pixi_spec::GitReference::Branch(_) + | pixi_spec::GitReference::Tag(_) + | pixi_spec::GitReference::Rev(_) => { + return Err(PlatformUnsat::LockedPyPIGitRefMismatch { + name: spec.name.clone().to_string(), + expected_ref: reference.to_string(), + found_ref: pinned_git_spec.source.reference.to_string(), + } + .into()); + } + // Only DefaultBranch in lock is satisfiable + pixi_spec::GitReference::DefaultBranch => { + return Ok(()); + } + } } if pinned_git_spec.source.subdirectory @@ -2599,12 +2617,35 @@ mod tests { .unwrap(); // This should not pypi_satifisfies_requirement(&non_matching_spec, &locked_data, &project_root).unwrap_err(); - // Removing the rev from the Requirement should satisfy any revision - let spec = pep508_requirement_to_uv_requirement( + + // Removing the rev from the Requirement should NOT satisfy when lock has explicit Rev. + // This ensures that when a user removes an explicit ref from the manifest, + // the lock file gets re-resolved. + let spec_without_rev = pep508_requirement_to_uv_requirement( pep508_rs::Requirement::from_str("mypkg @ git+https://github.com/mypkg").unwrap(), ) .unwrap(); - pypi_satifisfies_requirement(&spec, &locked_data, &project_root).unwrap(); + pypi_satifisfies_requirement(&spec_without_rev, &locked_data, &project_root).unwrap_err(); + + // When lock has DefaultBranch (no explicit ref), removing rev from manifest should satisfy + let locked_data_default_branch = PypiPackageData { + name: "mypkg".parse().unwrap(), + version: Version::from_str("0.1.0").unwrap(), + // No ?rev= query param, only the fragment with commit hash + location: "git+https://github.com/mypkg.git#29932f3915935d773dc8d52c292cadd81c81071d" + .parse() + .expect("failed to parse url"), + hash: None, + requires_dist: vec![], + requires_python: None, + editable: false, + }; + pypi_satifisfies_requirement( + &spec_without_rev, + &locked_data_default_branch, + &project_root, + ) + .unwrap(); } // Currently this test is missing from `good_satisfiability`, so we test the diff --git a/crates/pixi_uv_conversions/src/conversions.rs b/crates/pixi_uv_conversions/src/conversions.rs index 17ef4432f1..2c2a07f5af 100644 --- a/crates/pixi_uv_conversions/src/conversions.rs +++ b/crates/pixi_uv_conversions/src/conversions.rs @@ -292,9 +292,21 @@ pub fn into_pixi_reference(git_reference: uv_git_types::GitReference) -> PixiRef } /// Convert a solved [`GitSourceDist`] into [`PinnedGitSpec`] -pub fn into_pinned_git_spec(dist: GitSourceDist) -> PinnedGitSpec { - let reference = into_pixi_reference(dist.git.reference().clone()); - +/// +/// The `original_reference` parameter allows preserving the original git reference +/// from the manifest (e.g., `Branch("main")`). When uv resolves a git dependency, +/// it may normalize branch references to `DefaultBranch`, losing the original +/// branch information. If provided, this original reference will be used instead +/// of the one from uv's resolution. +/// +/// If no original reference is provided (user didn't specify branch/tag/rev), +/// we store the resolved commit as `Rev(commit)` rather than `DefaultBranch`. +/// This ensures the lock file has a precise reference that doesn't require +/// cache lookups when re-resolving (similar to how uv's lockfile works). +pub fn into_pinned_git_spec( + dist: GitSourceDist, + original_reference: Option, +) -> PinnedGitSpec { // Necessary to convert between our gitsha and uv gitsha. let git_sha = PixiGitSha::from_str( &dist @@ -305,6 +317,12 @@ pub fn into_pinned_git_spec(dist: GitSourceDist) -> PinnedGitSpec { ) .expect("we expect it to be a valid sha"); + // Use the original reference from the manifest if provided. + // If no explicit reference was specified, use DefaultBranch. + // The precise commit is already captured in the fragment (`#commit`), + // so we don't need to duplicate it in the query string as `?rev=commit`. + let reference = original_reference.unwrap_or(PixiReference::DefaultBranch); + let pinned_checkout = PinnedGitCheckout::new( git_sha, dist.subdirectory.map(|sd| sd.to_string_lossy().to_string()),