Document store options #4011

thufschmitt · 2020-09-14T11:25:04Z

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

+-------------------+     +----------------+     +-------+
|    SubSubStore    | --> |    SubStore    | --> | Store |
+-------------------+     +----------------+     +-------+
  |                         |                      |
  |                         |                      |
  v                         v                      v
+-------------------+     +----------------+     +-------------+     +--------+
| SubSubStoreConfig | --> | SubStoreConfig | --> | StoreConfig | --> | Config |
+-------------------+     +----------------+     +-------------+     +--------+

(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)

Ericson2314 · 2020-09-14T16:57:54Z

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.

edolstra · 2020-09-15T11:45:26Z

Include the full nlohmann/json header in config.hh

Doesn't this increase compile times a lot?

thufschmitt · 2020-09-15T12:02:02Z

Include the full nlohmann/json header in config.hh

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 config.hh also includes nlohmann/json. I might be wrong (not sure how to check that) but if that's the case then compile times should remain exactly the same (except for the perl bindings, but I don't think that the increase here is significant)

thufschmitt · 2020-09-15T12:28:50Z

I like this a lot! But should the vertical arrows in the diagram point in the other direction?

Nah I don't think so. Store is a subclass of StoreConfig (and likewise for the others)

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.

#3829 seems mostly orthogonal, or did I miss something?

thufschmitt · 2020-09-15T12:31:12Z

Include the full nlohmann/json header in config.hh

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 config.hh also includes nlohmann/json. I might be wrong (not sure how to check that) but if that's the case then compile times should remain exactly the same (except for the perl bindings, but I don't think that the increase here is significant)

Dunno how reliable it is, but the CI times between this branch and master are sensibly the same. I also tried to nix-build nix before and after
f1730e0 and the duration was significantly the same

edolstra · 2020-09-15T13:00:53Z

My intuition (based on the fact that Nix itself was building fine without that change) is that every .cc file that includes config.hh also includes nlohmann/json.

Maybe because precompiled-headers.h includes it. Try building with make PRECOMPILE_HEADERS=0.

Ericson2314 · 2020-09-15T14:39:04Z

#3829 seems mostly orthogonal, or did I miss something?

