Add hashes and versions to all distributions #3589
Conversation
| } | ||
| } | ||
|
|
||
| fn from_source_dist( | ||
| source_dist: &distribution_types::SourceDist, | ||
| hashes: &[HashDigest], |
There was a problem hiding this comment.
Don't love making hashes an argument everywhere but seemed like the most straightforward option (since it only exists on AnnotatedDist).
There was a problem hiding this comment.
Yeah hmmm, I don't like it either.
I think when I was writing this, I was assuming the hashes would be added to types like PathSourceDist directly. Out of curiosity, why didn't you take that approach? AIUI, hashes are on some of the distribution types but not all of them. I was thinking we would make that a bit more consistent.
There was a problem hiding this comment.
They're not quite the same. The hash on the registry distribution is just the hash reported by the registry, not the hash we compute locally. And the distribution exists well before we compute its hashes.
There was a problem hiding this comment.
We don't compute hashes for registry distributions during resolution. We only validate them during installation.
charliermarsh
force-pushed
the
charlie/package-resolve
branch
3 times, most recently
from
0458e62 to
780dbc3
Compare
|
👍 I'm gonna defer to @BurntSushi on the names since he has the bigger picture in his head w/r/t the other structs in |
charliermarsh
force-pushed
the
charlie/package-resolve
branch
from
780dbc3 to
52fcf94
Compare
There was a problem hiding this comment.
I'm okay with this coming in as-is. I don't think I see anything wrong, although I don't love the code structure/organization. I elaborated a bit in the comments. It is possible that I might have some gaps in my understanding though, so I could have misguided opinions here.
| } | ||
| } | ||
|
|
||
| fn from_source_dist( | ||
| source_dist: &distribution_types::SourceDist, | ||
| hashes: &[HashDigest], |
There was a problem hiding this comment.
Yeah hmmm, I don't like it either.
I think when I was writing this, I was assuming the hashes would be added to types like PathSourceDist directly. Out of curiosity, why didn't you take that approach? AIUI, hashes are on some of the distribution types but not all of them. I was thinking we would make that a bit more consistent.
| .or(hashes.first()) | ||
| .cloned() | ||
| .map(Hash::from); | ||
| Ok(SourceDist { url, hash }) |
There was a problem hiding this comment.
Is there a difference between the hash on the annotated dist versus the hash that's already on the RegistrySourceDist?
There was a problem hiding this comment.
In this case I think we should just ignore the hashes from the annotated dist here actually.
The hash on the RegistrySourceDist is the hash reported by the registry for that specific distribution (file). The hashes on the annotated dist will be the collection of all hashes for all distributions in the registry.
| "Every package should have metadata: {:?}", | ||
| dist.version_id() | ||
| ) | ||
| }; |
There was a problem hiding this comment.
I don't love these panics. (I think some like this were already in this code though.) I think this circles back to my idea of putting this data on the distribution types. Maybe that would lead to increased memory usage though? Not sure.
There was a problem hiding this comment.
We can maybe put it on ResolvedDist? I'd have to look, but it'd be responsible to do so separately IMO since this pattern already exists here.
There was a problem hiding this comment.
Hmm, no, because we need to be able to go from compatible dist to resolved dist... At some point we need to merge the metadata with the distribution, and I guess that's here (for now, at least).
| "Every package should have metadata: {:?}", | ||
| dist.version_id() | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Not necessarily asking for any changes here, but just a "shout out loud": I feel like I've seen this pattern a lot where the code for processing distributions repeats itself. This looks almost identical to the metadata extraction above for example. (I don't yet have opinions on how to resolve this or even if it's worth resolving.)
There was a problem hiding this comment.
Yeah it's nearly identical. I'm gonna look at DRYing it up.
There was a problem hiding this comment.
Sorry, to be clear: it's exactly the same. They're different arms in a match.
crates/uv-resolver/src/lock.rs
Outdated
| .first() | ||
| .or(hashes.first()) | ||
| .cloned() | ||
| .map(Hash::from); |
There was a problem hiding this comment.
So... IIUC, hashes can be two different things. If the distribution comes from a registry, it's the list of hashes served by the registry. If the distribution comes from elsewhere, it's the hashes we computed.
charliermarsh
force-pushed
the
charlie/package-resolve
branch
from
52fcf94 to
4ff08ea
Compare
charliermarsh
force-pushed
the
charlie/package-resolve
branch
from
4ff08ea to
0f72845
Compare
Summary
In
ResolutionGraph::from_state, we have mechanisms to grab the hashes and metadata for all distributions -- but we then throw that information away. This PR preserves it on a newAnnotatedDist(yikes, open to suggestions) that wrapsResolvedDistand includes (1) the hashes (computed or from the registry) and (2) theMetadata23, which lets us extract the version.Closes #3356.
Closes #3357.