Skip to content
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

Merged
merged 25 commits into from Sep 16, 2020
Merged

Document store options #4011

merged 25 commits into from Sep 16, 2020

Conversation

@regnat
Copy link
Contributor

regnat commented Sep 14, 2020

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

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

@regnat regnat force-pushed the tweag:document-store-options branch from 36cd4c1 to 2148560 Sep 14, 2020
@Ericson2314
Copy link
Member

Ericson2314 commented Sep 14, 2020

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
Copy link
Member

edolstra commented Sep 15, 2020

Include the full nlohmann/json header in config.hh

Doesn't this increase compile times a lot?

@regnat
Copy link
Contributor Author

regnat commented Sep 15, 2020

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)

@regnat
Copy link
Contributor Author

regnat commented Sep 15, 2020

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?

@regnat
Copy link
Contributor Author

regnat commented Sep 15, 2020

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
Copy link
Member

edolstra commented Sep 15, 2020

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
Copy link
Member

Ericson2314 commented Sep 15, 2020

#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 Outdated Show resolved Hide resolved
@edolstra
Copy link
Member

edolstra commented Sep 16, 2020

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)
@regnat regnat force-pushed the tweag:document-store-options branch from e9300a0 to 6cd57b1 Sep 16, 2020
regnat added 19 commits Sep 8, 2020
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`
regnat added 5 commits Sep 15, 2020
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
@regnat
Copy link
Contributor Author

regnat commented Sep 16, 2020

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

@regnat regnat force-pushed the tweag:document-store-options branch from c6112b2 to c29624b Sep 16, 2020
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
@regnat
Copy link
Contributor Author

regnat commented Sep 16, 2020

Rebased on top of master to fix a small conflict

@edolstra edolstra merged commit 5080d4e into NixOS:master Sep 16, 2020
2 checks passed
2 checks passed
tests (ubuntu-latest)
Details
tests (macos-latest)
Details
@Ericson2314
Copy link
Member

Ericson2314 commented Sep 22, 2020

@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?

@regnat
Copy link
Contributor Author

regnat commented Sep 22, 2020

@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
Copy link
Member

Ericson2314 commented Sep 23, 2020

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
Copy link
Member

Ericson2314 commented Sep 23, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.