From 22afa8fb4dd7e459faf531dd8a9c3580d02c7e2a Mon Sep 17 00:00:00 2001
From: regnat <rg@regnat.ovh>
Date: Thu, 10 Sep 2020 10:55:51 +0200
Subject: [PATCH] Separate store configs from the implems

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
---
 src/libstore/binary-cache-store.cc       |  3 +-
 src/libstore/binary-cache-store.hh       |  8 +++-
 src/libstore/dummy-store.cc              |  4 +-
 src/libstore/http-binary-cache-store.cc  | 16 ++++----
 src/libstore/legacy-ssh-store.cc         | 17 ++++----
 src/libstore/local-binary-cache-store.cc | 18 ++++-----
 src/libstore/local-store.hh              |  2 +-
 src/libstore/remote-store.cc             | 10 ++---
 src/libstore/remote-store.hh             | 34 ++++++++++++----
 src/libstore/s3-binary-cache-store.cc    | 14 ++++---
 src/libstore/ssh-store.cc                | 25 ++++++------
 src/libstore/store-api.cc                |  2 +-
 src/libstore/store-api.hh                | 50 ++++++++++++++----------
 13 files changed, 120 insertions(+), 83 deletions(-)

diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc
index 5433fe50d..34f844a18 100644
--- a/src/libstore/binary-cache-store.cc
+++ b/src/libstore/binary-cache-store.cc
@@ -22,7 +22,8 @@
 namespace nix {
 
 BinaryCacheStore::BinaryCacheStore(const Params & params)
-    : Store(params)
+    : BinaryCacheStoreConfig(params)
+    , Store(params)
 {
     if (secretKeyFile != "")
         secretKey = std::unique_ptr<SecretKey>(new SecretKey(readFile(secretKeyFile)));
diff --git a/src/libstore/binary-cache-store.hh b/src/libstore/binary-cache-store.hh
index 881398ea2..f1fc82013 100644
--- a/src/libstore/binary-cache-store.hh
+++ b/src/libstore/binary-cache-store.hh
@@ -11,9 +11,9 @@ namespace nix {
 
 struct NarInfo;
 
-class BinaryCacheStore : public Store
+struct BinaryCacheStoreConfig : virtual StoreConfig
 {
-public:
+    using StoreConfig::StoreConfig;
 
     const Setting<std::string> compression{this, "xz", "compression", "NAR compression method ('xz', 'bzip2', or 'none')"};
     const Setting<bool> writeNARListing{this, false, "write-nar-listing", "whether to write a JSON file listing the files in each NAR"};
@@ -22,6 +22,10 @@ public:
     const Setting<Path> localNarCache{this, "", "local-nar-cache", "path to a local cache of NARs"};
     const Setting<bool> parallelCompression{this, false, "parallel-compression",
         "enable multi-threading compression, available for xz only currently"};
+};
+
+class BinaryCacheStore : public Store, public virtual BinaryCacheStoreConfig
+{
 
 private:
 
diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc
index 52086445e..d4560c13d 100644
--- a/src/libstore/dummy-store.cc
+++ b/src/libstore/dummy-store.cc
@@ -2,6 +2,8 @@
 
 namespace nix {
 
+struct DummyStoreConfig : StoreConfig {};
+
 struct DummyStore : public Store
 {
     DummyStore(const std::string uri, const Params & params)
@@ -54,6 +56,6 @@ struct DummyStore : public Store
     { unsupported("buildDerivation"); }
 };
 
-static RegisterStoreImplementation<DummyStore> regStore;
+static RegisterStoreImplementation<DummyStore, DummyStoreConfig> regStore;
 
 }
diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc
index e76bac6e2..c3fb455fa 100644
--- a/src/libstore/http-binary-cache-store.cc
+++ b/src/libstore/http-binary-cache-store.cc
@@ -7,7 +7,12 @@ namespace nix {
 
 MakeError(UploadToHTTP, Error);
 
-class HttpBinaryCacheStore : public BinaryCacheStore
+struct HttpBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
+{
+    using BinaryCacheStoreConfig::BinaryCacheStoreConfig;
+};
+
+class HttpBinaryCacheStore : public BinaryCacheStore, public HttpBinaryCacheStoreConfig
 {
 private:
 
@@ -23,16 +28,11 @@ private:
 
 public:
 
-    HttpBinaryCacheStore(
-        const Params & params)
-        : HttpBinaryCacheStore("dummy", params)
-    {
-    }
-
     HttpBinaryCacheStore(
         const Path & _cacheUri,
         const Params & params)
         : BinaryCacheStore(params)
+        , HttpBinaryCacheStoreConfig(params)
         , cacheUri(_cacheUri)
     {
         if (cacheUri.back() == '/')
@@ -176,6 +176,6 @@ protected:
 
 };
 
-static RegisterStoreImplementation<HttpBinaryCacheStore> regStore;
+static RegisterStoreImplementation<HttpBinaryCacheStore, HttpBinaryCacheStoreConfig> regStore;
 
 }
diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc
index 777ba7520..5a08fbba3 100644
--- a/src/libstore/legacy-ssh-store.cc
+++ b/src/libstore/legacy-ssh-store.cc
@@ -9,15 +9,21 @@
 
 namespace nix {
 
-struct LegacySSHStore : public Store
+struct LegacySSHStoreConfig : virtual StoreConfig
 {
+    using StoreConfig::StoreConfig;
     const Setting<int> maxConnections{this, 1, "max-connections", "maximum number of concurrent SSH connections"};
     const Setting<Path> sshKey{this, "", "ssh-key", "path to an SSH private key"};
     const Setting<bool> compress{this, false, "compress", "whether to compress the connection"};
     const Setting<Path> remoteProgram{this, "nix-store", "remote-program", "path to the nix-store executable on the remote system"};
     const Setting<std::string> remoteStore{this, "", "remote-store", "URI of the store on the remote system"};
+};
 
+struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig
+{
     // Hack for getting remote build log output.
+    // Intentionally not in `StoreConfig` so that it doesn't appear in the
+    // documentation
     const Setting<int> logFD{this, -1, "log-fd", "file descriptor to which SSH's stderr is connected"};
 
     struct Connection
@@ -37,12 +43,9 @@ struct LegacySSHStore : public Store
 
     static std::vector<std::string> uriPrefixes() { return {"ssh"}; }
 
-    LegacySSHStore(const Params & params)
-        : LegacySSHStore("dummy", params)
-    {}
-
     LegacySSHStore(const string & host, const Params & params)
-        : Store(params)
+        : LegacySSHStoreConfig(params)
+        , Store(params)
         , host(host)
         , connections(make_ref<Pool<Connection>>(
             std::max(1, (int) maxConnections),
@@ -329,6 +332,6 @@ public:
     }
 };
 
-static RegisterStoreImplementation<LegacySSHStore> regStore;
+static RegisterStoreImplementation<LegacySSHStore, LegacySSHStoreConfig> regStore;
 
 }
diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc
index bcd23eb82..fc3ae94c2 100644
--- a/src/libstore/local-binary-cache-store.cc
+++ b/src/libstore/local-binary-cache-store.cc
@@ -4,7 +4,12 @@
 
 namespace nix {
 
-class LocalBinaryCacheStore : public BinaryCacheStore
+struct LocalBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
+{
+    using BinaryCacheStoreConfig::BinaryCacheStoreConfig;
+};
+
+class LocalBinaryCacheStore : public BinaryCacheStore, public virtual LocalBinaryCacheStoreConfig
 {
 private:
 
@@ -12,16 +17,11 @@ private:
 
 public:
 
-    LocalBinaryCacheStore(
-        const Params & params)
-        : LocalBinaryCacheStore("dummy", params)
-    {
-    }
-
     LocalBinaryCacheStore(
         const Path & binaryCacheDir,
         const Params & params)
-        : BinaryCacheStore(params)
+        : LocalBinaryCacheStoreConfig(params)
+        , BinaryCacheStore(params)
         , binaryCacheDir(binaryCacheDir)
     {
     }
@@ -102,6 +102,6 @@ std::vector<std::string> LocalBinaryCacheStore::uriPrefixes()
         return {"file"};
 }
 
-static RegisterStoreImplementation<LocalBinaryCacheStore> regStore;
+static RegisterStoreImplementation<LocalBinaryCacheStore, LocalBinaryCacheStoreConfig> regStore;
 
 }
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index d5e6d68ef..8b6972847 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -95,7 +95,7 @@ public:
 
 private:
 
-    Setting<bool> requireSigs{(Store*) this,
+    Setting<bool> requireSigs{(StoreConfig*) this,
         settings.requireSigs,
         "require-sigs", "whether store paths should have a trusted signature on import"};
 
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 5f455a62a..619e9a7f1 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -94,6 +94,7 @@ void write(const Store & store, Sink & out, const std::optional<StorePath> & sto
 /* TODO: Separate these store impls into different files, give them better names */
 RemoteStore::RemoteStore(const Params & params)
     : Store(params)
+    , RemoteStoreConfig(params)
     , connections(make_ref<Pool<Connection>>(
             std::max(1, (int) maxConnections),
             [this]() { return openConnectionWrapper(); },
@@ -124,6 +125,7 @@ ref<RemoteStore::Connection> RemoteStore::openConnectionWrapper()
 
 UDSRemoteStore::UDSRemoteStore(const Params & params)
     : Store(params)
+    , UDSRemoteStoreConfig(params)
     , LocalFSStore(params)
     , RemoteStore(params)
 {
@@ -131,11 +133,9 @@ UDSRemoteStore::UDSRemoteStore(const Params & params)
 
 
 UDSRemoteStore::UDSRemoteStore(std::string socket_path, const Params & params)
-    : Store(params)
-    , LocalFSStore(params)
-    , RemoteStore(params)
-    , path(socket_path)
+    : UDSRemoteStore(params)
 {
+    path.emplace(socket_path);
 }
 
 
@@ -982,6 +982,6 @@ std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source *
     return nullptr;
 }
 
-static RegisterStoreImplementation<UDSRemoteStore> regStore;
+static RegisterStoreImplementation<UDSRemoteStore, UDSRemoteStoreConfig> regStore;
 
 }
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index 07cd5daf8..ba2f69427 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -16,19 +16,23 @@ struct FdSource;
 template<typename T> class Pool;
 struct ConnectionHandle;
 
+struct RemoteStoreConfig : virtual StoreConfig
+{
+    using StoreConfig::StoreConfig;
+
+    const Setting<int> maxConnections{(StoreConfig*) this, 1,
+            "max-connections", "maximum number of concurrent connections to the Nix daemon"};
+
+    const Setting<unsigned int> maxConnectionAge{(StoreConfig*) this, std::numeric_limits<unsigned int>::max(),
+            "max-connection-age", "number of seconds to reuse a connection"};
+};
 
 /* FIXME: RemoteStore is a misnomer - should be something like
    DaemonStore. */
-class RemoteStore : public virtual Store
+class RemoteStore : public virtual Store, public virtual RemoteStoreConfig
 {
 public:
 
-    const Setting<int> maxConnections{(Store*) this, 1,
-            "max-connections", "maximum number of concurrent connections to the Nix daemon"};
-
-    const Setting<unsigned int> maxConnectionAge{(Store*) this, std::numeric_limits<unsigned int>::max(),
-            "max-connection-age", "number of seconds to reuse a connection"};
-
     virtual bool sameMachine() = 0;
 
     RemoteStore(const Params & params);
@@ -141,7 +145,21 @@ private:
 
 };
 
-class UDSRemoteStore : public LocalFSStore, public RemoteStore
+struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreConfig
+{
+    UDSRemoteStoreConfig(const Store::Params & params)
+        : LocalFSStoreConfig(params)
+        , RemoteStoreConfig(params)
+    {
+    }
+
+    UDSRemoteStoreConfig()
+        : UDSRemoteStoreConfig(Store::Params({}))
+    {
+    }
+};
+
+class UDSRemoteStore : public LocalFSStore, public RemoteStore, public virtual UDSRemoteStoreConfig
 {
 public:
 
diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc
index ab27f203f..b75d05555 100644
--- a/src/libstore/s3-binary-cache-store.cc
+++ b/src/libstore/s3-binary-cache-store.cc
@@ -172,8 +172,9 @@ S3Helper::FileTransferResult S3Helper::getObject(
     return res;
 }
 
-struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore
+struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
 {
+    using BinaryCacheStoreConfig::BinaryCacheStoreConfig;
     const Setting<std::string> profile{this, "", "profile", "The name of the AWS configuration profile to use."};
     const Setting<std::string> region{this, Aws::Region::US_EAST_1, "region", {"aws-region"}};
     const Setting<std::string> scheme{this, "", "scheme", "The scheme to use for S3 requests, https by default."};
@@ -185,20 +186,21 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore
         this, false, "multipart-upload", "whether to use multi-part uploads"};
     const Setting<uint64_t> bufferSize{
         this, 5 * 1024 * 1024, "buffer-size", "size (in bytes) of each part in multi-part uploads"};
+};
 
+struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCacheStoreConfig
+{
     std::string bucketName;
 
     Stats stats;
 
     S3Helper s3Helper;
 
-    S3BinaryCacheStoreImpl(const Params & params)
-        : S3BinaryCacheStoreImpl("dummy-bucket", params) {}
-
     S3BinaryCacheStoreImpl(
         const std::string & bucketName,
         const Params & params)
-        : S3BinaryCacheStore(params)
+        : S3BinaryCacheStoreConfig(params)
+        , S3BinaryCacheStore(params)
         , bucketName(bucketName)
         , s3Helper(profile, region, scheme, endpoint)
     {
@@ -434,7 +436,7 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore
 
 };
 
-static RegisterStoreImplementation<S3BinaryCacheStoreImpl> regStore;
+static RegisterStoreImplementation<S3BinaryCacheStoreImpl, S3BinaryCacheStoreConfig> regStore;
 
 }
 
diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc
index 85ddce8be..2b6e2a298 100644
--- a/src/libstore/ssh-store.cc
+++ b/src/libstore/ssh-store.cc
@@ -8,23 +8,22 @@
 
 namespace nix {
 
-class SSHStore : public RemoteStore
+struct SSHStoreConfig : virtual RemoteStoreConfig
+{
+    using RemoteStoreConfig::RemoteStoreConfig;
+    const Setting<Path> sshKey{(StoreConfig*) this, "", "ssh-key", "path to an SSH private key"};
+    const Setting<bool> compress{(StoreConfig*) this, false, "compress", "whether to compress the connection"};
+    const Setting<Path> remoteProgram{(StoreConfig*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"};
+    const Setting<std::string> remoteStore{(StoreConfig*) this, "", "remote-store", "URI of the store on the remote system"};
+};
+
+class SSHStore : public RemoteStore, public virtual SSHStoreConfig
 {
 public:
 
-    const Setting<Path> sshKey{(Store*) this, "", "ssh-key", "path to an SSH private key"};
-    const Setting<bool> compress{(Store*) this, false, "compress", "whether to compress the connection"};
-    const Setting<Path> remoteProgram{(Store*) this, "nix-daemon", "remote-program", "path to the nix-daemon executable on the remote system"};
-    const Setting<std::string> remoteStore{(Store*) this, "", "remote-store", "URI of the store on the remote system"};
-
-    SSHStore(
-        const Params & params)
-        : SSHStore("dummy", params)
-    {
-    }
-
     SSHStore(const std::string & host, const Params & params)
         : Store(params)
+        , RemoteStoreConfig(params)
         , RemoteStore(params)
         , host(host)
         , master(
@@ -82,6 +81,6 @@ ref<RemoteStore::Connection> SSHStore::openConnection()
     return conn;
 }
 
-static RegisterStoreImplementation<SSHStore> regStore;
+static RegisterStoreImplementation<SSHStore, SSHStoreConfig> regStore;
 
 }
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index 831d4f30f..fe3f5221c 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -346,7 +346,7 @@ ValidPathInfo Store::addToStoreSlow(std::string_view name, const Path & srcPath,
 
 
 Store::Store(const Params & params)
-    : Config(params)
+    : StoreConfig(params)
     , state({(size_t) pathInfoCacheSize})
 {
 }
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 6e081da41..61552f74e 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -145,12 +145,9 @@ struct BuildResult
     }
 };
 
-
-class Store : public std::enable_shared_from_this<Store>, public Config
+struct StoreConfig : public Config
 {
-public:
-
-    typedef std::map<std::string, std::string> Params;
+    using Config::Config;
 
     const PathSetting storeDir_{this, false, settings.nixStore,
         "store", "path to the Nix store"};
@@ -168,6 +165,14 @@ public:
         "system-features",
         "Optional features that the system this store builds on implements (like \"kvm\")."};
 
+};
+
+class Store : public std::enable_shared_from_this<Store>, public virtual StoreConfig
+{
+public:
+
+    typedef std::map<std::string, std::string> Params;
+
 protected:
 
     struct PathInfoCacheValue {
@@ -632,22 +637,25 @@ protected:
 
 };
 
-
-class LocalFSStore : public virtual Store
+struct LocalFSStoreConfig : virtual StoreConfig
 {
-public:
-
-    // FIXME: the (Store*) cast works around a bug in gcc that causes
+    using StoreConfig::StoreConfig;
+    // FIXME: the (StoreConfig*) cast works around a bug in gcc that causes
     // it to omit the call to the Setting constructor. Clang works fine
     // either way.
-    const PathSetting rootDir{(Store*) this, true, "",
+    const PathSetting rootDir{(StoreConfig*) this, true, "",
         "root", "directory prefixed to all other paths"};
-    const PathSetting stateDir{(Store*) this, false,
+    const PathSetting stateDir{(StoreConfig*) this, false,
         rootDir != "" ? rootDir + "/nix/var/nix" : settings.nixStateDir,
         "state", "directory where Nix will store state"};
-    const PathSetting logDir{(Store*) this, false,
+    const PathSetting logDir{(StoreConfig*) this, false,
         rootDir != "" ? rootDir + "/nix/var/log/nix" : settings.nixLogDir,
         "log", "directory where Nix will store state"};
+};
+
+class LocalFSStore : public virtual Store, public LocalFSStoreConfig
+{
+public:
 
     const static string drvsLogDir;
 
@@ -753,13 +761,13 @@ std::list<ref<Store>> getDefaultSubstituters();
 struct StoreFactory
 {
     std::function<std::shared_ptr<Store> (const std::string & uri, const Store::Params & params)> create;
-    std::function<std::shared_ptr<Store> (const Store::Params & params)> createDummy;
+    std::function<std::shared_ptr<StoreConfig> ()> getConfig;
 };
 struct Implementations
 {
     static std::vector<StoreFactory> * registered;
 
-    template<typename T>
+    template<typename T, typename TConfig>
     static void add()
     {
         if (!registered) registered = new std::vector<StoreFactory>();
@@ -768,21 +776,21 @@ struct Implementations
                 ([](const std::string & uri, const Store::Params & params)
                  -> std::shared_ptr<Store>
                  { return std::make_shared<T>(uri, params); }),
-            .createDummy =
-                ([](const Store::Params & params)
-                 -> std::shared_ptr<Store>
-                 { return std::make_shared<T>(params); })
+            .getConfig =
+                ([]()
+                 -> std::shared_ptr<StoreConfig>
+                 { return std::make_shared<TConfig>(); })
         };
         registered->push_back(factory);
     }
 };
 
-template<typename T>
+template<typename T, typename TConfig>
 struct RegisterStoreImplementation
 {
     RegisterStoreImplementation()
     {
-        Implementations::add<T>();
+        Implementations::add<T, TConfig>();
     }
 };