Add Setting<std::filesystem::path> and Setting<std::optional<std::filesystem::path>> specializations
#14632
Conversation
Like PathSetting, this normalizes the path (without resolving symlinks).
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
src/libutil/configuration.cc
Outdated
| template<> | ||
| std::filesystem::path BaseSetting<std::filesystem::path>::parse(const std::string & str) const | ||
| { | ||
| return std::filesystem::path(str).lexically_normal(); |
There was a problem hiding this comment.
Shouldn't we also ensure that the path is absolute? That's what PathSetting requires currently
xokdvium
left a comment
xokdvium
left a comment
There was a problem hiding this comment.
We should probably ensure that the path is not absolute, as PathSetting already does.
Ericson2314
force-pushed
the
path-setting
branch
from
93c51ac to
6c4ed46
Compare
|
Fixed, we can just use |
Ericson2314
changed the title
Setting<std::filesystem::path> specialization
| Setting<std::filesystem::path> jsonLogPath{ | ||
| this, | ||
| "", | ||
| {}, |
There was a problem hiding this comment.
This doesn't look right tho? What's a default constructed path? Shouldn't this be an optional?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah right Setting<Path> is just Setting<std::string>, the thing was totally busted before.
There was a problem hiding this comment.
The intent of this PR is that the path can be empty. E.g. json-log-path can be unset.
There was a problem hiding this comment.
But the semantics of an empty path is that of a std::nullopt. Empty path doesn't make much sense
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
edolstra
force-pushed
the
path-setting
branch
from
6c4ed46 to
93c51ac
Compare
Ericson2314
changed the title
Setting<std::filesystem::path> specializationSetting<std::filesystem::path> and Setting<std::optional<std::filesystem::path>> specializations
Ericson2314
force-pushed
the
path-setting
branch
from
b5c2501 to
1e36f20
Compare
|
Modulo the CI fixes sounds good |
|
Ah, precompiled headers strike again |
3 participants
Motivation
Like
PathSetting, this normalizes the path (without resolving symlinks).Eventually, we should replace
PathSettingbySetting<std::filesystem::path>, andOptionalPathSettingbySetting<std::optional<std::filesystem::path>>.Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.