From a1fa5b850e57c2e4154d747357038a5ef95367f0 Mon Sep 17 00:00:00 2001 From: konstin Date: Mon, 17 Mar 2025 11:07:18 +0100 Subject: [PATCH 1/2] Reject lockfiles with incoherent versions Reject lockfiles where the package version and the wheel versions are incoherent. This implicitly checks that all wheel files have the same version. It does not check for the source dist version, since a source dist may not contain a version in the filename and attempting to deserialize source dist filenames we may not need is a performance overhead for something that's already slow in `uv run`. Fixes #12164 --- crates/uv-resolver/src/lock/mod.rs | 20 +++++++ crates/uv/tests/it/sync.rs | 94 ++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index d66920fef8ab7..eea08f668220a 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -2871,6 +2871,19 @@ impl PackageWire { requires_python: &RequiresPython, unambiguous_package_ids: &FxHashMap, ) -> Result { + // Consistency check + if let Some(version) = &self.id.version { + for wheel in &self.wheels { + if version != &wheel.filename.version { + return Err(LockError::from(LockErrorKind::InconsistentVersions { + name: self.id.name, + })); + } + } + // We can't check the source dist version since it does not need to contain the version + // in the filename. + } + let unwire_deps = |deps: Vec| -> Result, LockError> { deps.into_iter() .map(|dep| dep.unwire(requires_python, unambiguous_package_ids)) @@ -5156,6 +5169,13 @@ enum LockErrorKind { #[source] err: uv_distribution::Error, }, + /// A package has inconsistent versions in a single entry + // Using name instead of id since the version in the id is part of the conflict. + #[error("Locked package and file versions are inconsistent for `{name}`", name = name.cyan())] + InconsistentVersions { + /// The name of the package with the inconsistent entry. + name: PackageName, + }, #[error( "Found conflicting extras `{package1}[{extra1}]` \ and `{package2}[{extra2}]` enabled simultaneously" diff --git a/crates/uv/tests/it/sync.rs b/crates/uv/tests/it/sync.rs index d94e9d2b5981e..8934b5b4e846a 100644 --- a/crates/uv/tests/it/sync.rs +++ b/crates/uv/tests/it/sync.rs @@ -8473,3 +8473,97 @@ fn prune_cache_url_subdirectory() -> Result<()> { Ok(()) } + +/// Test that incoherence in the versions in a package entry of the lockfile versions is caught. +/// +/// See +#[test] +fn locked_version_coherence() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["iniconfig"] + "#, + )?; + + uv_snapshot!(context.filters(), context.lock(), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 2 packages in [TIME] + "); + + let lock = context.read("uv.lock"); + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + lock, @r#" + version = 1 + revision = 1 + requires-python = ">=3.12" + + [options] + exclude-newer = "2024-03-25T00:00:00Z" + + [[package]] + name = "iniconfig" + version = "2.0.0" + source = { registry = "https://pypi.org/simple" } + sdist = { url = "https://files.pythonhosted.org/packages/d7/4b/cbd8e699e64a6f16ca3a8220661b5f83792b3017d0f79807cb8708d33913/iniconfig-2.0.0.tar.gz", hash = "sha256:2d91e135bf72d31a410b17c16da610a82cb55f6b0477d1a902134b24a455b8b3", size = 4646 } + wheels = [ + { url = "https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl", hash = "sha256:b6a85871a79d2e3b22d2d1b94ac2824226a63c6b741c88f7ae975f18b6778374", size = 5892 }, + ] + + [[package]] + name = "project" + version = "0.1.0" + source = { virtual = "." } + dependencies = [ + { name = "iniconfig" }, + ] + + [package.metadata] + requires-dist = [{ name = "iniconfig" }] + "#); + }); + + // Write an inconsistent iniconfig entry + context + .temp_dir + .child("uv.lock") + .write_str(&lock.replace(r#"version = "2.0.0""#, r#"version = "1.0.0""#))?; + + // An inconsistent lockfile should fail with `--locked` + uv_snapshot!(context.filters(), context.sync().arg("--locked"), @r" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to parse `uv.lock` + Caused by: Locked package and file versions are inconsistent for `iniconfig` + "); + + // Without `--locked`, we could fail or recreate the lockfile, currently, we fail. + uv_snapshot!(context.filters(), context.lock(), @r" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to parse `uv.lock` + Caused by: Locked package and file versions are inconsistent for `iniconfig` + "); + + Ok(()) +} From 36e41818957500bb7ddfa4ca4406c86a7d79c8d6 Mon Sep 17 00:00:00 2001 From: konstin Date: Mon, 17 Mar 2025 12:29:52 +0100 Subject: [PATCH 2/2] Catch versions incoherences from source dists only --- crates/uv-distribution-types/src/buildable.rs | 16 +++++++++++-- crates/uv-distribution-types/src/lib.rs | 9 +++++++ crates/uv-distribution/src/source/mod.rs | 2 ++ crates/uv-installer/src/plan.rs | 24 +++++++++++++++---- crates/uv-requirements/src/lib.rs | 1 + crates/uv-requirements/src/source_tree.rs | 1 + crates/uv-requirements/src/unnamed.rs | 4 ++++ crates/uv-resolver/src/lock/mod.rs | 5 ++++ crates/uv/src/commands/project/sync.rs | 10 ++++++-- 9 files changed, 64 insertions(+), 8 deletions(-) diff --git a/crates/uv-distribution-types/src/buildable.rs b/crates/uv-distribution-types/src/buildable.rs index e633c81ada918..30178e395103e 100644 --- a/crates/uv-distribution-types/src/buildable.rs +++ b/crates/uv-distribution-types/src/buildable.rs @@ -37,8 +37,13 @@ impl BuildableSource<'_> { match self { Self::Dist(SourceDist::Registry(dist)) => Some(&dist.version), Self::Dist(SourceDist::Path(dist)) => dist.version.as_ref(), - Self::Dist(_) => None, - Self::Url(_) => None, + Self::Dist(SourceDist::Directory(dist)) => dist.version.as_ref(), + Self::Dist(SourceDist::Git(dist)) => dist.version.as_ref(), + Self::Dist(SourceDist::DirectUrl(dist)) => dist.version.as_ref(), + Self::Url(SourceUrl::Git(url)) => url.version, + Self::Url(SourceUrl::Direct(url)) => url.version, + Self::Url(SourceUrl::Path(url)) => url.version, + Self::Url(SourceUrl::Directory(url)) => url.version, } } @@ -132,6 +137,7 @@ impl std::fmt::Display for SourceUrl<'_> { #[derive(Debug, Clone)] pub struct DirectSourceUrl<'a> { pub url: &'a Url, + pub version: Option<&'a Version>, pub subdirectory: Option<&'a Path>, pub ext: SourceDistExtension, } @@ -146,6 +152,7 @@ impl std::fmt::Display for DirectSourceUrl<'_> { pub struct GitSourceUrl<'a> { /// The URL with the revision and subdirectory fragment. pub url: &'a VerbatimUrl, + pub version: Option<&'a Version>, pub git: &'a GitUrl, /// The URL without the revision and subdirectory fragment. pub subdirectory: Option<&'a Path>, @@ -161,6 +168,7 @@ impl<'a> From<&'a GitSourceDist> for GitSourceUrl<'a> { fn from(dist: &'a GitSourceDist) -> Self { Self { url: &dist.url, + version: dist.version.as_ref(), git: &dist.git, subdirectory: dist.subdirectory.as_deref(), } @@ -170,6 +178,7 @@ impl<'a> From<&'a GitSourceDist> for GitSourceUrl<'a> { #[derive(Debug, Clone)] pub struct PathSourceUrl<'a> { pub url: &'a Url, + pub version: Option<&'a Version>, pub path: Cow<'a, Path>, pub ext: SourceDistExtension, } @@ -184,6 +193,7 @@ impl<'a> From<&'a PathSourceDist> for PathSourceUrl<'a> { fn from(dist: &'a PathSourceDist) -> Self { Self { url: &dist.url, + version: dist.version.as_ref(), path: Cow::Borrowed(&dist.install_path), ext: dist.ext, } @@ -193,6 +203,7 @@ impl<'a> From<&'a PathSourceDist> for PathSourceUrl<'a> { #[derive(Debug, Clone)] pub struct DirectorySourceUrl<'a> { pub url: &'a Url, + pub version: Option<&'a Version>, pub install_path: Cow<'a, Path>, pub editable: bool, } @@ -207,6 +218,7 @@ impl<'a> From<&'a DirectorySourceDist> for DirectorySourceUrl<'a> { fn from(dist: &'a DirectorySourceDist) -> Self { Self { url: &dist.url, + version: dist.version.as_ref(), install_path: Cow::Borrowed(&dist.install_path), editable: dist.editable, } diff --git a/crates/uv-distribution-types/src/lib.rs b/crates/uv-distribution-types/src/lib.rs index ba8f2b6d6550a..653ca2d9b7bab 100644 --- a/crates/uv-distribution-types/src/lib.rs +++ b/crates/uv-distribution-types/src/lib.rs @@ -294,6 +294,8 @@ pub struct DirectUrlSourceDist { /// Unlike [`DirectUrlBuiltDist`], we can't require a full filename with a version here, people /// like using e.g. `foo @ https://github.com/org/repo/archive/master.zip` pub name: PackageName, + /// The expected version, if known. + pub version: Option, /// The URL without the subdirectory fragment. pub location: Box, /// The subdirectory within the archive in which the source distribution is located. @@ -308,6 +310,8 @@ pub struct DirectUrlSourceDist { #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct GitSourceDist { pub name: PackageName, + /// The expected version, if known. + pub version: Option, /// The URL without the revision and subdirectory fragment. pub git: Box, /// The subdirectory within the Git repository in which the source distribution is located. @@ -333,6 +337,8 @@ pub struct PathSourceDist { #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct DirectorySourceDist { pub name: PackageName, + /// The expected version, if known. + pub version: Option, /// The absolute path to the distribution which we use for installing. pub install_path: PathBuf, /// Whether the package should be installed in editable mode. @@ -374,6 +380,7 @@ impl Dist { DistExtension::Source(ext) => { Ok(Self::Source(SourceDist::DirectUrl(DirectUrlSourceDist { name, + version: None, location: Box::new(location), subdirectory, ext, @@ -462,6 +469,7 @@ impl Dist { // Determine whether the path represents an archive or a directory. Ok(Self::Source(SourceDist::Directory(DirectorySourceDist { name, + version: None, install_path, editable, r#virtual, @@ -478,6 +486,7 @@ impl Dist { ) -> Result { Ok(Self::Source(SourceDist::Git(GitSourceDist { name, + version: None, git: Box::new(git), subdirectory, url, diff --git a/crates/uv-distribution/src/source/mod.rs b/crates/uv-distribution/src/source/mod.rs index d7af5d3314cfa..a42b22d4908c3 100644 --- a/crates/uv-distribution/src/source/mod.rs +++ b/crates/uv-distribution/src/source/mod.rs @@ -138,6 +138,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { source, &PathSourceUrl { url: &url, + version: Some(&dist.version), path: Cow::Owned(path), ext: dist.ext, }, @@ -287,6 +288,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { source, &PathSourceUrl { url: &url, + version: Some(&dist.version), path: Cow::Owned(path), ext: dist.ext, }, diff --git a/crates/uv-installer/src/plan.rs b/crates/uv-installer/src/plan.rs index 157e2c21cfbd2..07f3f1cc926bd 100644 --- a/crates/uv-installer/src/plan.rs +++ b/crates/uv-installer/src/plan.rs @@ -264,7 +264,11 @@ impl<'a> Planner<'a> { // Find the most-compatible wheel from the cache, since we don't know // the filename in advance. if let Some(wheel) = built_index.url(sdist)? { - if wheel.filename.name == sdist.name { + if wheel.filename.name == sdist.name + && dist + .version() + .is_some_and(|version| version == &wheel.filename.version) + { let cached_dist = wheel.into_url_dist(sdist); debug!("URL source requirement already cached: {cached_dist}"); cached.push(CachedDist::Url(cached_dist)); @@ -282,7 +286,11 @@ impl<'a> Planner<'a> { // Find the most-compatible wheel from the cache, since we don't know // the filename in advance. if let Some(wheel) = built_index.git(sdist) { - if wheel.filename.name == sdist.name { + if wheel.filename.name == sdist.name + && dist + .version() + .is_some_and(|version| version == &wheel.filename.version) + { let cached_dist = wheel.into_git_dist(sdist); debug!("Git source requirement already cached: {cached_dist}"); cached.push(CachedDist::Url(cached_dist)); @@ -305,7 +313,11 @@ impl<'a> Planner<'a> { // Find the most-compatible wheel from the cache, since we don't know // the filename in advance. if let Some(wheel) = built_index.path(sdist)? { - if wheel.filename.name == sdist.name { + if wheel.filename.name == sdist.name + && dist + .version() + .is_some_and(|version| version == &wheel.filename.version) + { let cached_dist = wheel.into_path_dist(sdist); debug!("Path source requirement already cached: {cached_dist}"); cached.push(CachedDist::Url(cached_dist)); @@ -328,7 +340,11 @@ impl<'a> Planner<'a> { // Find the most-compatible wheel from the cache, since we don't know // the filename in advance. if let Some(wheel) = built_index.directory(sdist)? { - if wheel.filename.name == sdist.name { + if wheel.filename.name == sdist.name + && dist + .version() + .is_some_and(|version| version == &wheel.filename.version) + { let cached_dist = wheel.into_directory_dist(sdist); debug!("Directory source requirement already cached: {cached_dist}"); cached.push(CachedDist::Url(cached_dist)); diff --git a/crates/uv-requirements/src/lib.rs b/crates/uv-requirements/src/lib.rs index bbe76a1b9fe0c..657deef406f7a 100644 --- a/crates/uv-requirements/src/lib.rs +++ b/crates/uv-requirements/src/lib.rs @@ -62,6 +62,7 @@ pub(crate) fn required_dist( url, } => Dist::Source(SourceDist::Git(GitSourceDist { name: requirement.name.clone(), + version: None, git: Box::new(git.clone()), subdirectory: subdirectory.clone(), url: url.clone(), diff --git a/crates/uv-requirements/src/source_tree.rs b/crates/uv-requirements/src/source_tree.rs index 2b077861a282c..d2ca7102ca56e 100644 --- a/crates/uv-requirements/src/source_tree.rs +++ b/crates/uv-requirements/src/source_tree.rs @@ -176,6 +176,7 @@ impl<'a, Context: BuildContext> SourceTreeResolver<'a, Context> { }; let source = SourceUrl::Directory(DirectorySourceUrl { url: &url, + version: None, install_path: Cow::Borrowed(source_tree), editable: false, }); diff --git a/crates/uv-requirements/src/unnamed.rs b/crates/uv-requirements/src/unnamed.rs index 9356e42dea7cb..c7f7f45409b3d 100644 --- a/crates/uv-requirements/src/unnamed.rs +++ b/crates/uv-requirements/src/unnamed.rs @@ -238,6 +238,7 @@ impl<'a, Context: BuildContext> NamedRequirementsResolver<'a, Context> { SourceUrl::Directory(DirectorySourceUrl { url: &requirement.url.verbatim, + version: None, install_path: Cow::Borrowed(&parsed_directory_url.install_path), editable: parsed_directory_url.editable, }) @@ -249,6 +250,7 @@ impl<'a, Context: BuildContext> NamedRequirementsResolver<'a, Context> { }; SourceUrl::Path(PathSourceUrl { url: &requirement.url.verbatim, + version: None, path: Cow::Borrowed(&parsed_path_url.install_path), ext, }) @@ -260,12 +262,14 @@ impl<'a, Context: BuildContext> NamedRequirementsResolver<'a, Context> { }; SourceUrl::Direct(DirectSourceUrl { url: &parsed_archive_url.url, + version: None, subdirectory: parsed_archive_url.subdirectory.as_deref(), ext, }) } ParsedUrl::Git(parsed_git_url) => SourceUrl::Git(GitSourceUrl { url: &requirement.url.verbatim, + version: None, git: &parsed_git_url.url, subdirectory: parsed_git_url.subdirectory.as_deref(), }), diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index eea08f668220a..ba27afb87a857 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -2336,6 +2336,7 @@ impl Package { let install_path = absolute_path(workspace_root, path)?; let dir_dist = DirectorySourceDist { name: self.id.name.clone(), + version: self.id.version.clone(), url: verbatim_url(&install_path, &self.id)?, install_path, editable: false, @@ -2347,6 +2348,7 @@ impl Package { let install_path = absolute_path(workspace_root, path)?; let dir_dist = DirectorySourceDist { name: self.id.name.clone(), + version: self.id.version.clone(), url: verbatim_url(&install_path, &self.id)?, install_path, editable: true, @@ -2358,6 +2360,7 @@ impl Package { let install_path = absolute_path(workspace_root, path)?; let dir_dist = DirectorySourceDist { name: self.id.name.clone(), + version: self.id.version.clone(), url: verbatim_url(&install_path, &self.id)?, install_path, editable: false, @@ -2387,6 +2390,7 @@ impl Package { let git_dist = GitSourceDist { name: self.id.name.clone(), + version: self.id.version.clone(), url: VerbatimUrl::from_url(url), git: Box::new(git_url), subdirectory: git.subdirectory.clone(), @@ -2407,6 +2411,7 @@ impl Package { }); let direct_dist = DirectUrlSourceDist { name: self.id.name.clone(), + version: self.id.version.clone(), location: Box::new(location), subdirectory: subdirectory.clone(), ext, diff --git a/crates/uv/src/commands/project/sync.rs b/crates/uv/src/commands/project/sync.rs index 6b742d213c81b..a031c2bf0c6d6 100644 --- a/crates/uv/src/commands/project/sync.rs +++ b/crates/uv/src/commands/project/sync.rs @@ -742,11 +742,16 @@ fn apply_editable_mode(resolution: Resolution, editable: EditableMode) -> Resolu // Filter out any editable distributions. EditableMode::NonEditable => resolution.map(|dist| { - let ResolvedDist::Installable { dist, version } = dist else { + let ResolvedDist::Installable { + dist, + version: installable_version, + } = dist + else { return None; }; let Dist::Source(SourceDist::Directory(DirectorySourceDist { name, + version, install_path, editable: true, r#virtual: false, @@ -759,12 +764,13 @@ fn apply_editable_mode(resolution: Resolution, editable: EditableMode) -> Resolu Some(ResolvedDist::Installable { dist: Arc::new(Dist::Source(SourceDist::Directory(DirectorySourceDist { name: name.clone(), + version: version.clone(), install_path: install_path.clone(), editable: false, r#virtual: false, url: url.clone(), }))), - version: version.clone(), + version: installable_version.clone(), }) }), }