Skip to content
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.

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

Error on lockfiles with inconsistent source distribution versions #12237

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 14 additions & 2 deletions crates/uv-distribution-types/src/buildable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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,
}
Expand All @@ -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>,
Expand All @@ -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(),
}
Expand All @@ -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,
}
Expand All @@ -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,
}
Expand All @@ -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,
}
Expand All @@ -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,
}
Expand Down
9 changes: 9 additions & 0 deletions crates/uv-distribution-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Version>,
/// The URL without the subdirectory fragment.
pub location: Box<Url>,
/// The subdirectory within the archive in which the source distribution is located.
Expand All @@ -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<Version>,
/// The URL without the revision and subdirectory fragment.
pub git: Box<GitUrl>,
/// The subdirectory within the Git repository in which the source distribution is located.
Expand All @@ -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<Version>,
/// The absolute path to the distribution which we use for installing.
pub install_path: PathBuf,
/// Whether the package should be installed in editable mode.
Expand Down Expand Up @@ -374,6 +380,7 @@ impl Dist {
DistExtension::Source(ext) => {
Ok(Self::Source(SourceDist::DirectUrl(DirectUrlSourceDist {
name,
version: None,
location: Box::new(location),
subdirectory,
ext,
Expand Down Expand Up @@ -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,
Expand All @@ -478,6 +486,7 @@ impl Dist {
) -> Result<Dist, Error> {
Ok(Self::Source(SourceDist::Git(GitSourceDist {
name,
version: None,
git: Box::new(git),
subdirectory,
url,
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-distribution/src/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down
24 changes: 20 additions & 4 deletions crates/uv-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

These will be false if we don't know the sdist version, right? Should we instead maintain the old behavior in that case?

Something like:

if wheel.filename.name == sdist.name
  && dist.version().map_or(true, |version| version == &wheel.filename.version)

&& 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));
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand Down
1 change: 1 addition & 0 deletions crates/uv-requirements/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
1 change: 1 addition & 0 deletions crates/uv-requirements/src/source_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down
4 changes: 4 additions & 0 deletions crates/uv-requirements/src/unnamed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand All @@ -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,
})
Expand All @@ -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(),
}),
Expand Down
25 changes: 25 additions & 0 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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(),
Expand All @@ -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,
Expand Down Expand Up @@ -2871,6 +2876,19 @@ impl PackageWire {
requires_python: &RequiresPython,
unambiguous_package_ids: &FxHashMap<PackageName, PackageId>,
) -> Result<Package, LockError> {
// 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<DependencyWire>| -> Result<Vec<Dependency>, LockError> {
deps.into_iter()
.map(|dep| dep.unwire(requires_python, unambiguous_package_ids))
Expand Down Expand Up @@ -5156,6 +5174,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"
Expand Down
10 changes: 8 additions & 2 deletions crates/uv/src/commands/project/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(),
})
}),
}
Expand Down
Loading
Loading