Skip to content

Commit

Permalink
Add version to ResolvedDist (#9102)
Browse files Browse the repository at this point in the history
## Summary

I need this for the derivation chain work
(#8962), but it just seems
generally useful. You can't always get a version from a `Dist` (it could
be URL-based!), but when we create a `ResolvedDist`, we _do_ know the
version (and not just the URL). This PR preserves it.
  • Loading branch information
charliermarsh authored Nov 14, 2024
1 parent 17181d9 commit 9339e55
Show file tree
Hide file tree
Showing 10 changed files with 107 additions and 80 deletions.
4 changes: 2 additions & 2 deletions crates/uv-distribution-types/src/prioritized_distribution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ impl<'a> CompatibleDist<'a> {
/// Return the [`ResolvedDistRef`] to use during resolution.
pub fn for_resolution(&self) -> ResolvedDistRef<'a> {
match *self {
CompatibleDist::InstalledDist(dist) => ResolvedDistRef::Installed(dist),
CompatibleDist::InstalledDist(dist) => ResolvedDistRef::Installed { dist },
CompatibleDist::SourceDist { sdist, prioritized } => {
ResolvedDistRef::InstallableRegistrySourceDist { sdist, prioritized }
}
Expand All @@ -458,7 +458,7 @@ impl<'a> CompatibleDist<'a> {
/// Return the [`ResolvedDistRef`] to use during installation.
pub fn for_installation(&self) -> ResolvedDistRef<'a> {
match *self {
CompatibleDist::InstalledDist(dist) => ResolvedDistRef::Installed(dist),
CompatibleDist::InstalledDist(dist) => ResolvedDistRef::Installed { dist },
CompatibleDist::SourceDist { sdist, prioritized } => {
ResolvedDistRef::InstallableRegistrySourceDist { sdist, prioritized }
}
Expand Down
4 changes: 2 additions & 2 deletions crates/uv-distribution-types/src/resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ impl Diagnostic for ResolutionDiagnostic {
impl From<&ResolvedDist> for Requirement {
fn from(resolved_dist: &ResolvedDist) -> Self {
let source = match resolved_dist {
ResolvedDist::Installable(dist) => match dist {
ResolvedDist::Installable { dist, .. } => match dist {
Dist::Built(BuiltDist::Registry(wheels)) => RequirementSource::Registry {
specifier: uv_pep440::VersionSpecifiers::from(
uv_pep440::VersionSpecifier::equals_version(
Expand Down Expand Up @@ -229,7 +229,7 @@ impl From<&ResolvedDist> for Requirement {
r#virtual: sdist.r#virtual,
},
},
ResolvedDist::Installed(dist) => RequirementSource::Registry {
ResolvedDist::Installed { dist } => RequirementSource::Registry {
specifier: uv_pep440::VersionSpecifiers::from(
uv_pep440::VersionSpecifier::equals_version(dist.version().clone()),
),
Expand Down
84 changes: 48 additions & 36 deletions crates/uv-distribution-types/src/resolved.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::fmt::{Display, Formatter};

use uv_pep440::Version;
use uv_pep508::PackageName;
use uv_pypi_types::Yanked;

Expand All @@ -15,14 +15,16 @@ use crate::{
#[derive(Debug, Clone, Hash)]
#[allow(clippy::large_enum_variant)]
pub enum ResolvedDist {
Installed(InstalledDist),
Installable(Dist),
Installed { dist: InstalledDist },
Installable { dist: Dist, version: Version },
}

/// A variant of [`ResolvedDist`] with borrowed inner distributions.
#[derive(Debug, Clone)]
pub enum ResolvedDistRef<'a> {
Installed(&'a InstalledDist),
Installed {
dist: &'a InstalledDist,
},
InstallableRegistrySourceDist {
/// The source distribution that should be used.
sdist: &'a RegistrySourceDist,
Expand All @@ -41,36 +43,44 @@ impl ResolvedDist {
/// Return true if the distribution is editable.
pub fn is_editable(&self) -> bool {
match self {
Self::Installable(dist) => dist.is_editable(),
Self::Installed(dist) => dist.is_editable(),
Self::Installable { dist, .. } => dist.is_editable(),
Self::Installed { dist } => dist.is_editable(),
}
}

/// Return true if the distribution refers to a local file or directory.
pub fn is_local(&self) -> bool {
match self {
Self::Installable(dist) => dist.is_local(),
Self::Installed(dist) => dist.is_local(),
Self::Installable { dist, .. } => dist.is_local(),
Self::Installed { dist } => dist.is_local(),
}
}

/// Returns the [`IndexUrl`], if the distribution is from a registry.
pub fn index(&self) -> Option<&IndexUrl> {
match self {
Self::Installable(dist) => dist.index(),
Self::Installed(_) => None,
Self::Installable { dist, .. } => dist.index(),
Self::Installed { .. } => None,
}
}

/// Returns the [`Yanked`] status of the distribution, if available.
pub fn yanked(&self) -> Option<&Yanked> {
match self {
Self::Installable(dist) => match dist {
Self::Installable { dist, .. } => match dist {
Dist::Source(SourceDist::Registry(sdist)) => sdist.file.yanked.as_ref(),
Dist::Built(BuiltDist::Registry(wheel)) => wheel.best_wheel().file.yanked.as_ref(),
_ => None,
},
Self::Installed(_) => None,
Self::Installed { .. } => None,
}
}

/// Returns the version of the distribution.
pub fn version(&self) -> &Version {
match self {
Self::Installable { version, .. } => version,
Self::Installed { dist } => dist.version(),
}
}
}
Expand All @@ -87,7 +97,10 @@ impl ResolvedDistRef<'_> {
(&source.name, &source.version),
"expected chosen sdist to match prioritized sdist"
);
ResolvedDist::Installable(Dist::Source(SourceDist::Registry(source)))
ResolvedDist::Installable {
dist: Dist::Source(SourceDist::Registry(source)),
version: sdist.version.clone(),
}
}
Self::InstallableRegistryBuiltDist {
wheel, prioritized, ..
Expand All @@ -100,9 +113,14 @@ impl ResolvedDistRef<'_> {
// This is okay because we're only here if the prioritized dist
// has at least one wheel, so this always succeeds.
let built = prioritized.built_dist().expect("at least one wheel");
ResolvedDist::Installable(Dist::Built(BuiltDist::Registry(built)))
ResolvedDist::Installable {
dist: Dist::Built(BuiltDist::Registry(built)),
version: wheel.filename.version.clone(),
}
}
Self::Installed(dist) => ResolvedDist::Installed((*dist).clone()),
Self::Installed { dist } => ResolvedDist::Installed {
dist: (*dist).clone(),
},
}
}
}
Expand All @@ -112,7 +130,7 @@ impl Display for ResolvedDistRef<'_> {
match self {
Self::InstallableRegistrySourceDist { sdist, .. } => Display::fmt(sdist, f),
Self::InstallableRegistryBuiltDist { wheel, .. } => Display::fmt(wheel, f),
Self::Installed(dist) => Display::fmt(dist, f),
Self::Installed { dist } => Display::fmt(dist, f),
}
}
}
Expand All @@ -122,15 +140,15 @@ impl Name for ResolvedDistRef<'_> {
match self {
Self::InstallableRegistrySourceDist { sdist, .. } => sdist.name(),
Self::InstallableRegistryBuiltDist { wheel, .. } => wheel.name(),
Self::Installed(dist) => dist.name(),
Self::Installed { dist } => dist.name(),
}
}
}

impl DistributionMetadata for ResolvedDistRef<'_> {
fn version_or_url(&self) -> VersionOrUrlRef {
match self {
Self::Installed(installed) => VersionOrUrlRef::Version(installed.version()),
Self::Installed { dist } => VersionOrUrlRef::Version(dist.version()),
Self::InstallableRegistrySourceDist { sdist, .. } => sdist.version_or_url(),
Self::InstallableRegistryBuiltDist { wheel, .. } => wheel.version_or_url(),
}
Expand All @@ -140,15 +158,15 @@ impl DistributionMetadata for ResolvedDistRef<'_> {
impl Identifier for ResolvedDistRef<'_> {
fn distribution_id(&self) -> DistributionId {
match self {
Self::Installed(dist) => dist.distribution_id(),
Self::Installed { dist } => dist.distribution_id(),
Self::InstallableRegistrySourceDist { sdist, .. } => sdist.distribution_id(),
Self::InstallableRegistryBuiltDist { wheel, .. } => wheel.distribution_id(),
}
}

fn resource_id(&self) -> ResourceId {
match self {
Self::Installed(dist) => dist.resource_id(),
Self::Installed { dist } => dist.resource_id(),
Self::InstallableRegistrySourceDist { sdist, .. } => sdist.resource_id(),
Self::InstallableRegistryBuiltDist { wheel, .. } => wheel.resource_id(),
}
Expand All @@ -158,48 +176,42 @@ impl Identifier for ResolvedDistRef<'_> {
impl Name for ResolvedDist {
fn name(&self) -> &PackageName {
match self {
Self::Installable(dist) => dist.name(),
Self::Installed(dist) => dist.name(),
Self::Installable { dist, .. } => dist.name(),
Self::Installed { dist } => dist.name(),
}
}
}

impl DistributionMetadata for ResolvedDist {
fn version_or_url(&self) -> VersionOrUrlRef {
match self {
Self::Installed(installed) => installed.version_or_url(),
Self::Installable(dist) => dist.version_or_url(),
Self::Installed { dist } => dist.version_or_url(),
Self::Installable { dist, .. } => dist.version_or_url(),
}
}
}

impl Identifier for ResolvedDist {
fn distribution_id(&self) -> DistributionId {
match self {
Self::Installed(dist) => dist.distribution_id(),
Self::Installable(dist) => dist.distribution_id(),
Self::Installed { dist } => dist.distribution_id(),
Self::Installable { dist, .. } => dist.distribution_id(),
}
}

fn resource_id(&self) -> ResourceId {
match self {
Self::Installed(dist) => dist.resource_id(),
Self::Installable(dist) => dist.resource_id(),
Self::Installed { dist } => dist.resource_id(),
Self::Installable { dist, .. } => dist.resource_id(),
}
}
}

impl From<Dist> for ResolvedDist {
fn from(value: Dist) -> Self {
ResolvedDist::Installable(value)
}
}

impl Display for ResolvedDist {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match self {
Self::Installed(dist) => dist.fmt(f),
Self::Installable(dist) => dist.fmt(f),
Self::Installed { dist } => dist.fmt(f),
Self::Installable { dist, .. } => dist.fmt(f),
}
}
}
10 changes: 5 additions & 5 deletions crates/uv-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,18 @@ impl<'a> Planner<'a> {
}
}

let ResolvedDist::Installable(installable) = dist else {
let ResolvedDist::Installable { dist, .. } = dist else {
unreachable!("Installed distribution could not be found in site-packages: {dist}");
};

if cache.must_revalidate(dist.name()) {
debug!("Must revalidate requirement: {}", dist.name());
remote.push(installable.clone());
remote.push(dist.clone());
continue;
}

// Identify any cached distributions that satisfy the requirement.
match installable {
match dist {
Dist::Built(BuiltDist::Registry(wheel)) => {
if let Some(distribution) = registry_index.get(wheel.name()).find_map(|entry| {
if *entry.index.url() != wheel.best_wheel().index {
Expand Down Expand Up @@ -309,8 +309,8 @@ impl<'a> Planner<'a> {
}
}

debug!("Identified uncached distribution: {installable}");
remote.push(installable.clone());
debug!("Identified uncached distribution: {dist}");
remote.push(dist.clone());
}

// Remove any unnecessary packages.
Expand Down
12 changes: 6 additions & 6 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2360,8 +2360,8 @@ impl Source {
fn from_resolved_dist(resolved_dist: &ResolvedDist, root: &Path) -> Result<Source, LockError> {
match *resolved_dist {
// We pass empty installed packages for locking.
ResolvedDist::Installed(_) => unreachable!(),
ResolvedDist::Installable(ref dist) => Source::from_dist(dist, root),
ResolvedDist::Installed { .. } => unreachable!(),
ResolvedDist::Installable { ref dist, .. } => Source::from_dist(dist, root),
}
}

Expand Down Expand Up @@ -2892,8 +2892,8 @@ impl SourceDist {
) -> Result<Option<SourceDist>, LockError> {
match annotated_dist.dist {
// We pass empty installed packages for locking.
ResolvedDist::Installed(_) => unreachable!(),
ResolvedDist::Installable(ref dist) => {
ResolvedDist::Installed { .. } => unreachable!(),
ResolvedDist::Installable { ref dist, .. } => {
SourceDist::from_dist(id, dist, &annotated_dist.hashes, annotated_dist.index())
}
}
Expand Down Expand Up @@ -3174,8 +3174,8 @@ impl Wheel {
fn from_annotated_dist(annotated_dist: &AnnotatedDist) -> Result<Vec<Wheel>, LockError> {
match annotated_dist.dist {
// We pass empty installed packages for locking.
ResolvedDist::Installed(_) => unreachable!(),
ResolvedDist::Installable(ref dist) => {
ResolvedDist::Installed { .. } => unreachable!(),
ResolvedDist::Installable { ref dist, .. } => {
Wheel::from_dist(dist, &annotated_dist.hashes, annotated_dist.index())
}
}
Expand Down
13 changes: 8 additions & 5 deletions crates/uv-resolver/src/lock/target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,11 +266,14 @@ impl<'env> InstallTarget<'env> {
) {
map.insert(
dist.id.name.clone(),
ResolvedDist::Installable(dist.to_dist(
self.workspace().install_path(),
TagPolicy::Required(tags),
build_options,
)?),
ResolvedDist::Installable {
dist: dist.to_dist(
self.workspace().install_path(),
TagPolicy::Required(tags),
build_options,
)?,
version: dist.id.version.clone(),
},
);
hashes.insert(dist.id.name.clone(), dist.hashes());
}
Expand Down
9 changes: 8 additions & 1 deletion crates/uv-resolver/src/resolution/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,14 @@ impl ResolutionGraph {
archive.metadata.clone()
};

(dist.into(), hashes, Some(metadata))
(
ResolvedDist::Installable {
dist,
version: version.clone(),
},
hashes,
Some(metadata),
)
} else {
let dist = pins
.get(name, version)
Expand Down
4 changes: 2 additions & 2 deletions crates/uv-resolver/src/resolution/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ impl AnnotatedDist {
/// Returns the [`IndexUrl`] of the distribution, if it is from a registry.
pub(crate) fn index(&self) -> Option<&IndexUrl> {
match &self.dist {
ResolvedDist::Installed(_) => None,
ResolvedDist::Installable(dist) => match dist {
ResolvedDist::Installed { .. } => None,
ResolvedDist::Installable { dist, .. } => match dist {
Dist::Built(dist) => match dist {
BuiltDist::Registry(dist) => Some(&dist.best_wheel().index),
BuiltDist::DirectUrl(_) => None,
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
ResolvedDistRef::InstallableRegistryBuiltDist { wheel, .. } => wheel
.filename()
.unwrap_or(Cow::Borrowed("unknown filename")),
ResolvedDistRef::Installed(_) => Cow::Borrowed("installed"),
ResolvedDistRef::Installed { .. } => Cow::Borrowed("installed"),
};

debug!(
Expand Down Expand Up @@ -1958,15 +1958,15 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let dist = dist.for_resolution().to_owned();

let response = match dist {
ResolvedDist::Installable(dist) => {
ResolvedDist::Installable { dist, .. } => {
let metadata = provider
.get_or_build_wheel_metadata(&dist)
.boxed_local()
.await?;

Response::Dist { dist, metadata }
}
ResolvedDist::Installed(dist) => {
ResolvedDist::Installed { dist } => {
let metadata = dist.metadata().map_err(|err| {
ResolveError::ReadInstalled(Box::new(dist.clone()), err)
})?;
Expand Down Expand Up @@ -2621,7 +2621,7 @@ impl<'a> From<ResolvedDistRef<'a>> for Request {
let built = prioritized.built_dist().expect("at least one wheel");
Request::Dist(Dist::Built(BuiltDist::Registry(built)))
}
ResolvedDistRef::Installed(dist) => Request::Installed(dist.clone()),
ResolvedDistRef::Installed { dist } => Request::Installed(dist.clone()),
}
}
}
Expand Down
Loading

0 comments on commit 9339e55

Please sign in to comment.