WIP: Add Store API-based HashedMirrors #3673
Conversation
This is needed to get content addressed data without the Nix store path. Unfortunately this is not always available to us, so we need to make it optional with a default (ca = "").
This is a replacement for “hashed-mirrors” which provide a way to supply fixed output, flat store objects. An advantage of hashed-mirrors over other substituters is that we can use them in multiple store directories. HashedMirrorStore succeeds hashed mirror, but now uses the store api. url format for HashedMirrorStore looks like: - hashed-mirror+file:// - hashed-mirror+http:// The hashed-mirrors option still works, it just rewrites them as substituters for the above. Currently only file:// is implemented, http:// still needs to be added.
HashedMirrorStore can be trusted on different store dirs, references are disallowed and the hash used does not include a store path.
| @@ -263,15 +263,15 @@ void BinaryCacheStore::addToStore(const ValidPathInfo & info, Source & narSource | |||
| stats.narInfoWrite++; | |||
| } | |||
|
|
|||
| bool BinaryCacheStore::isValidPathUncached(const StorePath & storePath) | |||
| bool BinaryCacheStore::isValidPathUncached(const StorePath & storePath, const std::string ca) | |||
There was a problem hiding this comment.
nit for later: but std::string_view would be better
There was a problem hiding this comment.
Yes, const std::string isn't very useful. Maybe you intended const std::string &.
There was a problem hiding this comment.
actually i just meant the callee shouldn't modify ca - const std::string & would let you modify it. but it's confusing right next to const StorePath &.
| BuildMode buildMode = bmNormal) override | ||
| { unsupported("buildDerivation"); } | ||
|
|
||
| StorePathSet queryValidPaths(const StorePathSet & paths, SubstituteFlag maybeSubstitute, std::map<std::string, std::string> pathsInfo) override |
There was a problem hiding this comment.
We can use StorePath keys if #3450 is merged
|
It's not entirely clear to me why we need a separate |
|
Current BinaryCacheStore requires that .narinfo's exist for each valid path. That hasn't been the case with hashed-mirrors, and I think it would break existing hashed-mirror workflows. |
|
I would just convert hashed mirrors into binary caches by copying the tarballs to a binary cache, and then get rid of the hashed mirror support in |
|
In fact |
|
@edolstra So you mean we can keep the extra CA arguments, and use them to look up data in any store regardless of it's prefix, by using the CA argument to compute the path for that store's prefix on the fly? |
| @@ -445,7 +448,7 @@ public: | |||
| sizes) of a set of paths. If a path does not have substitute | |||
| info, it's omitted from the resulting ‘infos’ map. */ | |||
| virtual void querySubstitutablePathInfos(const StorePathSet & paths, | |||
| SubstitutablePathInfos & infos) { return; }; | |||
| SubstitutablePathInfos & infos, std::map<std::string, Derivation> drvs = {}) { return; }; | |||
There was a problem hiding this comment.
Ought to be std::map<StorePath, std::string /* std::optional<ContentAddress> */>>, and I think that can replace the paths argument.
In particular, tthe set is extended into a map where the set items are now map keys, and the map values are the optional content addresses associated with each store path.
There was a problem hiding this comment.
For back-compat, we can overload the method, and turn the set into a map with all empty strings.
|
closing - and will open a PR with a slightly different approach |
|
opened #3689 as an alternative |
This is a replacement for “hashed-mirrors” which provide a way to
supply fixed output, flat store objects. An advantage of
hashed-mirrors over other substituters is that we can use them in
multiple store directories. HashedMirrorStore succeeds hashed mirror,
but now uses the store api.
url format for HashedMirrorStore looks like:
The hashed-mirrors option still works, it just rewrites them as
substituters for the above.
Currently only file:// is implemented, http:// still needs to be
added.
@edolstra Does this make sense to you? Specifically, I'm wondering if adding
cahere breaks any assumptions we have in the store api. Leaving this partially unfinished (no http support) to wait for some feedback.