From e3c289dbe945415367dc1f54f85ad2452f5a97e0 Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Sat, 17 Aug 2024 20:55:41 +0200 Subject: [PATCH] libutil/config: unify path setting types There have been multiple setting types for paths that are supposed to be canonicalised, depending on whether zero or one, one, or any number of paths is to be specified. Naturally, they behaved in slightly different ways in the code. Simplify things by unifying them and removing special behaviour (mainly the "multiple paths type can coerce to boolean" thing). Change-Id: I7c1ce95e9c8e1829a866fb37d679e167811e9705 --- src/libcmd/repl.cc | 2 +- src/libexpr/eval-settings.hh | 2 +- src/libstore/globals.hh | 2 +- src/libstore/local-fs-store.hh | 8 ++-- src/libstore/store-api.hh | 2 +- src/libutil/config.cc | 52 +++------------------- src/libutil/config.hh | 69 +++++++---------------------- tests/unit/libutil/paths-setting.cc | 16 +------ 8 files changed, 32 insertions(+), 121 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index b8bfc25eb..ce19bc1eb 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -926,7 +926,7 @@ void NixRepl::loadFiles() void NixRepl::loadReplOverlays() { - if (!evalSettings.replOverlays) { + if (evalSettings.replOverlays.get().empty()) { return; } diff --git a/src/libexpr/eval-settings.hh b/src/libexpr/eval-settings.hh index 4673c509b..444e298a0 100644 --- a/src/libexpr/eval-settings.hh +++ b/src/libexpr/eval-settings.hh @@ -151,7 +151,7 @@ struct EvalSettings : Config This is useful for debugging warnings in third-party Nix code. )"}; - PathsSetting replOverlays{this, Paths(), "repl-overlays", + PathsSetting replOverlays{this, Paths(), "repl-overlays", R"( A list of files containing Nix expressions that can be used to add default bindings to [`nix diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 4709ac715..aba99d969 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -634,7 +634,7 @@ public: line. )"}; - OptionalPathSetting diffHook{ + PathsSetting> diffHook{ this, std::nullopt, "diff-hook", R"( Absolute path to an executable capable of diffing build diff --git a/src/libstore/local-fs-store.hh b/src/libstore/local-fs-store.hh index 56de47424..5b7a149cb 100644 --- a/src/libstore/local-fs-store.hh +++ b/src/libstore/local-fs-store.hh @@ -11,21 +11,21 @@ struct LocalFSStoreConfig : virtual StoreConfig { using StoreConfig::StoreConfig; - const OptionalPathSetting rootDir{this, std::nullopt, + const PathsSetting> rootDir{this, std::nullopt, "root", "Directory prefixed to all other paths."}; - const PathSetting stateDir{this, + const PathsSetting stateDir{this, rootDir.get() ? *rootDir.get() + "/nix/var/nix" : settings.nixStateDir, "state", "Directory where Lix will store state."}; - const PathSetting logDir{this, + const PathsSetting logDir{this, rootDir.get() ? *rootDir.get() + "/nix/var/log/nix" : settings.nixLogDir, "log", "directory where Lix will store log files."}; - const PathSetting realStoreDir{this, + const PathsSetting realStoreDir{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 25bc0c823..efd0e4d9b 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -144,7 +144,7 @@ struct StoreConfig : public Config return std::nullopt; } - const PathSetting storeDir_{this, settings.nixStore, + const PathsSetting storeDir_{this, settings.nixStore, "store", R"( Logical location of the Nix store, usually diff --git a/src/libutil/config.cc b/src/libutil/config.cc index 3c4f4798b..8e20f1321 100644 --- a/src/libutil/config.cc +++ b/src/libutil/config.cc @@ -434,34 +434,12 @@ static Path parsePath(const AbstractSetting & s, const std::string & str) return canonPath(str); } -PathSetting::PathSetting(Config * options, - const Path & def, - const std::string & name, - const std::string & description, - const std::set & aliases) - : BaseSetting(def, true, name, description, aliases) -{ - options->addSetting(this); -} - -Path PathSetting::parse(const std::string & str) const +template<> Path PathsSetting::parse(const std::string & str) const { return parsePath(*this, str); } - -OptionalPathSetting::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 OptionalPathSetting::parse(const std::string & str) const +template<> std::optional PathsSetting>::parse(const std::string & str) const { if (str == "") return std::nullopt; @@ -469,23 +447,7 @@ std::optional OptionalPathSetting::parse(const std::string & str) const return parsePath(*this, str); } -void OptionalPathSetting::operator =(const std::optional & v) -{ - this->assign(v); -} - -PathsSetting::PathsSetting(Config * options, - const Paths & def, - const std::string & name, - const std::string & description, - const std::set & aliases) - : BaseSetting(def, true, name, description, aliases) -{ - options->addSetting(this); -} - - -Paths PathsSetting::parse(const std::string & str) const +template<> Paths PathsSetting::parse(const std::string & str) const { auto strings = tokenizeString(str); Paths parsed; @@ -497,10 +459,10 @@ Paths PathsSetting::parse(const std::string & str) const return parsed; } -PathsSetting::operator bool() const noexcept -{ - return !get().empty(); -} +template class PathsSetting; +template class PathsSetting>; +template class PathsSetting; + bool GlobalConfig::set(const std::string & name, const std::string & value) { diff --git a/src/libutil/config.hh b/src/libutil/config.hh index d1812f47e..dbca4b406 100644 --- a/src/libutil/config.hh +++ b/src/libutil/config.hh @@ -353,69 +353,32 @@ 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. + * A special setting for Paths. + * These are automatically canonicalised (e.g. "/foo//bar/" becomes "/foo/bar"). + * The empty string is not permitted when a path is required. */ -class PathSetting : public BaseSetting +template +class PathsSetting : public BaseSetting { public: - - PathSetting(Config * options, - const Path & def, - const std::string & name, - const std::string & description, - const std::set & aliases = {}); - - Path parse(const std::string & str) const override; - - Path operator +(const char * p) const { return value + p; } - - 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 = {}); - - std::optional parse(const std::string & str) const override; - - void operator =(const std::optional & v); -}; - -/** - * Like `OptionalPathSetting`, but allows multiple paths. - */ -class PathsSetting : public BaseSetting -{ -public: - PathsSetting(Config * options, - const Paths & def, + const T & def, const std::string & name, const std::string & description, - const std::set & aliases = {}); + const std::set & aliases = {}, + const bool documentDefault = true, + std::optional experimentalFeature = std::nullopt) + : BaseSetting(def, documentDefault, name, description, aliases, std::move(experimentalFeature)) + { + options->addSetting(this); + } - Paths parse(const std::string & str) const override; + T parse(const std::string & str) const override; - void operator =(const Paths & v); - - operator bool() const noexcept; + void operator =(const T & v) { this->assign(v); } }; + struct GlobalConfig : public AbstractConfig { typedef std::vector ConfigRegistrations; diff --git a/tests/unit/libutil/paths-setting.cc b/tests/unit/libutil/paths-setting.cc index b450b0cf9..17cb125c8 100644 --- a/tests/unit/libutil/paths-setting.cc +++ b/tests/unit/libutil/paths-setting.cc @@ -15,7 +15,7 @@ public: : Config() { } - PathsSetting paths{this, Paths(), "paths", "documentation"}; + PathsSetting paths{this, Paths(), "paths", "documentation"}; }; struct PathsSettingTest : public ::testing::Test { @@ -55,20 +55,6 @@ TEST_F(PathsSettingTest, parse) { ); } -TEST_F(PathsSettingTest, bool) { - auto config = mkConfig(); - // No paths: - ASSERT_FALSE(config.paths); - // Set a path: - config.set("paths", "/puppy.nix"); - // Now there are paths: - ASSERT_TRUE(config.paths); - - // Multiple paths count too: - config.set("paths", "/puppy.nix /doggy.nix"); - ASSERT_TRUE(config.paths); -} - TEST_F(PathsSettingTest, append) { auto config = mkConfig();