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

StoreReference -> <Name>StoreConfig -> <Name>Store #10766

Open
Ericson2314 opened this issue May 23, 2024 · 2 comments
Open

StoreReference -> <Name>StoreConfig -> <Name>Store #10766

Ericson2314 opened this issue May 23, 2024 · 2 comments
Labels
good first issue Quick win for first-time contributors settings Settings, global flags, nix.conf significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. store Issues and pull requests concerning the Nix store

Comments

@Ericson2314
Copy link
Member

Today, when we open a store, we always go directly from a StoreReference to the <name>Store data type directly, creating a (typed, per store) <name>StoreConfig merely in the process. Instead, we should also create the <name>StoreConfig as a separate step, and then create the store from it.

Here's what needs to be done for that to work:

1. Make sure every <name>StoreConfig has a (scheme, authority, params) constructor

Right now, they just have params (single argument) constructors. This is still needed for the docs, but it should not be the only option. There should also be three-argument constructors as described above.

In order for us to actually make use of the other parameters, any fields which store the scheme and authority (if they are side-effect-less!!) should be moved from the corresponding <name>Store to <name>StoreConfig. For example, host should be moved from SSHStore and LegacySSHStore to SSHStoreConfig and LegacySSHStoreConfig. (And actually we can do a dedup by again moving that field to CommonSSHStoreConfig, where both type can share it.)

2. *StoreConfig should be member not base class

Right now, every <Name>Store type has its <Name>StoreConfig as a virtual base class. This won't because Config cannot be copied/moved (due to silly reasons, but let's ignore that) so we cannot initialize a *Store from a pe-eexisting *StoreConfig.

To solve this problem we should switch from inheritance to fields. In particular, every one of these *Store -> *StoreConfig virtual base classes should become a ref<FooConfig> fooConfig;. ref<..> is a crucial choice --- it works with upcasting such that from one RootStoreConfig allocation we can create as many ref<BaseClassStoreConfig> as we need, without creating redundant fresh allocations / more configs that could get out of sync (and waste space, but that's not so bad.)

Do note that its only the *Store -> *StoreConfig edges which should become fields instead. the *StoreConfig -> *StoreConfig and *Store -> *Store edges should stay as virtual inheritance --- for now, at least ;).

3. NameStore::NameStore(ref<NameStoreConfig>) constructors

