Skip to content

fix: always add git requirement to the lockfile #5187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ruben-arts wants to merge 3 commits into from
Closed

fix: always add git requirement to the lockfile #5187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea57bd5
fix: always add git requirement to the lockfile
ruben-arts Dec 22, 2025
4abbc6e
fix: simplify GitUrl construction in lockfile
ruben-arts Dec 23, 2025
2b19198
Merge branch 'main' into fix/pypi-git-rev-in-lock
nichmor Dec 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 36 additions & 7 deletions crates/pixi_core/src/lock_file/resolve/pypi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,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_pixi_reference,
into_uv_git_reference, 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,
Expand All @@ -48,8 +49,8 @@ use uv_configuration::{Constraints, Overrides};
use uv_distribution::DistributionDatabase;
use uv_distribution_types::{
BuiltDist, ConfigSettings, DependencyMetadata, Diagnostic, Dist, FileLocation, HashPolicy,
IndexCapabilities, IndexUrl, Name, RequirementSource, RequiresPython, Resolution, ResolvedDist,
SourceDist, ToUrlError,
IndexCapabilities, IndexUrl, Name, Requirement, RequirementSource, RequiresPython, Resolution,
ResolvedDist, SourceDist, ToUrlError,
};
use uv_git_types::GitUrl;
use uv_pep508::VerbatimUrl;
Expand Down Expand Up @@ -355,6 +356,10 @@ pub async fn resolve_pypi(
.collect::<Result<Vec<_>, _>>()
.into_diagnostic()?;

// Clone requirements for later use in lock file generation
// We need this because requirements will be moved into the resolver
let requirements_for_locking = requirements.clone();

// Determine the python interpreter that is installed as part of the conda
// packages.
let python_record = locked_pixi_records
Expand Down Expand Up @@ -629,7 +634,13 @@ pub async fn resolve_pypi(
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);
// Construct the GitUrl with the reference (branch/tag) from the pinned_git_spec
// to preserve the branch information in the lock file
let git_url = GitUrl::from_fields(
display_safe,
into_uv_git_reference(pinned_git_spec.source.reference.into()),
Some(git_oid),
Copy link
Contributor Author

@ruben-arts ruben-arts Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sure about this, it seems that the .with_precise didn't do what it should.

Copy link
Contributor

@nichmor nichmor Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem was with this part into_uv_git_reference ( git reference wasn't saved ) when parsing from display_safe

)?;

let constraint_source = RequirementSource::Git {
git: git_url,
Expand Down Expand Up @@ -821,6 +832,7 @@ pub async fn resolve_pypi(
&context.capabilities,
context.concurrency.downloads,
project_root,
&requirements_for_locking,
)
.await?;

Expand Down Expand Up @@ -943,6 +955,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<'_>,
Expand All @@ -951,6 +964,7 @@ async fn lock_pypi_packages(
index_capabilities: &IndexCapabilities,
concurrent_downloads: usize,
abs_project_root: &Path,
original_requirements: &[Requirement],
) -> miette::Result<Vec<(PypiPackageData, PypiPackageEnvironmentData)>> {
let mut locked_packages = LockedPypiPackages::with_capacity(resolution.len());
let database =
Expand Down Expand Up @@ -1071,7 +1085,22 @@ async fn lock_pypi_packages(
}
SourceDist::Git(git) => {
// convert resolved source dist into a pinned git spec
let pinned_git_spec = into_pinned_git_spec(git.clone());
let mut pinned_git_spec = into_pinned_git_spec(git.clone());

// Look up the original requirement to get the git reference (branch/tag)
// It may have resolved the reference to just a commit, losing the branch/tag info
if let Some(original_req) = original_requirements
.iter()
.find(|r| &r.name == dist.name())
&& let RequirementSource::Git {
git: original_git, ..
} = &original_req.source
{
// Use the reference from the original requirement instead of what UV resolved
pinned_git_spec.source.reference =
Copy link
Contributor

@nichmor nichmor Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you always want to override the reference? It may be that the reference is already set, maybe we can skip in this case, searching through the original requirements?

Copy link
Contributor Author

@ruben-arts ruben-arts Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how to fix this. The reference is default branch but there must be multiple code paths as it sometimes is written without and sometimes with the reference in the url.

into_pixi_reference(original_git.reference().clone());
Copy link
Contributor Author

@ruben-arts ruben-arts Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pushes in the actual requirement, is this right? It feels strange that that information is not in the resolved spec.

}

(
pinned_git_spec.into_locked_git_url().to_url().into(),
hash,
Expand Down
31 changes: 21 additions & 10 deletions crates/pixi_core/src/lock_file/satisfiability/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ use typed_path::Utf8TypedPathBuf;
use url::Url;
use uv_distribution_filename::{DistExtension, ExtensionError, SourceDistExtension};
use uv_distribution_types::{RequirementSource, RequiresPython};
use uv_git_types::GitReference;
use uv_pypi_types::ParsedUrlError;

use super::{
Expand Down Expand Up @@ -970,11 +969,6 @@ 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 *reference == GitReference::DefaultBranch {
return Ok(());
}

if pinned_git_spec.source.subdirectory
!= subdirectory
Expand All @@ -995,8 +989,9 @@ pub(crate) fn pypi_satifisfies_requirement(
}
.into());
}
// If the spec does specify a revision than the revision must match
// convert first to the same type
// The git reference (branch/tag/rev) must match exactly between
// the requirement and the lock file. This ensures that if a user
// adds or removes a branch specification, the lock file will update.
let pixi_reference = into_pixi_reference(reference.clone());

if pinned_git_spec.source.reference == pixi_reference {
Expand Down Expand Up @@ -2608,12 +2603,28 @@ 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
// Removing the rev from the Requirement now requires the lock file to also
// not have an explicit rev (must match exactly). This is a fix for issue #5185.
let spec = 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();
// This should now fail because the lock file has an explicit rev but the spec doesn't
pypi_satifisfies_requirement(&spec, &locked_data, &project_root).unwrap_err();

// But if the lock file also doesn't have an explicit rev, it should match
let locked_data_no_rev = PypiPackageData {
name: "mypkg".parse().unwrap(),
version: Version::from_str("0.1.0").unwrap(),
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, &locked_data_no_rev, &project_root).unwrap();
}

// Currently this test is missing from `good_satisfiability`, so we test the
Expand Down
Loading