It is mostly orthogonal. But the getStoreType it removed was a very incomplete way of also figuring out the sort of store without actually initializing anything. (It was very incomplete because it didn't interact with the store registration mechanism at all.)

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.

src/libstore/store-api.cc

edolstra · 2020-09-16T09:41:46Z

The describe-stores command crashes:

$ nix describe-stores
nix: src/libutil/config.hh:202: virtual nix::AbstractSetting::~AbstractSetting(): Assertion `created == 123' failed.
Aborted (core dumped)

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

thufschmitt · 2020-09-16T11:55:17Z

Maybe because precompiled-headers.h includes it. Try building with make PRECOMPILE_HEADERS=0.

That was it indeed!

I've moved the implementation of BaseSetting<T>::toJSONObj() to a separate header file that's only included where needed so that config.hh can depend only on json_fwd rather than on the full json header.

The describe-stores command crashes:

Ekk. Fixed, I had forgotten to restore some casts in the s3 store (assumed that all the stores were somehow tested in installcheck, but obviously the s3 store is trickier to test…). I've also added a test that runs nix describe-stores to ensure that this won't happen again

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

thufschmitt · 2020-09-16T12:00:38Z

Rebased on top of master to fix a small conflict

Ericson2314 · 2020-09-22T03:06:07Z

@regnat how come we have MyStore(Params & params) and not just MyStore(std::string & scheme, std::string & uri, Params & params) as the default? Say there is no default for the rest of the URL?

thufschmitt · 2020-09-22T07:45:24Z

@regnat how come we have MyStore(Params & params) and not just MyStore(std::string & scheme, std::string & uri, Params & params) as the default? Say there is no default for the rest of the URL?

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 RegisterStoreImplementation is

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

Ericson2314 · 2020-09-23T18:58:20Z

OK thanks for the feedback. For the record, cde9c15 was my attempt. I might see if I can remove some constructors from master to rule that issue out.

Ericson2314 · 2020-09-23T22:18:43Z

Ah, I just forgot to upcast this for the config constructor. Nevermind!

thufschmitt force-pushed the document-store-options branch from 36cd4c1 to 2148560 Compare September 14, 2020 13:31

edolstra reviewed Sep 16, 2020

View reviewed changes

src/libstore/store-api.cc Outdated Show resolved Hide resolved

thufschmitt force-pushed the document-store-options branch from e9300a0 to 6cd57b1 Compare September 16, 2020 11:30

thufschmitt added 19 commits September 16, 2020 13:53

Make the store plugins more introspectable

7d5bdf8

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.

Fix the registration of stores

fa32560

Complete the toJSON instance for Setting<T>

3c525d1

Don't let it just contain the value, but also the other fields of the setting (description, aliases, etc..)

Add a default value for the settings

35042c9

The default value is initialized when creating the setting and unchanged after that

fixup! Separate the instantiation and initialisation of the stores

aa4eac3

Make nix describe-stores functional

dae39f0

Using the `*Config` class hierarchy

fixup! Make the store plugins more introspectable

d184ad1

Correctly call all the parent contructors of the stores

5895184

Using virtual inheritance means that only the default constructors of the parent classes will be called, which isn't what we want

Properly filter the stores according to their declared uriSchemes

7f103dc

When opening a store, only try the stores whose `uriSchemes()` include the current one

fixup! Correctly call all the parent contructors of the stores

1129913

fixup! Make the store plugins more introspectable

29a6323

Make uri schemes grammar more RFC-compliant

d65962d

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

Document the new store hierarchy

f24f088

Add a name to the stores

b73adac

So that it can be printed by `nix describe-stores`

Add a markdown output to nix describe-stores

634cb2a

Fix build issues with gcc

888f7af

fixup! Add a default value for the settings

a1e82ba

thufschmitt added 5 commits September 16, 2020 13:53

Add (StoreConfig*) casts to work around a GCC bug

fc2d31c

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

Don't include nlohmann/json.hpp in config.hh

e0817cb

Instead make a separate header with the template implementation of `BaseSetting<T>::toJSONObj` that can be included where needed

Fix the s3 store

d72927a

Add some necessary casts in the initialisation of the store's config

Add a test for nix describe-stores

c29624b

Doesn't test much, but at least ensures that the command runs properly

thufschmitt force-pushed the document-store-options branch from c6112b2 to c29624b Compare September 16, 2020 11:59

Remove useless exception copy

77a0e2c

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

edolstra merged commit 5080d4e into NixOS:master Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document store options #4011

Document store options #4011

thufschmitt commented Sep 14, 2020

Ericson2314 commented Sep 14, 2020

edolstra commented Sep 15, 2020 •

edited

thufschmitt commented Sep 15, 2020

thufschmitt commented Sep 15, 2020

thufschmitt commented Sep 15, 2020

edolstra commented Sep 15, 2020

Ericson2314 commented Sep 15, 2020

edolstra commented Sep 16, 2020

thufschmitt commented Sep 16, 2020

thufschmitt commented Sep 16, 2020

Ericson2314 commented Sep 22, 2020 •

edited

thufschmitt commented Sep 22, 2020

Ericson2314 commented Sep 23, 2020 •

edited

Ericson2314 commented Sep 23, 2020

Document store options #4011

Document store options #4011

Conversation

thufschmitt commented Sep 14, 2020

Ericson2314 commented Sep 14, 2020

edolstra commented Sep 15, 2020 • edited

thufschmitt commented Sep 15, 2020

thufschmitt commented Sep 15, 2020

thufschmitt commented Sep 15, 2020

edolstra commented Sep 15, 2020

Ericson2314 commented Sep 15, 2020

edolstra commented Sep 16, 2020

thufschmitt commented Sep 16, 2020

thufschmitt commented Sep 16, 2020

Ericson2314 commented Sep 22, 2020 • edited

thufschmitt commented Sep 22, 2020

Ericson2314 commented Sep 23, 2020 • edited

Ericson2314 commented Sep 23, 2020

edolstra commented Sep 15, 2020 •

edited

Ericson2314 commented Sep 22, 2020 •

edited

Ericson2314 commented Sep 23, 2020 •

edited