Ensure all store types support "real" URIs

In particular `local://<path>` and `unix://` (without any path) now work, and mean the same things as `local` and `daemon`, respectively. We thus now have the opportunity to desguar `local` and `daemon` early. This will allow me to make a change to NixOS#9839 requested during review to desugar those earlier.
obsidiansystems · Jan 25, 2024 · cbf479e · cbf479e
1 parent 979b00b
commit cbf479e
Show file tree

Hide file tree

Showing 16 changed files with 101 additions and 51 deletions.
diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc
@@ -18,9 +18,12 @@ struct DummyStoreConfig : virtual StoreConfig {
 
 struct DummyStore : public virtual DummyStoreConfig, public virtual Store
 {
-    DummyStore(const std::string scheme, const std::string uri, const Params & params)
+    DummyStore(std::string_view scheme, std::optional<std::string_view> authority, const Params & params)
         : DummyStore(params)
-    { }
+    {
+        if (authority)
+            warn("`dummy://` store URIs should not contain an authority part. This will become an error in future versions of Nix");
+    }
 
     DummyStore(const Params & params)
         : StoreConfig(params)

diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc
@@ -39,15 +39,18 @@ class HttpBinaryCacheStore : public virtual HttpBinaryCacheStoreConfig, public v
 public:
 
     HttpBinaryCacheStore(
-        const std::string & scheme,
-        const Path & _cacheUri,
+        std::string_view scheme,
+        std::optional<PathView> _cacheUri,
         const Params & params)
         : StoreConfig(params)
         , BinaryCacheStoreConfig(params)
         , HttpBinaryCacheStoreConfig(params)
         , Store(params)
         , BinaryCacheStore(params)
-        , cacheUri(scheme + "://" + _cacheUri)
+        , cacheUri(
+            std::string { scheme }
+            + "://"
+            + _cacheUri.value_or(""))
     {
         if (cacheUri.back() == '/')
             cacheUri.pop_back();

diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc
@@ -29,21 +29,24 @@ struct LegacySSHStore::Connection : public ServeProto::BasicClientConnection
 };
 
 
-LegacySSHStore::LegacySSHStore(const std::string & scheme, const std::string & host, const Params & params)
+LegacySSHStore::LegacySSHStore(
+    std::string_view scheme,
+    std::optional<std::string_view> host,
+    const Params & params)
     : StoreConfig(params)
     , CommonSSHStoreConfig(params)
     , LegacySSHStoreConfig(params)
     , Store(params)
-    , host(host)
+    , host(host.value_or(""))
     , connections(make_ref<Pool<Connection>>(
         std::max(1, (int) maxConnections),
         [this]() { return openConnection(); },
         [](const ref<Connection> & r) { return r->good; }
         ))
     , master(
-        host,
-        sshKey,
-        sshPublicHostKey,
+        host.value_or(""),
+        sshKey.get(),
+        sshPublicHostKey.get(),
         // Use SSH master only if using more than 1 connection.
         connections->capacity() > 1,
         compress,

diff --git a/src/libstore/legacy-ssh-store.hh b/src/libstore/legacy-ssh-store.hh
@@ -41,7 +41,10 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
 
     static std::set<std::string> uriSchemes() { return {"ssh"}; }
 
-    LegacySSHStore(const std::string & scheme, const std::string & host, const Params & params);
+    LegacySSHStore(
+        std::string_view scheme,
+        std::optional<std::string_view> host,
+        const Params & params);
 
     ref<Connection> openConnection();
 

diff --git a/src/libstore/local-binary-cache-store.cc b/src/libstore/local-binary-cache-store.cc
@@ -29,15 +29,15 @@ class LocalBinaryCacheStore : public virtual LocalBinaryCacheStoreConfig, public
 public:
 
     LocalBinaryCacheStore(
-        const std::string scheme,
-        const Path & binaryCacheDir,
+        std::string_view scheme,
+        std::optional<PathView> binaryCacheDir,
         const Params & params)
         : StoreConfig(params)
         , BinaryCacheStoreConfig(params)
         , LocalBinaryCacheStoreConfig(params)
         , Store(params)
         , BinaryCacheStore(params)
-        , binaryCacheDir(binaryCacheDir)
+        , binaryCacheDir(binaryCacheDir.value_or(""))
     {
     }
 

diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
@@ -446,10 +446,20 @@ LocalStore::LocalStore(const Params & params)
 }
 
 
-LocalStore::LocalStore(std::string scheme, std::string path, const Params & params)
-    : LocalStore(params)
+LocalStore::LocalStore(
+    std::string_view scheme,
+    std::optional<PathView> path,
+    const Params & _params)
+    : LocalStore([&]{
+        // ?root=... overrides path
+        if (path && _params.count("root") == 0) {
+            auto params = _params;
+            params.insert_or_assign("root", std::string { *path });
+            return params;
+        }
+        return _params;
+    }())
 {
-    throw UnimplementedError("LocalStore");
 }
 
 

diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
@@ -138,12 +138,15 @@ public:
      * necessary.
      */
     LocalStore(const Params & params);
-    LocalStore(std::string scheme, std::string path, const Params & params);
+    LocalStore(
+        std::string_view scheme,
+        std::optional<PathView> path,
+        const Params & params);
 
     ~LocalStore();
 
     static std::set<std::string> uriSchemes()
-    { return {}; }
+    { return {"local"}; }
 
     /**
      * Implementations of abstract store API methods.

diff --git a/src/libstore/s3-binary-cache-store.cc b/src/libstore/s3-binary-cache-store.cc
@@ -213,7 +213,7 @@ struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
           support it.
 
           > **Note**
-          > 
+          >
           > HTTPS should be used if the cache might contain sensitive
           > information.
         )"};
@@ -224,7 +224,7 @@ struct S3BinaryCacheStoreConfig : virtual BinaryCacheStoreConfig
           Do not specify this setting if you're using Amazon S3.
 
           > **Note**
-          > 
+          >
           > This endpoint must support HTTPS and will use path-based
           > addressing instead of virtual host based addressing.
         )"};
@@ -269,16 +269,16 @@ struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual
     S3Helper s3Helper;
 
     S3BinaryCacheStoreImpl(
-        const std::string & uriScheme,
-        const std::string & bucketName,
+        std::string_view uriScheme,
+        std::optional<std::string_view> bucketName,
         const Params & params)
         : StoreConfig(params)
         , BinaryCacheStoreConfig(params)
         , S3BinaryCacheStoreConfig(params)
         , Store(params)
         , BinaryCacheStore(params)
         , S3BinaryCacheStore(params)
-        , bucketName(bucketName)
+        , bucketName(bucketName.value_or(""))
         , s3Helper(profile, region, scheme, endpoint)
     {
         diskCache = getNarInfoDiskCache();

diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc
@@ -34,18 +34,21 @@ class SSHStore : public virtual SSHStoreConfig, public virtual RemoteStore
 {
 public:
 
-    SSHStore(const std::string & scheme, const std::string & host, const Params & params)
+    SSHStore(
+        std::string_view scheme,
+        std::optional<std::string_view> host,
+        const Params & params)
         : StoreConfig(params)
         , RemoteStoreConfig(params)
         , CommonSSHStoreConfig(params)
         , SSHStoreConfig(params)
         , Store(params)
         , RemoteStore(params)
-        , host(host)
+        , host(host.value_or(""))
         , master(
-            host,
-            sshKey,
-            sshPublicHostKey,
+            host.value_or(""),
+            sshKey.get(),
+            sshPublicHostKey.get(),
             // Use SSH master only if using more than 1 connection.
             connections->capacity() > 1,
             compress)
@@ -141,7 +144,10 @@ class MountedSSHStore : public virtual MountedSSHStoreConfig, public virtual SSH
 {
 public:
 
-    MountedSSHStore(const std::string & scheme, const std::string & host, const Params & params)
+    MountedSSHStore(
+        std::string_view scheme,
+        std::optional<std::string_view> host,
+        const Params & params)
         : StoreConfig(params)
         , RemoteStoreConfig(params)
         , CommonSSHStoreConfig(params)

diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc
@@ -6,7 +6,11 @@
 
 namespace nix {
 
-SSHMaster::SSHMaster(const std::string & host, const std::string & keyFile, const std::string & sshPublicHostKey, bool useMaster, bool compress, int logFD)
+SSHMaster::SSHMaster(
+    std::string_view host,
+    std::string_view keyFile,
+    std::string_view sshPublicHostKey,
+    bool useMaster, bool compress, int logFD)
     : host(host)
     , fakeSSH(host == "localhost")
     , keyFile(keyFile)

diff --git a/src/libstore/ssh.hh b/src/libstore/ssh.hh
@@ -33,7 +33,11 @@ private:
 
 public:
 
-    SSHMaster(const std::string & host, const std::string & keyFile, const std::string & sshPublicHostKey, bool useMaster, bool compress, int logFD = -1);
+    SSHMaster(
+        std::string_view host,
+        std::string_view keyFile,
+        std::string_view sshPublicHostKey,
+        bool useMaster, bool compress, int logFD = -1);
 
     struct Connection
     {

diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
@@ -1378,9 +1378,7 @@ std::shared_ptr<Store> openFromNonUri(const std::string & uri, const Store::Para
                 warn("'%s' does not exist, so Nix will use '%s' as a chroot store", stateDir, chrootStore);
             } else
                 debug("'%s' does not exist, so Nix will use '%s' as a chroot store", stateDir, chrootStore);
-            Store::Params params2;
-            params2["root"] = chrootStore;
-            return std::make_shared<LocalStore>(params2);
+            return std::make_shared<LocalStore>("local", chrootStore, params);
         }
         #endif
         else
@@ -1390,9 +1388,7 @@ std::shared_ptr<Store> openFromNonUri(const std::string & uri, const Store::Para
     } else if (uri == "local") {
         return std::make_shared<LocalStore>(params);
     } else if (isNonUriPath(uri)) {
-        Store::Params params2 = params;
-        params2["root"] = absPath(uri);
-        return std::make_shared<LocalStore>(params2);
+        return std::make_shared<LocalStore>("local", absPath(uri), params);
     } else {
         return nullptr;
     }
@@ -1409,21 +1405,26 @@ std::shared_ptr<Store> openFromNonUri(const std::string & uri, const Store::Para
 //   will be transformed into `root@::1` for SSH (same for `[::1]` -> `::1`).
 // * If the URL looks like `root@::1` it will be left as-is.
 // * In any other case, the string will be left as-is.
-static std::string extractConnStr(const std::string &proto, const std::string &connStr)
+static std::optional<std::string> extractConnStr(
+    const std::string & proto,
+    const std::string & connStr)
 {
+    std::string connStr2 = connStr;
+
     if (proto.rfind("ssh") != std::string::npos) {
         std::smatch result;
         std::regex v6AddrRegex("^((.*)@)?\\[(.*)\\]$");
 
         if (std::regex_match(connStr, result, v6AddrRegex)) {
-            if (result[1].matched) {
-                return result.str(1) + result.str(3);
-            }
-            return result.str(3);
+            connStr2 = result[1].matched
+                ? result.str(1) + result.str(3)
+                : result.str(3);
         }
     }
 
-    return connStr;
+    return connStr2 == ""
+        ? std::nullopt
+        : (std::optional { std::move(connStr2) });
 }
 
 ref<Store> openStore(const std::string & uri_,

diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
@@ -894,7 +894,10 @@ std::list<ref<Store>> getDefaultSubstituters();
 struct StoreFactory
 {
     std::set<std::string> uriSchemes;
-    std::function<std::shared_ptr<Store> (const std::string & scheme, const std::string & uri, const Store::Params & params)> create;
+    std::function<std::shared_ptr<Store> (
+        std::string_view scheme,
+        std::optional<std::string_view> authority,
+        const Store::Params & params)> create;
     std::function<std::shared_ptr<StoreConfig> ()> getConfig;
 };
 
@@ -909,7 +912,7 @@ struct Implementations
         StoreFactory factory{
             .uriSchemes = T::uriSchemes(),
             .create =
-                ([](const std::string & scheme, const std::string & uri, const Store::Params & params)
+                ([](auto scheme, auto uri, auto & params)
                  -> std::shared_ptr<Store>
                  { return std::make_shared<T>(scheme, uri, params); }),
             .getConfig =

diff --git a/src/libstore/uds-remote-store.cc b/src/libstore/uds-remote-store.cc
@@ -36,12 +36,12 @@ UDSRemoteStore::UDSRemoteStore(const Params & params)
 
 
 UDSRemoteStore::UDSRemoteStore(
-    const std::string scheme,
-    std::string socket_path,
+    std::string_view scheme,
+    std::optional<PathView> socket_path,
     const Params & params)
     : UDSRemoteStore(params)
 {
-    path.emplace(socket_path);
+    path = socket_path;
 }
 
 
@@ -50,6 +50,7 @@ std::string UDSRemoteStore::getUri()
     if (path) {
         return std::string("unix://") + *path;
     } else {
+        // unix:// with no path also works. Change what we return?
         return "daemon";
     }
 }

diff --git a/src/libstore/uds-remote-store.hh b/src/libstore/uds-remote-store.hh
@@ -28,7 +28,10 @@ class UDSRemoteStore : public virtual UDSRemoteStoreConfig
 public:
 
     UDSRemoteStore(const Params & params);
-    UDSRemoteStore(const std::string scheme, std::string path, const Params & params);
+    UDSRemoteStore(
+        std::string_view scheme,
+        std::optional<PathView> path,
+        const Params & params);
 
     std::string getUri() override;
 

diff --git a/tests/functional/read-only-store.sh b/tests/functional/read-only-store.sh
@@ -9,7 +9,10 @@ clearStore
 happy () {
     # We can do a read-only query just fine with a read-only store
     nix --store local?read-only=true path-info $dummyPath
-
+
+    # `local://` also works.
+    nix --store local://?read-only=true path-info $dummyPath
+
     # We can "write" an already-present store-path a read-only store, because no IO is actually required
     nix-store --store local?read-only=true --add dummy
 }