From d2ce2e89b178e7e7467cf4c1e572704a4c2ca75e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 19 May 2023 10:56:59 -0400 Subject: [PATCH] Split `OptionalPathSetting` from `PathSetting` Rather than doing `allowEmpty` as boolean, have separate types and use `std::optional`. This makes it harder to forget the possibility of an empty path. The `build-hook` setting was categorized as a `PathSetting`, but actually it was split into arguments. No good! Now, it is `Setting` which actually reflects what it means and how it is used. Because of the subtyping, we now also have support for `Setting>` in general. I imagine this can be used to clean up many more settings also. --- src/libstore/build/hook-instance.cc | 6 +- src/libstore/build/local-derivation-goal.cc | 8 ++- src/libstore/globals.cc | 5 +- src/libstore/globals.hh | 6 +- src/libstore/local-fs-store.hh | 14 ++--- src/libstore/store-api.hh | 2 +- src/libutil/abstract-setting-to-json.hh | 1 + src/libutil/config-impl.hh | 61 +++++++++++++++++++- src/libutil/config.cc | 63 ++++++++------------- src/libutil/config.hh | 31 ++++++++-- 10 files changed, 134 insertions(+), 63 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>(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 7f87cdf55..cf44779b7 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -65,8 +65,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, @@ -1428,7 +1429,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(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 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 #include "config.hh" +#include "json-utils.hh" namespace nix { template diff --git a/src/libutil/config-impl.hh b/src/libutil/config-impl.hh index b6cae5ec3..14822050e 100644 --- a/src/libutil/config-impl.hh +++ b/src/libutil/config-impl.hh @@ -50,8 +50,11 @@ template<> void BaseSetting>::appendOrSet(std::set template void BaseSetting::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); } @@ -68,4 +71,60 @@ void BaseSetting::set(const std::string & str, bool append) } } +template<> void BaseSetting::convertToArg(Args & args, const std::string & category); + +template +void BaseSetting::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) +DECLARE_CONFIG_SERIALISER(bool) +DECLARE_CONFIG_SERIALISER(Strings) +DECLARE_CONFIG_SERIALISER(StringSet) +DECLARE_CONFIG_SERIALISER(StringMap) +DECLARE_CONFIG_SERIALISER(std::set) + +template +T BaseSetting::parse(const std::string & str) const +{ + static_assert(std::is_integral::value, "Integer required."); + + if (auto n = string2Int(str)) + return *n; + else + throw UsageError("setting '%s' has invalid value '%s'", name, str); +} + +template +std::string BaseSetting::to_string() const +{ + static_assert(std::is_integral::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 -void BaseSetting::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::parse(const std::string & str) const { return str; @@ -252,21 +229,17 @@ template<> std::string BaseSetting::to_string() const return value; } -template -T BaseSetting::parse(const std::string & str) const +template<> std::optional BaseSetting>::parse(const std::string & str) const { - static_assert(std::is_integral::value, "Integer required."); - if (auto n = string2Int(str)) - return *n; + if (str == "") + return std::nullopt; else - throw UsageError("setting '%s' has invalid value '%s'", name, str); + return { str }; } -template -std::string BaseSetting::to_string() const +template<> std::string BaseSetting>::to_string() const { - static_assert(std::is_integral::value, "Integer required."); - return std::to_string(value); + return value ? *value : ""; } template<> bool BaseSetting::parse(const std::string & str) const @@ -403,15 +376,25 @@ template class BaseSetting; template class BaseSetting; template class BaseSetting>; +static Path parsePath(const AbstractSetting & s, const std::string & str) +{ + 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 { - if (str == "") { - if (allowEmpty) - return ""; - else - throw UsageError("setting '%s' cannot be empty", name); - } else - return canonPath(str); + return parsePath(*this, str); +} + +std::optional 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) 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 { - bool allowEmpty; - public: PathSetting(Config * options, - bool allowEmpty, const Path & def, const std::string & name, const std::string & description, const std::set & aliases = {}) : BaseSetting(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> +{ +public: + + OptionalPathSetting(Config * options, + const std::optional & def, + const std::string & name, + const std::string & description, + const std::set & aliases = {}) + : BaseSetting>(def, true, name, description, aliases) + { + options->addSetting(this); + } + + std::optional parse(const std::string & str) const override; + + void operator =(const std::optional & v) { this->assign(v); } +}; + struct GlobalConfig : public AbstractConfig { typedef std::vector ConfigRegistrations;