Skip to content

Commit

Permalink
Fall back to distributions without hashes in resolver (#2949)
Browse files Browse the repository at this point in the history
## Summary

This represents a change to `--require-hashes` in the event that we
don't find a matching hash from the registry. The behavior in this PR is
closer to pip's.

Prior to this PR, if a distribution had no reported hash, or only
mismatched hashes, we would mark it as incompatible. Now, we mark it as
compatible, but we use the hash-agreement as part of the ordering, such
that we prefer any distribution with a matching hash, then any
distribution with no hash, then any distribution with a mismatched hash.

As a result, if an index reports incorrect hashes, but the user provides
the correct one, resolution now succeeds, where it would've failed.

Similarly, if an index omits hashes altogether, but the user provides
the correct one, resolution now succeeds, where it would've failed.

If we end up picking a distribution whose hash ultimately doesn't match,
we'll reject it later, after resolution.
  • Loading branch information
charliermarsh authored Apr 10, 2024
1 parent 1f3b5bb commit c18551f
Show file tree
Hide file tree
Showing 5 changed files with 319 additions and 183 deletions.
128 changes: 48 additions & 80 deletions crates/distribution-types/src/prioritized_distribution.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fmt::{Display, Formatter};

use pep440_rs::VersionSpecifiers;
use platform_tags::{IncompatibleTag, TagCompatibility, TagPriority};
use platform_tags::{IncompatibleTag, TagPriority};
use pypi_types::{HashDigest, Yanked};

use crate::{Dist, InstalledDist, ResolvedDistRef};
Expand Down Expand Up @@ -84,8 +84,6 @@ impl Display for IncompatibleDist {
IncompatibleWheel::RequiresPython(python) => {
write!(f, "it requires at python {python}")
}
IncompatibleWheel::MissingHash => f.write_str("it has no hash"),
IncompatibleWheel::MismatchedHash => f.write_str("the hash does not match"),
},
Self::Source(incompatibility) => match incompatibility {
IncompatibleSource::NoBuild => {
Expand All @@ -106,8 +104,6 @@ impl Display for IncompatibleDist {
IncompatibleSource::RequiresPython(python) => {
write!(f, "it requires python {python}")
}
IncompatibleSource::MissingHash => f.write_str("it has no hash"),
IncompatibleSource::MismatchedHash => f.write_str("the hash does not match"),
},
Self::Unavailable => f.write_str("no distributions are available"),
}
Expand All @@ -117,7 +113,7 @@ impl Display for IncompatibleDist {
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum WheelCompatibility {
Incompatible(IncompatibleWheel),
Compatible(TagPriority),
Compatible(Hash, TagPriority),
}

#[derive(Debug, PartialEq, Eq, Clone)]
Expand All @@ -126,27 +122,33 @@ pub enum IncompatibleWheel {
Tag(IncompatibleTag),
RequiresPython(VersionSpecifiers),
Yanked(Yanked),
MissingHash,
MismatchedHash,
NoBinary,
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum SourceDistCompatibility {
Incompatible(IncompatibleSource),
Compatible,
Compatible(Hash),
}

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum IncompatibleSource {
ExcludeNewer(Option<i64>),
RequiresPython(VersionSpecifiers),
Yanked(Yanked),
MissingHash,
MismatchedHash,
NoBuild,
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
pub enum Hash {
/// The hash is present, but does not match the expected value.
Mismatched,
/// The hash is missing.
Missing,
/// The hash matches the expected value.
Matched,
}

impl PrioritizedDist {
/// Create a new [`PrioritizedDist`] from the given wheel distribution.
pub fn from_built(
Expand Down Expand Up @@ -215,8 +217,19 @@ impl PrioritizedDist {
/// Return the highest-priority distribution for the package version, if any.
pub fn get(&self) -> Option<CompatibleDist> {
match (&self.0.wheel, &self.0.source) {
// If both are compatible, break ties based on the hash.
(
Some((wheel, WheelCompatibility::Compatible(wheel_hash, tag_priority))),
Some((source_dist, SourceDistCompatibility::Compatible(source_hash))),
) => {
if source_hash > wheel_hash {
Some(CompatibleDist::SourceDist(source_dist))
} else {
Some(CompatibleDist::CompatibleWheel(wheel, *tag_priority))
}
}
// Prefer the highest-priority, platform-compatible wheel.
(Some((wheel, WheelCompatibility::Compatible(tag_priority))), _) => {
(Some((wheel, WheelCompatibility::Compatible(_, tag_priority))), _) => {
Some(CompatibleDist::CompatibleWheel(wheel, *tag_priority))
}
// If we have a compatible source distribution and an incompatible wheel, return the
Expand All @@ -225,58 +238,36 @@ impl PrioritizedDist {
// using the wheel is faster.
(
Some((wheel, WheelCompatibility::Incompatible(_))),
Some((source_dist, SourceDistCompatibility::Compatible)),
Some((source_dist, SourceDistCompatibility::Compatible(_))),
) => Some(CompatibleDist::IncompatibleWheel { source_dist, wheel }),
// Otherwise, if we have a source distribution, return it.
(None, Some((source_dist, SourceDistCompatibility::Compatible))) => {
(None, Some((source_dist, SourceDistCompatibility::Compatible(_)))) => {
Some(CompatibleDist::SourceDist(source_dist))
}
_ => None,
}
}

/// Return the compatible source distribution, if any.
pub fn compatible_source(&self) -> Option<&Dist> {
self.0
.source
.as_ref()
.and_then(|(dist, compatibility)| match compatibility {
SourceDistCompatibility::Compatible => Some(dist),
SourceDistCompatibility::Incompatible(_) => None,
})
}

/// Return the incompatible source distribution, if any.
pub fn incompatible_source(&self) -> Option<(&Dist, &IncompatibleSource)> {
self.0
.source
.as_ref()
.and_then(|(dist, compatibility)| match compatibility {
SourceDistCompatibility::Compatible => None,
SourceDistCompatibility::Compatible(_) => None,
SourceDistCompatibility::Incompatible(incompatibility) => {
Some((dist, incompatibility))
}
})
}

/// Return the compatible built distribution, if any.
pub fn compatible_wheel(&self) -> Option<(&Dist, TagPriority)> {
self.0
.wheel
.as_ref()
.and_then(|(dist, compatibility)| match compatibility {
WheelCompatibility::Compatible(priority) => Some((dist, *priority)),
WheelCompatibility::Incompatible(_) => None,
})
}

/// Return the incompatible built distribution, if any.
pub fn incompatible_wheel(&self) -> Option<(&Dist, &IncompatibleWheel)> {
self.0
.wheel
.as_ref()
.and_then(|(dist, compatibility)| match compatibility {
WheelCompatibility::Compatible(_) => None,
WheelCompatibility::Compatible(_, _) => None,
WheelCompatibility::Incompatible(incompatibility) => Some((dist, incompatibility)),
})
}
Expand Down Expand Up @@ -335,7 +326,7 @@ impl<'a> CompatibleDist<'a> {

impl WheelCompatibility {
pub fn is_compatible(&self) -> bool {
matches!(self, Self::Compatible(_))
matches!(self, Self::Compatible(_, _))
}

/// Return `true` if the current compatibility is more compatible than another.
Expand All @@ -344,11 +335,12 @@ impl WheelCompatibility {
/// Compatible wheel ordering is determined by tag priority.
pub fn is_more_compatible(&self, other: &Self) -> bool {
match (self, other) {
(Self::Compatible(_), Self::Incompatible(_)) => true,
(Self::Compatible(tag_priority), Self::Compatible(other_tag_priority)) => {
tag_priority > other_tag_priority
}
(Self::Incompatible(_), Self::Compatible(_)) => false,
(Self::Compatible(_, _), Self::Incompatible(_)) => true,
(
Self::Compatible(hash, tag_priority),
Self::Compatible(other_hash, other_tag_priority),
) => (hash, tag_priority) > (other_hash, other_tag_priority),
(Self::Incompatible(_), Self::Compatible(_, _)) => false,
(Self::Incompatible(incompatibility), Self::Incompatible(other_incompatibility)) => {
incompatibility.is_more_compatible(other_incompatibility)
}
Expand All @@ -364,51 +356,38 @@ impl SourceDistCompatibility {
/// Incompatible source distribution priority selects a source distribution that was "closest" to being usable.
pub fn is_more_compatible(&self, other: &Self) -> bool {
match (self, other) {
(Self::Compatible, Self::Incompatible(_)) => true,
(Self::Compatible, Self::Compatible) => false, // Arbitrary
(Self::Incompatible(_), Self::Compatible) => false,
(Self::Compatible(_), Self::Incompatible(_)) => true,
(Self::Compatible(compatibility), Self::Compatible(other_compatibility)) => {
compatibility > other_compatibility
}
(Self::Incompatible(_), Self::Compatible(_)) => false,
(Self::Incompatible(incompatibility), Self::Incompatible(other_incompatibility)) => {
incompatibility.is_more_compatible(other_incompatibility)
}
}
}
}

impl From<TagCompatibility> for WheelCompatibility {
fn from(value: TagCompatibility) -> Self {
match value {
TagCompatibility::Compatible(priority) => Self::Compatible(priority),
TagCompatibility::Incompatible(tag) => Self::Incompatible(IncompatibleWheel::Tag(tag)),
}
}
}

impl IncompatibleSource {
fn is_more_compatible(&self, other: &Self) -> bool {
match self {
Self::ExcludeNewer(timestamp_self) => match other {
// Smaller timestamps are closer to the cut-off time
Self::ExcludeNewer(timestamp_other) => timestamp_other < timestamp_self,
Self::NoBuild
| Self::RequiresPython(_)
| Self::Yanked(_)
| Self::MissingHash
| Self::MismatchedHash => true,
Self::NoBuild | Self::RequiresPython(_) | Self::Yanked(_) => true,
},
Self::RequiresPython(_) => match other {
Self::ExcludeNewer(_) => false,
// Version specifiers cannot be reasonably compared
Self::RequiresPython(_) => false,
Self::NoBuild | Self::Yanked(_) | Self::MissingHash | Self::MismatchedHash => true,
Self::NoBuild | Self::Yanked(_) => true,
},
Self::Yanked(_) => match other {
Self::ExcludeNewer(_) | Self::RequiresPython(_) => false,
// Yanks with a reason are more helpful for errors
Self::Yanked(yanked_other) => matches!(yanked_other, Yanked::Reason(_)),
Self::NoBuild | Self::MissingHash | Self::MismatchedHash => true,
Self::NoBuild => true,
},
Self::MissingHash => false,
Self::MismatchedHash => false,
Self::NoBuild => false,
}
}
Expand All @@ -426,37 +405,26 @@ impl IncompatibleWheel {
timestamp_other < timestamp_self
}
},
Self::NoBinary
| Self::RequiresPython(_)
| Self::Tag(_)
| Self::Yanked(_)
| Self::MissingHash
| Self::MismatchedHash => true,
Self::NoBinary | Self::RequiresPython(_) | Self::Tag(_) | Self::Yanked(_) => true,
},
Self::Tag(tag_self) => match other {
Self::ExcludeNewer(_) => false,
Self::Tag(tag_other) => tag_other > tag_self,
Self::NoBinary
| Self::RequiresPython(_)
| Self::Yanked(_)
| Self::MissingHash
| Self::MismatchedHash => true,
Self::NoBinary | Self::RequiresPython(_) | Self::Yanked(_) => true,
},
Self::RequiresPython(_) => match other {
Self::ExcludeNewer(_) | Self::Tag(_) => false,
// Version specifiers cannot be reasonably compared
Self::RequiresPython(_) => false,
Self::NoBinary | Self::Yanked(_) | Self::MissingHash | Self::MismatchedHash => true,
Self::NoBinary | Self::Yanked(_) => true,
},
Self::Yanked(_) => match other {
Self::ExcludeNewer(_) | Self::Tag(_) | Self::RequiresPython(_) => false,
// Yanks with a reason are more helpful for errors
Self::Yanked(yanked_other) => matches!(yanked_other, Yanked::Reason(_)),
Self::NoBinary | Self::MissingHash | Self::MismatchedHash => true,
Self::NoBinary => true,
},
Self::NoBinary => false,
Self::MismatchedHash => false,
Self::MissingHash => false,
}
}
}
57 changes: 32 additions & 25 deletions crates/uv-resolver/src/flat_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ use tracing::instrument;

use distribution_filename::{DistFilename, SourceDistFilename, WheelFilename};
use distribution_types::{
BuiltDist, Dist, File, IncompatibleSource, IncompatibleWheel, IndexUrl, PrioritizedDist,
BuiltDist, Dist, File, Hash, IncompatibleSource, IncompatibleWheel, IndexUrl, PrioritizedDist,
RegistryBuiltDist, RegistrySourceDist, SourceDist, SourceDistCompatibility, WheelCompatibility,
};
use pep440_rs::Version;
use platform_tags::Tags;
use platform_tags::{TagCompatibility, Tags};
use pypi_types::HashDigest;
use uv_client::FlatIndexEntries;
use uv_configuration::{NoBinary, NoBuild};
Expand Down Expand Up @@ -140,20 +140,19 @@ impl FlatIndex {
}

// Check if hashes line up
if let Some(required_hashes) = required_hashes.get(&filename.name) {
if !required_hashes.is_empty() {
if hashes.is_empty() {
return SourceDistCompatibility::Incompatible(IncompatibleSource::MissingHash);
}
if !hashes.iter().any(|hash| required_hashes.contains(hash)) {
return SourceDistCompatibility::Incompatible(
IncompatibleSource::MismatchedHash,
);
}
let hash = if let Some(required_hashes) = required_hashes.get(&filename.name) {
if hashes.is_empty() {
Hash::Missing
} else if hashes.iter().any(|hash| required_hashes.contains(hash)) {
Hash::Matched
} else {
Hash::Mismatched
}
}
} else {
Hash::Matched
};

SourceDistCompatibility::Compatible
SourceDistCompatibility::Compatible(hash)
}

fn wheel_compatibility(
Expand All @@ -174,20 +173,28 @@ impl FlatIndex {
return WheelCompatibility::Incompatible(IncompatibleWheel::NoBinary);
}

// Determine a compatibility for the wheel based on tags.
let priority = match filename.compatibility(tags) {
TagCompatibility::Incompatible(tag) => {
return WheelCompatibility::Incompatible(IncompatibleWheel::Tag(tag))
}
TagCompatibility::Compatible(priority) => priority,
};

// Check if hashes line up
if let Some(required_hashes) = required_hashes.get(&filename.name) {
if !required_hashes.is_empty() {
if hashes.is_empty() {
return WheelCompatibility::Incompatible(IncompatibleWheel::MissingHash);
}
if !hashes.iter().any(|hash| required_hashes.contains(hash)) {
return WheelCompatibility::Incompatible(IncompatibleWheel::MismatchedHash);
}
let hash = if let Some(required_hashes) = required_hashes.get(&filename.name) {
if hashes.is_empty() {
Hash::Missing
} else if hashes.iter().any(|hash| required_hashes.contains(hash)) {
Hash::Matched
} else {
Hash::Mismatched
}
}
} else {
Hash::Matched
};

// Determine a compatibility for the wheel based on tags.
WheelCompatibility::from(filename.compatibility(tags))
WheelCompatibility::Compatible(hash, priority)
}

/// Get the [`FlatDistributions`] for the given package name.
Expand Down
Loading

0 comments on commit c18551f

Please sign in to comment.