-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit for later: but std::string_view
would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, const std::string
isn't very useful. Maybe you intended const std::string &
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ca
here breaks any assumptions we have in the store api. Leaving this partially unfinished (no http support) to wait for some feedback.