Skip to content

Commit

Permalink
Refactor unavailable metadata to shrink the resolver (#9769)
Browse files Browse the repository at this point in the history
The resolver methods are already too large and complex, especially
`choose_version*`, so i wanted to shrink and simplify them a bit before
adding new methods to them.

I've split `MetadataResponse` into three variants: success, non-fatal
error (reported through pubgrub), fatal error (reported as error trace).
The resulting non-fatal `MetadataUnavailable` type is equivalent to the
`IncompletePackage` type, so they are now merged. (`UnavailableVersion`
is a bit different since, besides the extra `IncompatibleDist` variant,
it have no error source attached). This shows that the missing metadata
variant was unused, which I removed.

Tagging as error messages for the logging format changes.
  • Loading branch information
konstin authored Dec 10, 2024
1 parent edf875e commit b751648
Show file tree
Hide file tree
Showing 6 changed files with 137 additions and 255 deletions.
8 changes: 4 additions & 4 deletions crates/uv-installer/src/site_packages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ impl SitePackages {

// Determine the dependencies for the given package.
let Ok(metadata) = distribution.metadata() else {
diagnostics.push(SitePackagesDiagnostic::IncompletePackage {
diagnostics.push(SitePackagesDiagnostic::MetadataUnavailable {
package: package.clone(),
path: distribution.path().to_owned(),
});
Expand Down Expand Up @@ -405,7 +405,7 @@ impl IntoIterator for SitePackages {

#[derive(Debug)]
pub enum SitePackagesDiagnostic {
IncompletePackage {
MetadataUnavailable {
/// The package that is missing metadata.
package: PackageName,
/// The path to the package.
Expand Down Expand Up @@ -445,7 +445,7 @@ impl Diagnostic for SitePackagesDiagnostic {
/// Convert the diagnostic into a user-facing message.
fn message(&self) -> String {
match self {
Self::IncompletePackage { package, path } => format!(
Self::MetadataUnavailable { package, path } => format!(
"The package `{package}` is broken or incomplete (unable to read `METADATA`). Consider recreating the virtualenv, or removing the package directory at: {}.", path.display(),
),
Self::IncompatiblePythonVersion {
Expand Down Expand Up @@ -482,7 +482,7 @@ impl Diagnostic for SitePackagesDiagnostic {
/// Returns `true` if the [`PackageName`] is involved in this diagnostic.
fn includes(&self, name: &PackageName) -> bool {
match self {
Self::IncompletePackage { package, .. } => name == package,
Self::MetadataUnavailable { package, .. } => name == package,
Self::IncompatiblePythonVersion { package, .. } => name == package,
Self::MissingDependency { package, .. } => name == package,
Self::IncompatibleDependency {
Expand Down
6 changes: 3 additions & 3 deletions crates/uv-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::pubgrub::{PubGrubPackage, PubGrubPackageInner, PubGrubReportFormatter
use crate::python_requirement::PythonRequirement;
use crate::resolution::ConflictingDistributionError;
use crate::resolver::{
IncompletePackage, ResolverEnvironment, UnavailablePackage, UnavailableReason,
MetadataUnavailable, ResolverEnvironment, UnavailablePackage, UnavailableReason,
};
use crate::Options;

Expand Down Expand Up @@ -145,7 +145,7 @@ pub struct NoSolutionError {
index_locations: IndexLocations,
index_capabilities: IndexCapabilities,
unavailable_packages: FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, MetadataUnavailable>>,
fork_urls: ForkUrls,
env: ResolverEnvironment,
workspace_members: BTreeSet<PackageName>,
Expand All @@ -163,7 +163,7 @@ impl NoSolutionError {
index_locations: IndexLocations,
index_capabilities: IndexCapabilities,
unavailable_packages: FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, MetadataUnavailable>>,
fork_urls: ForkUrls,
env: ResolverEnvironment,
workspace_members: BTreeSet<PackageName>,
Expand Down
72 changes: 11 additions & 61 deletions crates/uv-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::error::ErrorTree;
use crate::fork_urls::ForkUrls;
use crate::prerelease::AllowPrerelease;
use crate::python_requirement::{PythonRequirement, PythonRequirementSource};
use crate::resolver::{IncompletePackage, UnavailablePackage, UnavailableReason};
use crate::resolver::{MetadataUnavailable, UnavailablePackage, UnavailableReason};
use crate::{Flexibility, Options, RequiresPython, ResolverEnvironment};

use super::{PubGrubPackage, PubGrubPackageInner, PubGrubPython};
Expand Down Expand Up @@ -548,7 +548,7 @@ impl PubGrubReportFormatter<'_> {
index_capabilities: &IndexCapabilities,
available_indexes: &FxHashMap<PackageName, BTreeSet<IndexUrl>>,
unavailable_packages: &FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: &FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
incomplete_packages: &FxHashMap<PackageName, BTreeMap<Version, MetadataUnavailable>>,
fork_urls: &ForkUrls,
env: &ResolverEnvironment,
workspace_members: &BTreeSet<PackageName>,
Expand Down Expand Up @@ -679,7 +679,7 @@ impl PubGrubReportFormatter<'_> {
index_capabilities: &IndexCapabilities,
available_indexes: &FxHashMap<PackageName, BTreeSet<IndexUrl>>,
unavailable_packages: &FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: &FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
incomplete_packages: &FxHashMap<PackageName, BTreeMap<Version, MetadataUnavailable>>,
hints: &mut IndexSet<PubGrubHint>,
) {
let no_find_links = index_locations.flat_indexes().peekable().peek().is_none();
Expand All @@ -694,11 +694,6 @@ impl PubGrubReportFormatter<'_> {
Some(UnavailablePackage::Offline) => {
hints.insert(PubGrubHint::Offline);
}
Some(UnavailablePackage::MissingMetadata) => {
hints.insert(PubGrubHint::MissingPackageMetadata {
package: package.clone(),
});
}
Some(UnavailablePackage::InvalidMetadata(reason)) => {
hints.insert(PubGrubHint::InvalidPackageMetadata {
package: package.clone(),
Expand All @@ -720,37 +715,31 @@ impl PubGrubReportFormatter<'_> {
for (version, incomplete) in versions.iter().rev() {
if set.contains(version) {
match incomplete {
IncompletePackage::Offline => {
MetadataUnavailable::Offline => {
hints.insert(PubGrubHint::Offline);
}
IncompletePackage::MissingMetadata => {
hints.insert(PubGrubHint::MissingVersionMetadata {
package: package.clone(),
version: version.clone(),
});
}
IncompletePackage::InvalidMetadata(reason) => {
MetadataUnavailable::InvalidMetadata(reason) => {
hints.insert(PubGrubHint::InvalidVersionMetadata {
package: package.clone(),
version: version.clone(),
reason: reason.clone(),
reason: reason.to_string(),
});
}
IncompletePackage::InconsistentMetadata(reason) => {
MetadataUnavailable::InconsistentMetadata(reason) => {
hints.insert(PubGrubHint::InconsistentVersionMetadata {
package: package.clone(),
version: version.clone(),
reason: reason.clone(),
reason: reason.to_string(),
});
}
IncompletePackage::InvalidStructure(reason) => {
MetadataUnavailable::InvalidStructure(reason) => {
hints.insert(PubGrubHint::InvalidVersionStructure {
package: package.clone(),
version: version.clone(),
reason: reason.clone(),
reason: reason.to_string(),
});
}
IncompletePackage::RequiresPython(requires_python, python_version) => {
MetadataUnavailable::RequiresPython(requires_python, python_version) => {
hints.insert(PubGrubHint::IncompatibleBuildRequirement {
package: package.clone(),
version: version.clone(),
Expand Down Expand Up @@ -882,8 +871,6 @@ pub(crate) enum PubGrubHint {
NoIndex,
/// A package was not found in the registry, but network access was disabled.
Offline,
/// Metadata for a package could not be found.
MissingPackageMetadata { package: PubGrubPackage },
/// Metadata for a package could not be parsed.
InvalidPackageMetadata {
package: PubGrubPackage,
Expand All @@ -896,12 +883,6 @@ pub(crate) enum PubGrubHint {
// excluded from `PartialEq` and `Hash`
reason: String,
},
/// Metadata for a package version could not be found.
MissingVersionMetadata {
package: PubGrubPackage,
// excluded from `PartialEq` and `Hash`
version: Version,
},
/// Metadata for a package version could not be parsed.
InvalidVersionMetadata {
package: PubGrubPackage,
Expand Down Expand Up @@ -992,18 +973,12 @@ enum PubGrubHintCore {
},
NoIndex,
Offline,
MissingPackageMetadata {
package: PubGrubPackage,
},
InvalidPackageMetadata {
package: PubGrubPackage,
},
InvalidPackageStructure {
package: PubGrubPackage,
},
MissingVersionMetadata {
package: PubGrubPackage,
},
InvalidVersionMetadata {
package: PubGrubPackage,
},
Expand Down Expand Up @@ -1052,18 +1027,12 @@ impl From<PubGrubHint> for PubGrubHintCore {
}
PubGrubHint::NoIndex => Self::NoIndex,
PubGrubHint::Offline => Self::Offline,
PubGrubHint::MissingPackageMetadata { package, .. } => {
Self::MissingPackageMetadata { package }
}
PubGrubHint::InvalidPackageMetadata { package, .. } => {
Self::InvalidPackageMetadata { package }
}
PubGrubHint::InvalidPackageStructure { package, .. } => {
Self::InvalidPackageStructure { package }
}
PubGrubHint::MissingVersionMetadata { package, .. } => {
Self::MissingVersionMetadata { package }
}
PubGrubHint::InvalidVersionMetadata { package, .. } => {
Self::InvalidVersionMetadata { package }
}
Expand Down Expand Up @@ -1162,15 +1131,6 @@ impl std::fmt::Display for PubGrubHint {
":".bold(),
)
}
Self::MissingPackageMetadata { package } => {
write!(
f,
"{}{} Metadata for `{}` could not be found, as the wheel is missing a `METADATA` file",
"hint".bold().cyan(),
":".bold(),
package.bold()
)
}
Self::InvalidPackageMetadata { package, reason } => {
write!(
f,
Expand All @@ -1191,16 +1151,6 @@ impl std::fmt::Display for PubGrubHint {
textwrap::indent(reason, " ")
)
}
Self::MissingVersionMetadata { package, version } => {
write!(
f,
"{}{} Metadata for `{}` ({}) could not be found, as the wheel is missing a `METADATA` file",
"hint".bold().cyan(),
":".bold(),
package.cyan(),
format!("v{version}").cyan(),
)
}
Self::InvalidVersionMetadata {
package,
version,
Expand Down
62 changes: 34 additions & 28 deletions crates/uv-resolver/src/resolver/availability.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::fmt::{Display, Formatter};

use crate::resolver::MetadataUnavailable;
use uv_distribution_types::IncompatibleDist;
use uv_pep440::{Version, VersionSpecifiers};

Expand All @@ -21,17 +22,15 @@ impl Display for UnavailableReason {
}
}

/// The package version is unavailable and cannot be used. Unlike [`PackageUnavailable`], this
/// The package version is unavailable and cannot be used. Unlike [`MetadataUnavailable`], this
/// applies to a single version of the package.
///
/// Most variant are from [`MetadataResponse`] without the error source (since we don't format
/// the source).
/// Most variant are from [`MetadataResponse`] without the error source, since we don't format
/// the source and we want to merge unavailable messages across versions.
#[derive(Debug, Clone, Eq, PartialEq)]
pub(crate) enum UnavailableVersion {
/// Version is incompatible because it has no usable distributions
IncompatibleDist(IncompatibleDist),
/// The wheel metadata was not found.
MissingMetadata,
/// The wheel metadata was found, but could not be parsed.
InvalidMetadata,
/// The wheel metadata was found, but the metadata was inconsistent.
Expand All @@ -49,7 +48,6 @@ impl UnavailableVersion {
pub(crate) fn message(&self) -> String {
match self {
UnavailableVersion::IncompatibleDist(invalid_dist) => format!("{invalid_dist}"),
UnavailableVersion::MissingMetadata => "not include a `METADATA` file".into(),
UnavailableVersion::InvalidMetadata => "invalid metadata".into(),
UnavailableVersion::InconsistentMetadata => "inconsistent metadata".into(),
UnavailableVersion::InvalidStructure => "an invalid package format".into(),
Expand All @@ -63,7 +61,6 @@ impl UnavailableVersion {
pub(crate) fn singular_message(&self) -> String {
match self {
UnavailableVersion::IncompatibleDist(invalid_dist) => invalid_dist.singular_message(),
UnavailableVersion::MissingMetadata => format!("does {self}"),
UnavailableVersion::InvalidMetadata => format!("has {self}"),
UnavailableVersion::InconsistentMetadata => format!("has {self}"),
UnavailableVersion::InvalidStructure => format!("has {self}"),
Expand All @@ -75,7 +72,6 @@ impl UnavailableVersion {
pub(crate) fn plural_message(&self) -> String {
match self {
UnavailableVersion::IncompatibleDist(invalid_dist) => invalid_dist.plural_message(),
UnavailableVersion::MissingMetadata => format!("do {self}"),
UnavailableVersion::InvalidMetadata => format!("have {self}"),
UnavailableVersion::InconsistentMetadata => format!("have {self}"),
UnavailableVersion::InvalidStructure => format!("have {self}"),
Expand All @@ -91,6 +87,22 @@ impl Display for UnavailableVersion {
}
}

impl From<&MetadataUnavailable> for UnavailableVersion {
fn from(reason: &MetadataUnavailable) -> Self {
match reason {
MetadataUnavailable::Offline => UnavailableVersion::Offline,
MetadataUnavailable::InvalidMetadata(_) => UnavailableVersion::InvalidMetadata,
MetadataUnavailable::InconsistentMetadata(_) => {
UnavailableVersion::InconsistentMetadata
}
MetadataUnavailable::InvalidStructure(_) => UnavailableVersion::InvalidStructure,
MetadataUnavailable::RequiresPython(requires_python, _python_version) => {
UnavailableVersion::RequiresPython(requires_python.clone())
}
}
}
}

/// The package is unavailable and cannot be used.
#[derive(Debug, Clone, Eq, PartialEq)]
pub(crate) enum UnavailablePackage {
Expand All @@ -100,8 +112,6 @@ pub(crate) enum UnavailablePackage {
Offline,
/// The package was not found in the registry.
NotFound,
/// The package metadata was not found.
MissingMetadata,
/// The package metadata was found, but could not be parsed.
InvalidMetadata(String),
/// The package has an invalid structure.
Expand All @@ -114,7 +124,6 @@ impl UnavailablePackage {
UnavailablePackage::NoIndex => "not found in the provided package locations",
UnavailablePackage::Offline => "not found in the cache",
UnavailablePackage::NotFound => "not found in the package registry",
UnavailablePackage::MissingMetadata => "not include a `METADATA` file",
UnavailablePackage::InvalidMetadata(_) => "invalid metadata",
UnavailablePackage::InvalidStructure(_) => "an invalid package format",
}
Expand All @@ -125,7 +134,6 @@ impl UnavailablePackage {
UnavailablePackage::NoIndex => format!("was {self}"),
UnavailablePackage::Offline => format!("was {self}"),
UnavailablePackage::NotFound => format!("was {self}"),
UnavailablePackage::MissingMetadata => format!("does {self}"),
UnavailablePackage::InvalidMetadata(_) => format!("has {self}"),
UnavailablePackage::InvalidStructure(_) => format!("has {self}"),
}
Expand All @@ -138,22 +146,20 @@ impl Display for UnavailablePackage {
}
}

/// The package is unavailable at specific versions.
#[derive(Debug, Clone)]
pub(crate) enum IncompletePackage {
/// Network requests were disabled (i.e., `--offline`), and the wheel metadata was not found in the cache.
Offline,
/// The wheel metadata was not found.
MissingMetadata,
/// The wheel metadata was found, but could not be parsed.
InvalidMetadata(String),
/// The wheel metadata was found, but the metadata was inconsistent.
InconsistentMetadata(String),
/// The wheel has an invalid structure.
InvalidStructure(String),
/// The source distribution has a `requires-python` requirement that is not met by the installed
/// Python version (and static metadata is not available).
RequiresPython(VersionSpecifiers, Version),
impl From<&MetadataUnavailable> for UnavailablePackage {
fn from(reason: &MetadataUnavailable) -> Self {
match reason {
MetadataUnavailable::Offline => Self::Offline,
MetadataUnavailable::InvalidMetadata(err) => Self::InvalidMetadata(err.to_string()),
MetadataUnavailable::InconsistentMetadata(err) => {
Self::InvalidMetadata(err.to_string())
}
MetadataUnavailable::InvalidStructure(err) => Self::InvalidStructure(err.to_string()),
MetadataUnavailable::RequiresPython(..) => {
unreachable!("`requires-python` is only known upfront for registry distributions")
}
}
}
}

#[derive(Debug, Clone)]
Expand Down
Loading

0 comments on commit b751648

Please sign in to comment.