With the previous steps done, we change the actual store constructors (replacing the old ones) to not take `(scheme, authority, params), but instead take the corresponding config type (by a shared reference). This should be relatively straightforward so long as everything in the previous step was done properly.

4. openStore pure virtual method

We can create a new virtual ref<Store> openStore() = 0; method on StoreConfig itself. This method's implementations will just call the constructors we modified in the last step with shared_from_this on the <Name>StoreConfig (to get the ref<....>) with make_ref<...>(....).

This method does no work on its own (it just calls the constructors) but finishes "proving" that the (concrete, non abstract) <Name>StoreConfig and <Name>Store types are actually 1-1 --- for every store type, the constructor chooses a store config type, and for every store config type, the openStore method chooses a store type, and this is a bijection.

5. Update the store registration machinery

The store registration machinery already registers each pair of store and store config types. But we'll need to fix it to deal with the interface changes from above. Instead of trying to directly construct the store with the matching scheme, it could first construct the store config and then call openStore on that.

Actually, we should go one step further, the registration machinery should just look up the right store config type, and construct a ref<StoreConfig> then the user can construct a ref<Store> from that with our new openStore method. The registration machinery can only know about store config types, and no longer needs to know about store types at all!

@Ericson2314 Ericson2314 added good first issue Quick win for first-time contributors settings Settings, global flags, nix.conf significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. labels May 23, 2024
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue May 23, 2024
By moving `host` to the config, we can do a lot further cleanups and
dedups. This anticipates a world where we always go `StoreReference` ->
`*StoreConfig` -> `Store*` rather than skipping the middle step too.

Progress on NixOS#10766

Progress on NixOS/hydra#1164
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue May 24, 2024
By moving `host` to the config, we can do a lot further cleanups and
dedups. This anticipates a world where we always go `StoreReference` ->
`*StoreConfig` -> `Store*` rather than skipping the middle step too.

Progress on NixOS#10766

Progress on NixOS/hydra#1164
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue May 27, 2024
By moving `host` to the config, we can do a lot further cleanups and
dedups. This anticipates a world where we always go `StoreReference` ->
`*StoreConfig` -> `Store*` rather than skipping the middle step too.

Progress on NixOS#10766

Progress on NixOS/hydra#1164
@roberth
Copy link
Member

roberth commented May 29, 2024

SGTM.
Notes:

0. StoreReference already exists; more or less a URL
1. StoreConfig is a semantically meaningful URL, parsed into store-type-specific fields
2. Composition over inheritance is good. Use of config items will be explicit, which is good.
3-5. Sensible refactors.

@roberth roberth added the store Issues and pull requests concerning the Nix store label May 29, 2024
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
Progres towards NixOS#10766

I thought that NixOS#10768 achieved, but when I went to use this stuff (in
Hydra), turns out it did not. (Those `using FooConfig;` lines were not
working --- they are so finicky!) This PR gets the job done, and adds
some trivial unit tests to make sure I did what I intended.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
Progress towards NixOS#10766

I thought that NixOS#10768 achieved, but when I went to use this stuff (in
Hydra), turns out it did not. (Those `using FooConfig;` lines were not
working --- they are so finicky!) This PR gets the job done, and adds
some trivial unit tests to make sure I did what I intended.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
Progress towards NixOS#10766

I thought that NixOS#10768 achieved, but when I went to use this stuff (in
Hydra), turns out it did not. (Those `using FooConfig;` lines were not
working --- they are so finicky!) This PR gets the job done, and adds
some trivial unit tests to make sure I did what I intended.

I had to add add a header to expose `SSHStoreConfig`, after which the
preexisting `ssh-store-config.*` were very confusingly named files, so I
renamed them to `common-ssh-store-config.hh` to match the type defined
therein.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
Progress towards NixOS#10766

I thought that NixOS#10768 achieved, but when I went to use this stuff (in
Hydra), turns out it did not. (Those `using FooConfig;` lines were not
working --- they are so finicky!) This PR gets the job done, and adds
some trivial unit tests to make sure I did what I intended.

I had to add add a header to expose `SSHStoreConfig`, after which the
preexisting `ssh-store-config.*` were very confusingly named files, so I
renamed them to `common-ssh-store-config.hh` to match the type defined
therein.
fzakaria added a commit to fzakaria/nix that referenced this issue Jul 15, 2024
Following what is outlined in NixOS#10766 refactor the uds-remote-store such
that the member variables (state) don't live in the store itself but in
the config object.

Additionally, the config object includes a new necessary constructor
that takes a scheme & authority.

Minor:
* code formatting
* cleanup of getting default path
* added some comments
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
Progress towards NixOS#10766

I thought that NixOS#10768 achieved, but when I went to use this stuff (in
Hydra), turns out it did not. (Those `using FooConfig;` lines were not
working --- they are so finicky!) This PR gets the job done, and adds
some trivial unit tests to make sure I did what I intended.

I had to add add a header to expose `SSHStoreConfig`, after which the
preexisting `ssh-store-config.*` were very confusingly named files, so I
renamed them to `common-ssh-store-config.hh` to match the type defined
therein.
@Ericson2314
Copy link
Member Author

#11106 somewhat related

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 15, 2024
Progress towards NixOS#10766

I thought that NixOS#10768 achieved, but when I went to use this stuff (in
Hydra), turns out it did not. (Those `using FooConfig;` lines were not
working --- they are so finicky!) This PR gets the job done, and adds
some trivial unit tests to make sure I did what I intended.

I had to add add a header to expose `SSHStoreConfig`, after which the
preexisting `ssh-store-config.*` were very confusingly named files, so I
renamed them to `common-ssh-store-config.hh` to match the type defined
therein.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 16, 2024
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 16, 2024
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 16, 2024
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
Ericson2314 added a commit that referenced this issue Jul 18, 2024
Following what is outlined in #10766 refactor the uds-remote-store such
that the member variables (state) don't live in the store itself but in
the config object.

Additionally, the config object includes a new necessary constructor
that takes a scheme & authority.

Tests are commented out because of linking errors with the current config system.
When there is a new config system we can reenable them.

Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 18, 2024
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Jul 18, 2024
It is a property of the configuration of a store --- how a store URL is
parsed into a store config, not a store itself.

Progress towards NixOS#10766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Quick win for first-time contributors settings Settings, global flags, nix.conf significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. store Issues and pull requests concerning the Nix store
Projects
None yet
Development

No branches or pull requests

2 participants