Merge pull request #8374 from obsidiansystems/improve-path-setting

Split `OptionalPathSetting` from `PathSetting`

14 files changed, 230 insertions, 87 deletions

diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc
index 075ad554f..337c60bd4 100644
--- a/src/libstore/build/hook-instance.cc
+++ b/src/libstore/build/hook-instance.cc
@@ -5,14 +5,14 @@ namespace nix {
 
 HookInstance::HookInstance()
 {
-    debug("starting build hook '%s'", settings.buildHook);
+    debug("starting build hook '%s'", concatStringsSep(" ", settings.buildHook.get()));
 
-    auto buildHookArgs = tokenizeString<std::list<std::string>>(settings.buildHook.get());
+    auto buildHookArgs = settings.buildHook.get();
 
     if (buildHookArgs.empty())
         throw Error("'build-hook' setting is empty");
 
-    auto buildHook = buildHookArgs.front();
+    auto buildHook = canonPath(buildHookArgs.front());
     buildHookArgs.pop_front();
 
     Strings args;
diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index 09c1fcf08..ea7c52098 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -64,8 +64,9 @@ void handleDiffHook(
     const Path & tryA, const Path & tryB,
     const Path & drvPath, const Path & tmpDir)
 {
-    auto diffHook = settings.diffHook;
-    if (diffHook != "" && settings.runDiffHook) {
+    auto & diffHookOpt = settings.diffHook.get();
+    if (diffHookOpt && settings.runDiffHook) {
+        auto & diffHook = *diffHookOpt;
         try {
             auto diffRes = runProgram(RunOptions {
                 .program = diffHook,
@@ -1427,7 +1428,8 @@ void LocalDerivationGoal::startDaemon()
     Store::Params params;
     params["path-info-cache-size"] = "0";
     params["store"] = worker.store.storeDir;
-    params["root"] = getLocalStore().rootDir;
+    if (auto & optRoot = getLocalStore().rootDir.get())
+        params["root"] = *optRoot;
     params["state"] = "/no-such-path";
     params["log"] = "/no-such-path";
     auto store = make_ref<RestrictedStore>(params,
diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc
index d53377239..5a4cb1824 100644
--- a/src/libstore/globals.cc
+++ b/src/libstore/globals.cc
@@ -100,7 +100,10 @@ Settings::Settings()
     if (!pathExists(nixExePath)) {
         nixExePath = getSelfExe().value_or("nix");
     }
-    buildHook = nixExePath + " __build-remote";
+    buildHook = {
+        nixExePath,
+        "__build-remote",
+    };
 }
 
 void loadConfFile()
diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh
index 820898350..b8dcf1f76 100644
--- a/src/libstore/globals.hh
+++ b/src/libstore/globals.hh
@@ -236,7 +236,7 @@ public:
         )",
         {"build-timeout"}};
 
-    PathSetting buildHook{this, true, "", "build-hook",
+    Setting<Strings> buildHook{this, {}, "build-hook",
         R"(
           The path to the helper program that executes remote builds.
 
@@ -556,8 +556,8 @@ public:
           line.
         )"};
 
-    PathSetting diffHook{
-        this, true, "", "diff-hook",
+    OptionalPathSetting diffHook{
+        this, std::nullopt, "diff-hook",
         R"(
           Absolute path to an executable capable of diffing build
           results. The hook is executed if `run-diff-hook` is true, and the
diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh
index a03bb88f5..2ee2ef0c8 100644
--- a/src/libstore/local-fs-store.hh
+++ b/src/libstore/local-fs-store.hh
@@ -15,22 +15,22 @@ struct LocalFSStoreConfig : virtual StoreConfig
     // it to omit the call to the Setting constructor. Clang works fine
     // either way.
 
-    const PathSetting rootDir{(StoreConfig*) this, true, "",
+    const OptionalPathSetting rootDir{(StoreConfig*) this, std::nullopt,
         "root",
         "Directory prefixed to all other paths."};
 
-    const PathSetting stateDir{(StoreConfig*) this, false,
-        rootDir != "" ? rootDir + "/nix/var/nix" : settings.nixStateDir,
+    const PathSetting stateDir{(StoreConfig*) this,
+        rootDir.get() ? *rootDir.get() + "/nix/var/nix" : settings.nixStateDir,
         "state",
         "Directory where Nix will store state."};
 
-    const PathSetting logDir{(StoreConfig*) this, false,
-        rootDir != "" ? rootDir + "/nix/var/log/nix" : settings.nixLogDir,
+    const PathSetting logDir{(StoreConfig*) this,
+        rootDir.get() ? *rootDir.get() + "/nix/var/log/nix" : settings.nixLogDir,
         "log",
         "directory where Nix will store log files."};
 
-    const PathSetting realStoreDir{(StoreConfig*) this, false,
-        rootDir != "" ? rootDir + "/nix/store" : storeDir, "real",
+    const PathSetting realStoreDir{(StoreConfig*) this,
+        rootDir.get() ? *rootDir.get() + "/nix/store" : storeDir, "real",
         "Physical path of the Nix store."};
 };
 
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 2ecbe2708..14a862eef 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -114,7 +114,7 @@ struct StoreConfig : public Config
         return "";
     }
 
-    const PathSetting storeDir_{this, false, settings.nixStore,
+    const PathSetting storeDir_{this, settings.nixStore,
         "store",
         R"(
           Logical location of the Nix store, usually
diff --git a/src/libutil/abstract-setting-to-json.hh b/src/libutil/abstract-setting-to-json.hh
index 7b6c3fcb5..d506dfb74 100644
--- a/src/libutil/abstract-setting-to-json.hh
+++ b/src/libutil/abstract-setting-to-json.hh
@@ -3,6 +3,7 @@
 
 #include <nlohmann/json.hpp>
 #include "config.hh"
+#include "json-utils.hh"
 
 namespace nix {
 template<typename T>
diff --git a/src/libutil/args.cc b/src/libutil/args.cc
index 081dbeb28..3cf3ed9ca 100644
--- a/src/libutil/args.cc
+++ b/src/libutil/args.cc
@@ -1,10 +1,9 @@
 #include "args.hh"
 #include "hash.hh"
+#include "json-utils.hh"
 
 #include <glob.h>
 
-#include <nlohmann/json.hpp>
-
 namespace nix {
 
 void Args::addFlag(Flag && flag_)
@@ -247,11 +246,7 @@ nlohmann::json Args::toJSON()
             j["arity"] = flag->handler.arity;
         if (!flag->labels.empty())
             j["labels"] = flag->labels;
-        // TODO With C++23 use `std::optional::tranform`
-        if (auto & xp = flag->experimentalFeature)
-            j["experimental-feature"] = showExperimentalFeature(*xp);
-        else
-            j["experimental-feature"] = nullptr;
+        j["experimental-feature"] = flag->experimentalFeature;
         flags[name] = std::move(j);
     }
 
@@ -416,11 +411,7 @@ nlohmann::json MultiCommand::toJSON()
         cat["id"] = command->category();
         cat["description"] = trim(categories[command->category()]);
         j["category"] = std::move(cat);
-        // TODO With C++23 use `std::optional::tranform`
-        if (auto xp = command->experimentalFeature())
-            cat["experimental-feature"] = showExperimentalFeature(*xp);
-        else
-            cat["experimental-feature"] = nullptr;
+        cat["experimental-feature"] = command->experimentalFeature();
         cmds[name] = std::move(j);
     }
 
diff --git a/src/libutil/config-impl.hh b/src/libutil/config-impl.hh
index d9726a907..b9639e761 100644
--- a/src/libutil/config-impl.hh
+++ b/src/libutil/config-impl.hh
@@ -53,8 +53,11 @@ template<> void BaseSetting<std::set<ExperimentalFeature>>::appendOrSet(std::set
 template<typename T>
 void BaseSetting<T>::appendOrSet(T && newValue, bool append)
 {
-    static_assert(!trait::appendable, "using default `appendOrSet` implementation with an appendable type");
+    static_assert(
+        !trait::appendable,
+        "using default `appendOrSet` implementation with an appendable type");
     assert(!append);
+
     value = std::move(newValue);
 }
 
@@ -71,4 +74,60 @@ void BaseSetting<T>::set(const std::string & str, bool append)
     }
 }
 
+template<> void BaseSetting<bool>::convertToArg(Args & args, const std::string & category);
+
+template<typename T>
+void BaseSetting<T>::convertToArg(Args & args, const std::string & category)
+{
+    args.addFlag({
+        .longName = name,
+        .description = fmt("Set the `%s` setting.", name),
+        .category = category,
+        .labels = {"value"},
+        .handler = {[this](std::string s) { overridden = true; set(s); }},
+        .experimentalFeature = experimentalFeature,
+    });
+
+    if (isAppendable())
+        args.addFlag({
+            .longName = "extra-" + name,
+            .description = fmt("Append to the `%s` setting.", name),
+            .category = category,
+            .labels = {"value"},
+            .handler = {[this](std::string s) { overridden = true; set(s, true); }},
+            .experimentalFeature = experimentalFeature,
+        });
+}
+
+#define DECLARE_CONFIG_SERIALISER(TY) \
+    template<> TY BaseSetting< TY >::parse(const std::string & str) const; \
+    template<> std::string BaseSetting< TY >::to_string() const;
+
+DECLARE_CONFIG_SERIALISER(std::string)
+DECLARE_CONFIG_SERIALISER(std::optional<std::string>)
+DECLARE_CONFIG_SERIALISER(bool)
+DECLARE_CONFIG_SERIALISER(Strings)
+DECLARE_CONFIG_SERIALISER(StringSet)
+DECLARE_CONFIG_SERIALISER(StringMap)
+DECLARE_CONFIG_SERIALISER(std::set<ExperimentalFeature>)
+
+template<typename T>
+T BaseSetting<T>::parse(const std::string & str) const
+{
+    static_assert(std::is_integral<T>::value, "Integer required.");
+
+    if (auto n = string2Int<T>(str))
+        return *n;
+    else
+        throw UsageError("setting '%s' has invalid value '%s'", name, str);
+}
+
+template<typename T>
+std::string BaseSetting<T>::to_string() const
+{
+    static_assert(std::is_integral<T>::value, "Integer required.");
+
+    return std::to_string(value);
+}
+
 }
diff --git a/src/libutil/config.cc b/src/libutil/config.cc
index 085a884dc..38d406e8a 100644
--- a/src/libutil/config.cc
+++ b/src/libutil/config.cc
@@ -219,29 +219,6 @@ void AbstractSetting::convertToArg(Args & args, const std::string & category)
 {
 }
 
-template<typename T>
-void BaseSetting<T>::convertToArg(Args & args, const std::string & category)
-{
-    args.addFlag({
-        .longName = name,
-        .description = fmt("Set the `%s` setting.", name),
-        .category = category,
-        .labels = {"value"},
-        .handler = {[this](std::string s) { overridden = true; set(s); }},
-        .experimentalFeature = experimentalFeature,
-    });
-
-    if (isAppendable())
-        args.addFlag({
-            .longName = "extra-" + name,
-            .description = fmt("Append to the `%s` setting.", name),
-            .category = category,
-            .labels = {"value"},
-            .handler = {[this](std::string s) { overridden = true; set(s, true); }},
-            .experimentalFeature = experimentalFeature,
-        });
-}
-
 template<> std::string BaseSetting<std::string>::parse(const std::string & str) const
 {
     return str;
@@ -252,21 +229,17 @@ template<> std::string BaseSetting<std::string>::to_string() const
     return value;
 }
 
-template<typename T>
-T BaseSetting<T>::parse(const std::string & str) const
+template<> std::optional<std::string> BaseSetting<std::optional<std::string>>::parse(const std::string & str) const
 {
-    static_assert(std::is_integral<T>::value, "Integer required.");
-    if (auto n = string2Int<T>(str))
-        return *n;
+    if (str == "")
+        return std::nullopt;
     else
-        throw UsageError("setting '%s' has invalid value '%s'", name, str);
+        return { str };
 }
 
-template<typename T>
-std::string BaseSetting<T>::to_string() const
+template<> std::string BaseSetting<std::optional<std::string>>::to_string() const
 {
-    static_assert(std::is_integral<T>::value, "Integer required.");
-    return std::to_string(value);
+    return value ? *value : "";
 }
 
 template<> bool BaseSetting<bool>::parse(const std::string & str) const
@@ -403,17 +376,27 @@ template class BaseSetting<StringSet>;
 template class BaseSetting<StringMap>;
 template class BaseSetting<std::set<ExperimentalFeature>>;
 
-Path PathSetting::parse(const std::string & str) const
+static Path parsePath(const AbstractSetting & s, const std::string & str)
 {
-    if (str == "") {
-        if (allowEmpty)
-            return "";
-        else
-            throw UsageError("setting '%s' cannot be empty", name);
-    } else
+    if (str == "")
+        throw UsageError("setting '%s' is a path and paths cannot be empty", s.name);
+    else
         return canonPath(str);
 }
 
+Path PathSetting::parse(const std::string & str) const
+{
+    return parsePath(*this, str);
+}
+
+std::optional<Path> OptionalPathSetting::parse(const std::string & str) const
+{
+    if (str == "")
+        return std::nullopt;
+    else
+        return parsePath(*this, str);
+}
+
 bool GlobalConfig::set(const std::string & name, const std::string & value)
 {
     for (auto & config : *configRegistrations)
diff --git a/src/libutil/config.hh b/src/libutil/config.hh
index 2675baed7..cc8532587 100644
--- a/src/libutil/config.hh
+++ b/src/libutil/config.hh
@@ -353,21 +353,20 @@ public:
 /**
  * A special setting for Paths. These are automatically canonicalised
  * (e.g. "/foo//bar/" becomes "/foo/bar").
+ *
+ * It is mandatory to specify a path; i.e. the empty string is not
+ * permitted.
  */
 class PathSetting : public BaseSetting<Path>
 {
-    bool allowEmpty;
-
 public:
 
     PathSetting(Config * options,
-        bool allowEmpty,
         const Path & def,
         const std::string & name,
         const std::string & description,
         const std::set<std::string> & aliases = {})
         : BaseSetting<Path>(def, true, name, description, aliases)
-        , allowEmpty(allowEmpty)
     {
         options->addSetting(this);
     }
@@ -379,6 +378,30 @@ public:
     void operator =(const Path & v) { this->assign(v); }
 };
 
+/**
+ * Like `PathSetting`, but the absence of a path is also allowed.
+ *
+ * `std::optional` is used instead of the empty string for clarity.
+ */
+class OptionalPathSetting : public BaseSetting<std::optional<Path>>
+{
+public:
+
+    OptionalPathSetting(Config * options,
+        const std::optional<Path> & def,
+        const std::string & name,
+        const std::string & description,
+        const std::set<std::string> & aliases = {})
+        : BaseSetting<std::optional<Path>>(def, true, name, description, aliases)
+    {
+        options->addSetting(this);
+    }
+
+    std::optional<Path> parse(const std::string & str) const override;
+
+    void operator =(const std::optional<Path> & v) { this->assign(v); }
+};
+
 struct GlobalConfig : public AbstractConfig
 {
     typedef std::vector<Config*> ConfigRegistrations;
diff --git a/src/libutil/experimental-features.hh b/src/libutil/experimental-features.hh
index 507b0cc06..faf2e9398 100644
--- a/src/libutil/experimental-features.hh
+++ b/src/libutil/experimental-features.hh
@@ -3,7 +3,7 @@
 
 #include "comparator.hh"
 #include "error.hh"
-#include "nlohmann/json_fwd.hpp"
+#include "json-utils.hh"
 #include "types.hh"
 
 namespace nix {
@@ -94,4 +94,10 @@ public:
 void to_json(nlohmann::json &, const ExperimentalFeature &);
 void from_json(const nlohmann::json &, ExperimentalFeature &);
 
+/**
+ * It is always rendered as a string
+ */
+template<>
+struct json_avoids_null<ExperimentalFeature> : std::true_type {};
+
 }
diff --git a/src/libutil/json-utils.cc b/src/libutil/json-utils.cc
new file mode 100644
index 000000000..d7220e71d
--- /dev/null
+++ b/src/libutil/json-utils.cc
@@ -0,0 +1,19 @@
+#include "json-utils.hh"
+
+namespace nix {
+
+const nlohmann::json * get(const nlohmann::json & map, const std::string & key)
+{
+    auto i = map.find(key);
+    if (i == map.end()) return nullptr;
+    return &*i;
+}
+
+nlohmann::json * get(nlohmann::json & map, const std::string & key)
+{
+    auto i = map.find(key);
+    if (i == map.end()) return nullptr;
+    return &*i;
+}
+
+}
diff --git a/src/libutil/json-utils.hh b/src/libutil/json-utils.hh
index eb00e954f..5e63c1af4 100644
--- a/src/libutil/json-utils.hh
+++ b/src/libutil/json-utils.hh
@@ -2,21 +2,77 @@
 ///@file
 
 #include <nlohmann/json.hpp>
+#include <list>
 
 namespace nix {
 
-const nlohmann::json * get(const nlohmann::json & map, const std::string & key)
-{
-    auto i = map.find(key);
-    if (i == map.end()) return nullptr;
-    return &*i;
-}
+const nlohmann::json * get(const nlohmann::json & map, const std::string & key);
+
+nlohmann::json * get(nlohmann::json & map, const std::string & key);
+
+/**
+ * For `adl_serializer<std::optional<T>>` below, we need to track what
+ * types are not already using `null`. Only for them can we use `null`
+ * to represent `std::nullopt`.
+ */
+template<typename T>
+struct json_avoids_null;
+
+/**
+ * Handle numbers in default impl
+ */
+template<typename T>
+struct json_avoids_null : std::bool_constant<std::is_integral<T>::value> {};
+
+template<>
+struct json_avoids_null<std::nullptr_t> : std::false_type {};
+
+template<>
+struct json_avoids_null<bool> : std::true_type {};
+
+template<>
+struct json_avoids_null<std::string> : std::true_type {};
+
+template<typename T>
+struct json_avoids_null<std::vector<T>> : std::true_type {};
+
+template<typename T>
+struct json_avoids_null<std::list<T>> : std::true_type {};
+
+template<typename K, typename V>
+struct json_avoids_null<std::map<K, V>> : std::true_type {};
 
-nlohmann::json * get(nlohmann::json & map, const std::string & key)
-{
-    auto i = map.find(key);
-    if (i == map.end()) return nullptr;
-    return &*i;
 }
 
+namespace nlohmann {
+
+/**
+ * This "instance" is widely requested, see
+ * https://github.com/nlohmann/json/issues/1749, but momentum has stalled
+ * out. Writing there here in Nix as a stop-gap.
+ *
+ * We need to make sure the underlying type does not use `null` for this to
+ * round trip. We do that with a static assert.
+ */
+template<typename T>
+struct adl_serializer<std::optional<T>> {
+    static std::optional<T> from_json(const json & json) {
+        static_assert(
+            nix::json_avoids_null<T>::value,
+            "null is already in use for underlying type's JSON");
+        return json.is_null()
+            ? std::nullopt
+            : std::optional { adl_serializer<T>::from_json(json) };
+    }
+    static void to_json(json & json, std::optional<T> t) {
+        static_assert(
+            nix::json_avoids_null<T>::value,
+            "null is already in use for underlying type's JSON");
+        if (t)
+            adl_serializer<T>::to_json(json, *t);
+        else
+            json = nullptr;
+    }
+};
+
 }