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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ericson2314 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 1 commit
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
13 changes: 13 additions & 0 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,18 @@ std::string BaseSetting<StringMap>::to_string() const
[](const auto & kvpair) { return kvpair.first + "=" + kvpair.second; });
}

template<>
std::filesystem::path BaseSetting<std::filesystem::path>::parse(const std::string & str) const
{
return std::filesystem::path(str).lexically_normal();

@xokdvium xokdvium Nov 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also ensure that the path is absolute? That's what PathSetting requires currently

@Ericson2314 Ericson2314 Nov 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh yes good point.

}

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

template class BaseSetting<int>;
template class BaseSetting<unsigned int>;
template class BaseSetting<long>;
Expand All @@ -445,6 +457,7 @@ template class BaseSetting<Strings>;
template class BaseSetting<StringSet>;
template class BaseSetting<StringMap>;
template class BaseSetting<std::set<ExperimentalFeature>>;
template class BaseSetting<std::filesystem::path>;

static Path parsePath(const AbstractSetting & s, const std::string & str)
{
Expand Down
1 change: 1 addition & 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 @@ -134,6 +134,7 @@ DECLARE_CONFIG_SERIALISER(Strings)
DECLARE_CONFIG_SERIALISER(StringSet)
DECLARE_CONFIG_SERIALISER(StringMap)
DECLARE_CONFIG_SERIALISER(std::set<ExperimentalFeature>)
DECLARE_CONFIG_SERIALISER(std::filesystem::path)

template<typename T>
T BaseSetting<T>::parse(const std::string & str) const
Expand Down
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::filesystem::path> jsonLogPath{
this,
"",
{},

@xokdvium xokdvium Nov 26, 2025

Copy link
Copy Markdown
Contributor

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?

@xokdvium xokdvium Nov 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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?

@Ericson2314 Ericson2314 Nov 26, 2025

Copy link
Copy Markdown
Member

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.

@edolstra edolstra Nov 26, 2025

Copy link
Copy Markdown
Member Author

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.

@xokdvium xokdvium Nov 26, 2025

Copy link
Copy Markdown
Contributor

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

@xokdvium xokdvium Nov 26, 2025

Copy link
Copy Markdown
Contributor

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.

@Ericson2314 Ericson2314 Nov 26, 2025

Copy link
Copy Markdown
Member

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
2 changes: 1 addition & 1 deletion src/libutil/logging.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ void applyJSONLogger()
if (!loggerSettings.jsonLogPath.get().empty()) {
try {
std::vector<std::unique_ptr<Logger>> loggers;
loggers.push_back(makeJSONLogger(std::filesystem::path(loggerSettings.jsonLogPath.get()), false));
loggers.push_back(makeJSONLogger(loggerSettings.jsonLogPath.get(), false));
try {
logger = makeTeeLogger(std::move(logger), std::move(loggers));
} catch (...) {
Expand Down
Loading