libutil: Add PathFmt wrapped type for formatting fs::path, fix all double-quoting issues#15030
Conversation
github-actions
bot
added
new-cli
xokdvium
force-pushed
the
path-fmt-squash-double-quotes
branch
from
303f27d to
ffc9a41
Compare
| createOutLinksMaybe(buildables, store); | ||
|
|
||
| logger->cout("%s", app.program); | ||
| logger->cout("%s", app.program.string()); |
There was a problem hiding this comment.
This was a regression in 2.33. @Ericson2314
| script += fmt("SHELL=\"%s\"\n", shell); | ||
| if (foundInteractive) | ||
| script += fmt("PATH=\"%s${PATH:+:$PATH}\"\n", std::filesystem::path(shell).parent_path()); | ||
| script += fmt("PATH=\"%s${PATH:+:$PATH}\"\n", std::filesystem::path(shell).parent_path().string()); |
| std::cout << "Additional system types: " << concatStringsSep(", ", settings.extraPlatforms.get()) << "\n"; | ||
| std::cout << "Features: " << concatStringsSep(", ", cfg) << "\n"; | ||
| std::cout << "System configuration file: " << (settings.nixConfDir / "nix.conf") << "\n"; | ||
| std::cout << "System configuration file: " << (settings.nixConfDir / "nix.conf").string() << "\n"; |
There was a problem hiding this comment.
A regression in 2.33 too
| auto rootCgroupPath = *cgroupFS / getRootCgroup().rel(); | ||
| if (!pathExists(rootCgroupPath)) | ||
| throw Error("expected cgroup directory '%s'", rootCgroupPath); | ||
| throw Error("expected cgroup directory %s", PathFmt(rootCgroupPath)); |
| execv(buildHook.native().c_str(), stringsToCharPtrs(args).data()); | ||
|
|
||
| throw SysError("executing '%s'", buildHook); | ||
| throw SysError("executing %s", PathFmt(buildHook)); |
There was a problem hiding this comment.
Also double quotes
…uble-quoting issues This will once and for all get rid of all double-quoting issues. On windows the quoting is doubly bad because it escaped all \ to \\, which is very bad for error messages. In order to prevent future regression std::filesystem::path formatting now must use a special type PathFmt (like Magenta). In the future we could even change how we render filesystem paths.
xokdvium
force-pushed
the
path-fmt-squash-double-quotes
branch
from
ffc9a41 to
6dd89b5
Compare
github-actions
bot
added
the
with-tests
|
Can't we do this automatically, without the |
That could be an option. Ideally and eventually when strings are eradicated as paths we'd just have an overload for operator% that does the quoting for us. But considering just how many silent regressions it causes I think it's only logical to force compilation errors when not explicit. That necessarily forces changes to call sites to update all of the quoting – as is the intention. Evidence shows that we step into this footgun time and time again, so requiring changes is better than having more silent regressions. |
|
Yeah but my point is that if we change the default formatting to not include quotes, then we don't have this footgun anymore. It will behave like every other type (in particular |
|
Thank you for fixing. Here's a silly workaround in the meantime: |
4 participants
Motivation
This will once and for all get rid of all double-quoting issues. On windows the quoting
is doubly bad because it escaped all \ to \, which is very bad for error messages. In
order to prevent future regressions of
std::filesystem::pathformatting now must use a specialtype
PathFmt(likeMagenta). In the future we could even change how we render filesystem paths.Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.