Closed awelc closed 1 year ago
DependencyGraph
is the wrong place to store the set of fetched dependencies.
Why is it so bad? It's available in all places where we need to access the cache and, conceptually, it deals with dependencies so keeping this cache there does not seem that bad. It will indeed be kept until the dependency graph itself is discarded but at the same time we don't have to worry about explicitly managing its lifetime.
- DependencyGraph is the wrong place to store the set of fetched dependencies.
Why is it so bad?
It mostly boils down to separation of concerns, but that manifests in a couple of ways:
DependencyGraph
is the in-memory representation of a lock file -- so there is a 1:1 correspondence between those two things, which breaks with the addition of fetched_deps
. Not a big deal, but then every special case we add requires explanation, and their existence invites bugs from missing correspondences.ResolutionGraph
also needs to be able to fetch dependencies, so it seems strange to encapsulate the logic for that within one of its immutable inputs (Something that I think you noticed and worked around by cloning the fetched dependencies).Clearly you have it thought over quite a bit more than I do :-) I separated the cache from the dependency graph but left the writer as argument to download_and_update_if_remote
as it is also used in some random places during the resolution process so could not be isolated inside DependencyCache
anyway.
Motivation
The goal of this PR is to resolve https://github.com/MystenLabs/sui/issues/8346 - it caches information about packages fetched during a given dependency graph construction to avoid unnecessarily calling into Git and, at least in some cases, avoid re-fetching the same dependency.
The assumption here is that custom package hooks are registered once before the dependency graph is constructed which ( I hope) seems reasonable, but if not, we can always exclude custom deps from this type of caching.
Have you read the Contributing Guidelines on pull requests?
Yes