Merge pull request #4385 from obsidiansystems/store-subclass

Overhaul store subclassing

src/libstore/binary-cache-store.hh

@@ -24,7 +24,7 @@ struct BinaryCacheStoreConfig : virtual StoreConfig
         "enable multi-threading compression, available for xz only currently"};
 };
 
-class BinaryCacheStore : public Store, public virtual BinaryCacheStoreConfig
+class BinaryCacheStore : public virtual BinaryCacheStoreConfig, public virtual Store
 {
 
 private:

src/libstore/build/derivation-goal.cc

@@ -1985,7 +1985,7 @@ void DerivationGoal::writeStructuredAttrs()
     chownToBuilder(tmpDir + "/.attrs.sh");
 }
 
-struct RestrictedStoreConfig : LocalFSStoreConfig
+struct RestrictedStoreConfig : virtual LocalFSStoreConfig
 {
     using LocalFSStoreConfig::LocalFSStoreConfig;
     const std::string name() { return "Restricted Store"; }
@@ -1994,14 +1994,19 @@ struct RestrictedStoreConfig : LocalFSStoreConfig
 /* A wrapper around LocalStore that only allows building/querying of
    paths that are in the input closures of the build or were added via
    recursive Nix calls. */
-struct RestrictedStore : public LocalFSStore, public virtual RestrictedStoreConfig
+struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual LocalFSStore
 {
     ref<LocalStore> next;
 
     DerivationGoal & goal;
 
     RestrictedStore(const Params & params, ref<LocalStore> next, DerivationGoal & goal)
-        : StoreConfig(params), Store(params), LocalFSStore(params), next(next), goal(goal)
+        : StoreConfig(params)
+        , LocalFSStoreConfig(params)
+        , RestrictedStoreConfig(params)
+        , Store(params)
+        , LocalFSStore(params)
+        , next(next), goal(goal)
     { }
 
     Path getRealStoreDir() override

src/libstore/dummy-store.cc

@@ -9,14 +9,15 @@ struct DummyStoreConfig : virtual StoreConfig {
     const std::string name() override { return "Dummy Store"; }
 };
 
-struct DummyStore : public Store, public virtual DummyStoreConfig
+struct DummyStore : public virtual DummyStoreConfig, public virtual Store
 {
     DummyStore(const std::string scheme, const std::string uri, const Params & params)
         : DummyStore(params)
     { }
 
     DummyStore(const Params & params)
         : StoreConfig(params)
+        , DummyStoreConfig(params)
         , Store(params)
     { }
 

src/libstore/http-binary-cache-store.cc

@@ -15,7 +15,7 @@ struct HttpBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
     const std::string name() override { return "Http Binary Cache Store"; }
 };
 
-class HttpBinaryCacheStore : public BinaryCacheStore, public HttpBinaryCacheStoreConfig
+class HttpBinaryCacheStore : public virtual HttpBinaryCacheStoreConfig, public virtual BinaryCacheStore
 {
 private:
 
@@ -36,6 +36,9 @@ class HttpBinaryCacheStore : public BinaryCacheStore, public HttpBinaryCacheStor
         const Path & _cacheUri,
         const Params & params)
         : StoreConfig(params)
+        , BinaryCacheStoreConfig(params)
+        , HttpBinaryCacheStoreConfig(params)
+        , Store(params)
         , BinaryCacheStore(params)
         , cacheUri(scheme + "://" + _cacheUri)
     {

src/libstore/legacy-ssh-store.cc

@@ -22,7 +22,7 @@ struct LegacySSHStoreConfig : virtual StoreConfig
     const std::string name() override { return "Legacy SSH Store"; }
 };
 
-struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig
+struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Store
 {
     // Hack for getting remote build log output.
     // Intentionally not in `LegacySSHStoreConfig` so that it doesn't appear in
@@ -48,6 +48,7 @@ struct LegacySSHStore : public Store, public virtual LegacySSHStoreConfig
 
     LegacySSHStore(const string & scheme, const string & host, const Params & params)
         : StoreConfig(params)
+        , LegacySSHStoreConfig(params)
         , Store(params)
         , host(host)
         , connections(make_ref<Pool<Connection>>(

src/libstore/local-binary-cache-store.cc

@@ -11,7 +11,7 @@ struct LocalBinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
     const std::string name() override { return "Local Binary Cache Store"; }
 };
 
-class LocalBinaryCacheStore : public BinaryCacheStore, public virtual LocalBinaryCacheStoreConfig
+class LocalBinaryCacheStore : public virtual LocalBinaryCacheStoreConfig, public virtual BinaryCacheStore
 {
 private:
 
@@ -24,6 +24,9 @@ class LocalBinaryCacheStore : public BinaryCacheStore, public virtual LocalBinar
         const Path & binaryCacheDir,
         const Params & params)
         : StoreConfig(params)
+        , BinaryCacheStoreConfig(params)
+        , LocalBinaryCacheStoreConfig(params)
+        , Store(params)
         , BinaryCacheStore(params)
         , binaryCacheDir(binaryCacheDir)
     {

src/libstore/local-fs-store.hh

@@ -20,7 +20,7 @@ struct LocalFSStoreConfig : virtual StoreConfig
         "log", "directory where Nix will store state"};
 };
 
-class LocalFSStore : public virtual Store, public virtual LocalFSStoreConfig
+class LocalFSStore : public virtual LocalFSStoreConfig, public virtual Store
 {
 public:
 

src/libstore/local-store.cc

@@ -100,6 +100,8 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd)
 
 LocalStore::LocalStore(const Params & params)
     : StoreConfig(params)
+    , LocalFSStoreConfig(params)
+    , LocalStoreConfig(params)
     , Store(params)
     , LocalFSStore(params)
     , realStoreDir_{this, false, rootDir != "" ? rootDir + "/nix/store" : storeDir, "real",

src/libstore/local-store.hh

@@ -43,7 +43,7 @@ struct LocalStoreConfig : virtual LocalFSStoreConfig
 };
 
 
-class LocalStore : public LocalFSStore, public virtual LocalStoreConfig
+class LocalStore : public virtual LocalStoreConfig, public virtual LocalFSStore
 {
 private:
 

src/libstore/remote-store.cc

@@ -77,8 +77,8 @@ void write(const Store & store, Sink & out, const std::optional<ContentAddress>
 
 /* TODO: Separate these store impls into different files, give them better names */
 RemoteStore::RemoteStore(const Params & params)
-    : Store(params)
-    , RemoteStoreConfig(params)
+    : RemoteStoreConfig(params)
+    , Store(params)
     , connections(make_ref<Pool<Connection>>(
             std::max(1, (int) maxConnections),
             [this]() {

src/libstore/remote-store.hh

@@ -29,7 +29,7 @@ struct RemoteStoreConfig : virtual StoreConfig
 
 /* FIXME: RemoteStore is a misnomer - should be something like
    DaemonStore. */
-class RemoteStore : public virtual Store, public virtual RemoteStoreConfig
+class RemoteStore : public virtual RemoteStoreConfig, public virtual Store
 {
 public:
 

src/libstore/s3-binary-cache-store.cc

@@ -177,6 +177,11 @@ S3Helper::FileTransferResult S3Helper::getObject(
     return res;
 }
 
+S3BinaryCacheStore::S3BinaryCacheStore(const Params & params)
+    : BinaryCacheStoreConfig(params)
+    , BinaryCacheStore(params)
+{ }
+
 struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
 {
     using BinaryCacheStoreConfig::BinaryCacheStoreConfig;
@@ -195,7 +200,7 @@ struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
     const std::string name() override { return "S3 Binary Cache Store"; }
 };
 
-struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCacheStoreConfig
+struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual S3BinaryCacheStore
 {
     std::string bucketName;
 
@@ -208,6 +213,10 @@ struct S3BinaryCacheStoreImpl : public S3BinaryCacheStore, virtual S3BinaryCache
         const std::string & bucketName,
         const Params & params)
         : StoreConfig(params)
+        , BinaryCacheStoreConfig(params)
+        , S3BinaryCacheStoreConfig(params)
+        , Store(params)
+        , BinaryCacheStore(params)
         , S3BinaryCacheStore(params)
         , bucketName(bucketName)
         , s3Helper(profile, region, scheme, endpoint)

src/libstore/s3-binary-cache-store.hh

@@ -6,13 +6,11 @@
 
 namespace nix {
 
-class S3BinaryCacheStore : public BinaryCacheStore
+class S3BinaryCacheStore : public virtual BinaryCacheStore
 {
 protected:
 
-    S3BinaryCacheStore(const Params & params)
-        : BinaryCacheStore(params)
-    { }
+    S3BinaryCacheStore(const Params & params);
 
 public:
 

src/libstore/ssh-store.cc

@@ -20,12 +20,14 @@ struct SSHStoreConfig : virtual RemoteStoreConfig
     const std::string name() override { return "SSH Store"; }
 };
 
-class SSHStore : public virtual RemoteStore, public virtual SSHStoreConfig
+class SSHStore : public virtual SSHStoreConfig, public virtual RemoteStore
 {
 public:
 
     SSHStore(const std::string & scheme, const std::string & host, const Params & params)
         : StoreConfig(params)
+        , RemoteStoreConfig(params)
+        , SSHStoreConfig(params)
         , Store(params)
         , RemoteStore(params)
         , host(host)

src/libstore/store-api.hh

@@ -175,25 +175,7 @@ struct StoreConfig : public Config
 {
     using Config::Config;
 
-    /**
-     * When constructing a store implementation, we pass in a map `params` of
-     * parameters that's supposed to initialize the associated config.
-     * To do that, we must use the `StoreConfig(StringMap & params)`
-     * constructor, so we'd like to `delete` its default constructor to enforce
-     * it.
-     *
-     * However, actually deleting it means that all the subclasses of
-     * `StoreConfig` will have their default constructor deleted (because it's
-     * supposed to call the deleted default constructor of `StoreConfig`). But
-     * because we're always using virtual inheritance, the constructors of
-     * child classes will never implicitely call this one, so deleting it will
-     * be more painful than anything else.
-     *
-     * So we `assert(false)` here to ensure at runtime that the right
-     * constructor is always called without having to redefine a custom
-     * constructor for each `*Config` class.
-     */
-    StoreConfig() { assert(false); }
+    StoreConfig() = delete;
 
     virtual ~StoreConfig() { }
 

src/libstore/uds-remote-store.cc

@@ -15,6 +15,9 @@ namespace nix {
 
 UDSRemoteStore::UDSRemoteStore(const Params & params)
     : StoreConfig(params)
+    , LocalFSStoreConfig(params)
+    , RemoteStoreConfig(params)
+    , UDSRemoteStoreConfig(params)
     , Store(params)
     , LocalFSStore(params)
     , RemoteStore(params)

src/libstore/uds-remote-store.hh

@@ -14,15 +14,10 @@ struct UDSRemoteStoreConfig : virtual LocalFSStoreConfig, virtual RemoteStoreCon
     {
     }
 
-    UDSRemoteStoreConfig()
-        : UDSRemoteStoreConfig(Store::Params({}))
-    {
-    }
-
     const std::string name() override { return "Local Daemon Store"; }
 };
 
-class UDSRemoteStore : public LocalFSStore, public RemoteStore, public virtual UDSRemoteStoreConfig
+class UDSRemoteStore : public virtual UDSRemoteStoreConfig, public virtual LocalFSStore, public virtual RemoteStore
 {
 public: