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
Document store options #4011
Document store options #4011
Conversation
36cd4c1
to
2148560
Compare
I like this a lot! But should the vertical arrows in the diagram point in the other direction? I think https://github.com/NixOS/nix/pull/3829/files might also help ever so slightly with this. As it removes some stuff which awkwardly overlapped with the store registration mechanism. |
Doesn't this increase compile times a lot? |
My intuition (based on the fact that Nix itself was building fine without that change) is that every .cc file that includes |
Nah I don't think so.
#3829 seems mostly orthogonal, or did I miss something? |
Dunno how reliable it is, but the CI times between this branch and master are sensibly the same. I also tried to |
Maybe because |
It is mostly orthogonal. But the So your PR effectively restores the functionality I removed, while my PR removes the inferior alternate way of doing things. Together, they mean the API is strictly better :) --- you can figure out what store it is without duplicate code or doing effects. |
The
|
e9300a0
to
6cd57b1
Compare
Directly register the store classes rather than a function to build an instance of them. This gives the possibility to introspect static members of the class or choose different ways of instantiating them.
Add a new `init()` method to the `Store` class that is supposed to handle all the effectful initialisation needed to set-up the store. The constructor should remain side-effect free and just initialize the c++ data structure. The goal behind that is that we can create “dummy” instances of each store to query static properties about it (the parameters it accepts for example)
Don't let it just contain the value, but also the other fields of the setting (description, aliases, etc..)
The default value is initialized when creating the setting and unchanged after that
Rework the `Store` hierarchy so that there's now one hierarchy for the store configs and one for the implementations (where each implementation extends the corresponding config). So a class hierarchy like ``` StoreConfig-------->Store | | v v SubStoreConfig----->SubStore | | v v SubSubStoreConfig-->SubSubStore ``` (with virtual inheritance to prevent DDD). The advantage of this architecture is that we can now introspect the configuration of a store without having to instantiate the store itself
Using the `*Config` class hierarchy
Using virtual inheritance means that only the default constructors of the parent classes will be called, which isn't what we want
When opening a store, only try the stores whose `uriSchemes()` include the current one
Allow `-` and `.` in the RFC schemes as stated by [RFC3986](https://tools.ietf.org/html/rfc3986#section-3.1). Practically, this is needed so that `ssh-ng` is a valid URI scheme
So that it can be printed by `nix describe-stores`
Work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80431 that was already there in the code but was accidentally removed in the last commits
It is apparently required for using `toJSONObject()`, which we do inside the header file (because it's in a template). This was accidentally working when building Nix itself (presumably because `config.hh` was always included after `nlohman/json.hpp`) but caused a (pretty dirty) build failure in the perl bindings package.
Instead make a separate header with the template implementation of `BaseSetting<T>::toJSONObj` that can be included where needed
Add some necessary casts in the initialisation of the store's config
Doesn't test much, but at least ensures that the command runs properly
That was it indeed! I've moved the implementation of
Ekk. Fixed, I had forgotten to restore some casts in the s3 store (assumed that all the stores were somehow tested in |
c6112b2
to
c29624b
Compare
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Rebased on top of master to fix a small conflict |
@regnat how come we have |
There might be some not-so-sensible constructors here and there (I introduced a couple of these at some point and might have forgotten to clean them all up), but the interface expected by std::function<std::shared_ptr<Store> (const std::string & scheme, const std::string & uri, const Store::Params & params)> create; so we always need to provide the full url when opening a store |
OK thanks for the feedback. For the record, cde9c15 was my attempt. I might see if I can remove some constructors from |
Ah, I just forgot to upcast |
Fix #3963
This turned out to be much more convoluted than what I though, because the obvious solution (having a loop that instantiate one instance of each store to grab the available options) isn't really feasible − instantiating some stores requires doing some actual effectful (and potentially slow) things and it's hard to work around it. So instead I splitted the store hierarchy into two parallel class hierarchies, something like
(where
A --> B
means that A is a subclass of B).This complexifies things quite a bit (esp. because the diamond inheritance crazyness means that we have to use virtual inheritance everywhere) but allows to instantiate the config for a store independently of the store itself, allowing for a side-effect free and fast
nix describe-stores
command (being fast is I think really important as it's intended to run as part of the build)