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<Strings>` which actually reflects what it means and how it is
used.

Because of the subtyping, we now also have support for
`Setting<std::optional<String>>` in general. I imagine this can be used
to clean up many more settings also.
This commit is contained in:
John Ericson 2023-05-19 10:56:59 -04:00
parent c8825e9d8c
commit d2ce2e89b1
10 changed files with 134 additions and 63 deletions

View file

@ -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;

View file

@ -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<RestrictedStore>(params,

View file

@ -100,7 +100,10 @@ Settings::Settings()
if (!pathExists(nixExePath)) {
nixExePath = getSelfExe().value_or("nix");
}
buildHook = nixExePath + " __build-remote";
buildHook = {
nixExePath,
"__build-remote",
};
}
void loadConfFile()

View file

@ -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

View file

@ -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."};
};

View file

@ -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

View file

@ -3,6 +3,7 @@
#include <nlohmann/json.hpp>
#include "config.hh"
#include "json-utils.hh"
namespace nix {
template<typename T>

View file

@ -50,8 +50,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);
}
@ -68,4 +71,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);
}
}

View file

@ -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,15 +376,25 @@ template class BaseSetting<StringSet>;
template class BaseSetting<StringMap>;
template class BaseSetting<std::set<ExperimentalFeature>>;
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 "";
return parsePath(*this, str);
}
std::optional<Path> OptionalPathSetting::parse(const std::string & str) const
{
if (str == "")
return std::nullopt;
else
throw UsageError("setting '%s' cannot be empty", name);
} else
return canonPath(str);
return parsePath(*this, str);
}
bool GlobalConfig::set(const std::string & name, const std::string & value)

View file

@ -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;