Skip to content

Add Setting<std::filesystem::path> and Setting<std::optional<std::filesystem::path>> specializations #14632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 27, 2025
Merged

Add Setting<std::filesystem::path> and Setting<std::optional<std::filesystem::path>> specializations #14632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93c51ac
Add Setting<std::filesystem::path> specialization
edolstra Nov 24, 2025
37cf990
Merge branch 'master' into path-setting
Ericson2314 Nov 26, 2025
1e36f20
Fix issues with `std::filesystem::path` settings
Ericson2314 Nov 26, 2025
80c545b
Fix include errors masked by precompiled headers
Ericson2314 Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/libutil/configuration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,42 @@ std::string BaseSetting<StringMap>::to_string() const
[](const auto & kvpair) { return kvpair.first + "=" + kvpair.second; });
}

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);
}

template<>
std::filesystem::path BaseSetting<std::filesystem::path>::parse(const std::string & str) const
{
return parsePath(*this, str);
}

template<>
std::string BaseSetting<std::filesystem::path>::to_string() const
{
return value.string();
}

template<>
std::optional<std::filesystem::path>
BaseSetting<std::optional<std::filesystem::path>>::parse(const std::string & str) const
{
if (str == "")
return std::nullopt;
else
return parsePath(*this, str);
}

template<>
std::string BaseSetting<std::optional<std::filesystem::path>>::to_string() const
{
return value ? value->string() : "";
}

template class BaseSetting<int>;
template class BaseSetting<unsigned int>;
template class BaseSetting<long>;
Expand All @@ -445,14 +481,8 @@ template class BaseSetting<Strings>;
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);
}
template class BaseSetting<std::filesystem::path>;
template class BaseSetting<std::optional<std::filesystem::path>>;

PathSetting::PathSetting(
Config * options,
Expand Down
3 changes: 3 additions & 0 deletions src/libutil/include/nix/util/config-impl.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "nix/util/configuration.hh"
#include "nix/util/args.hh"
#include "nix/util/logging.hh"
#include "nix/util/file-path.hh"

namespace nix {

Expand Down Expand Up @@ -134,6 +135,8 @@ DECLARE_CONFIG_SERIALISER(Strings)
DECLARE_CONFIG_SERIALISER(StringSet)
DECLARE_CONFIG_SERIALISER(StringMap)
DECLARE_CONFIG_SERIALISER(std::set<ExperimentalFeature>)
DECLARE_CONFIG_SERIALISER(std::filesystem::path)
DECLARE_CONFIG_SERIALISER(std::optional<std::filesystem::path>)

template<typename T>
T BaseSetting<T>::parse(const std::string & str) const
Expand Down
5 changes: 5 additions & 0 deletions src/libutil/include/nix/util/file-path.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "nix/util/types.hh"
#include "nix/util/os-string.hh"
#include "nix/util/json-non-null.hh"

namespace nix {

Expand Down Expand Up @@ -53,4 +54,8 @@ std::optional<std::filesystem::path> maybePath(PathView path);

std::filesystem::path pathNG(PathView path);

template<>
struct json_avoids_null<std::filesystem::path> : std::true_type
{};

} // namespace nix
4 changes: 2 additions & 2 deletions src/libutil/include/nix/util/logging.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ struct LoggerSettings : Config
expression evaluation errors.
)"};

Setting<Path> jsonLogPath{
Setting<std::optional<std::filesystem::path>> jsonLogPath{
this,
"",
{},
Copy link
Contributor

@xokdvium xokdvium Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right tho? What's a default constructed path? Shouldn't this be an optional?

Copy link
Contributor

@xokdvium xokdvium Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below the json logger is only created when the path is not empty. Shouldn't this get the same treatment as OptionalPath when an empty string signifies std::nullopt?

Copy link
Member

@Ericson2314 Ericson2314 Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right Setting<Path> is just Setting<std::string>, the thing was totally busted before.

Copy link
Member Author

@edolstra edolstra Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of this PR is that the path can be empty. E.g. json-log-path can be unset.

Copy link
Contributor

@xokdvium xokdvium Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the semantics of an empty path is that of a std::nullopt. Empty path doesn't make much sense

Copy link
Contributor

@xokdvium xokdvium Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the settings perspective it's the same, but my slight quip is that it would be more legible in the code if std::filesystem::path specified in the settings would have the invariant of being absolute and non-empty. Not really a blocker since it's already this way, but I think it'd be nice if we are moving to std::filesystem for this stuff.

Not exactly sure what would happen if someone accidentally tried doing any I/O with an empty path with std::filesystem and it seems nice to avoid that altogether.

Copy link
Member

@Ericson2314 Ericson2314 Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should use std::optional and work just like OptionalPathSetting. (It could just replace OptionalPathSetting.) It should use parsePath so please put that back.

"json-log-path",
R"(
A file or unix socket to which JSON records of Nix's log output are
Expand Down
4 changes: 2 additions & 2 deletions src/libutil/logging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,10 @@ std::unique_ptr<Logger> makeJSONLogger(const std::filesystem::path & path, bool

void applyJSONLogger()
{
if (!loggerSettings.jsonLogPath.get().empty()) {
if (auto & opt = loggerSettings.jsonLogPath.get()) {
try {
std::vector<std::unique_ptr<Logger>> loggers;
loggers.push_back(makeJSONLogger(std::filesystem::path(loggerSettings.jsonLogPath.get()), false));
loggers.push_back(makeJSONLogger(*opt, false));
try {
logger = makeTeeLogger(std::move(logger), std::move(loggers));
} catch (...) {
Expand Down
Loading