libutil: replace string-based Path with std::filesystem::path across core libraries by amaanq · Pull Request #15313 · NixOS/nix

amaanq · 2026-02-21T05:40:46Z

Motivation

This takes the std::filesystem::path migration from the CLI layer into the core libraries (libutil, libstore, libexpr, libfetchers, libflake), converting function signatures, settings fields, and locals throughout file-system.hh, archive.hh, configuration.hh, and their implementations. PathSetting becomes Setting<std::filesystem::path> in local-overlay-store.hh and local-store.hh with .get() calls at use sites, and several writeFile overloads collapse now that the Path-based wrappers in file-system.hh are gone.

Context

This builds upon cli: continue converting Path to std::filesystem::path in CLI commands #15312

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

xokdvium · 2026-02-24T19:54:48Z

    auto deleteFromStore = [&](std::string_view baseName, bool isKnownPath) {
        Path path = storeDir + "/" + std::string(baseName);
-        Path realPath = config->realStoreDir + "/" + std::string(baseName);
+        auto realPath = config->realStoreDir.get() / std::string(baseName);


Oh we should be incredibly careful with the use of operator/ because if RHS iss absolute everything will blow up and we'll have a thousand CVEs on our hands.

Yeah that's a good point, here deleteFromStore is only called with either a store path or an entry from readdir, which will only give us the filename component.

xokdvium · 2026-02-24T19:54:56Z

            AutoCloseFD tmpDirFd = openDirectory(realPath);
            if (!tmpDirFd || !lockFile(tmpDirFd.get(), ltWrite, false)) {
-                debug("skipping locked tempdir '%s'", realPath);
+                debug("skipping locked tempdir '%s'", PathFmt(realPath));


Suggested change

debug("skipping locked tempdir '%s'", PathFmt(realPath));

debug("skipping locked tempdir %s", PathFmt(realPath));

xokdvium · 2026-02-24T19:56:28Z

            if (st.st_uid != 0 || st.st_gid != gr->gr_gid || (st.st_mode & ~S_IFMT) != perm) {
                if (chown(config->realStoreDir.get().c_str(), 0, gr->gr_gid) == -1)
-                    throw SysError("changing ownership of path '%1%'", config->realStoreDir);
+                    throw SysError("changing ownership of path '%s'", PathFmt(config->realStoreDir.get()));


Suggested change

throw SysError("changing ownership of path '%s'", PathFmt(config->realStoreDir.get()));

throw SysError("changing ownership of path %s", PathFmt(config->realStoreDir.get()));

xokdvium · 2026-02-24T19:56:42Z

        0600);
    if (!fdGCLock)
-        throw SysError("opening global GC lock '%1%'", fnGCLock);
+        throw SysError("opening global GC lock '%1%'", PathFmt(fnGCLock));


Suggested change

throw SysError("opening global GC lock '%1%'", PathFmt(fnGCLock));

throw SysError("opening global GC lock %1%", PathFmt(fnGCLock));

xokdvium · 2026-02-24T19:57:24Z

    AutoCloseDir dir(opendir(path.c_str()));
    if (!dir)
-        throw SysError("opening directory '%1%'", path);
+        throw SysError("opening directory '%s'", PathFmt(path));


Suggested change

throw SysError("opening directory '%s'", PathFmt(path));

throw SysError("opening directory %s", PathFmt(path));

xokdvium · 2026-02-24T19:57:33Z

    }
    if (errno)
-        throw SysError("reading directory '%1%'", path);
+        throw SysError("reading directory '%s'", PathFmt(path));


Suggested change

throw SysError("reading directory '%s'", PathFmt(path));

throw SysError("reading directory %s", PathFmt(path));

xokdvium · 2026-02-24T19:57:41Z

-    if (std::regex_search(path, std::regex("\\.app/Contents/.+$"))) {
-        debug("'%1%' is not allowed to be linked in macOS", path);
+    if (std::regex_search(path.string(), std::regex("\\.app/Contents/.+$"))) {
+        debug("'%s' is not allowed to be linked in macOS", PathFmt(path));


Suggested change

debug("'%s' is not allowed to be linked in macOS", PathFmt(path));

debug("%s is not allowed to be linked in macOS", PathFmt(path));

xokdvium · 2026-02-24T19:57:55Z

    /* This can still happen on top-level files. */
    if (st.st_nlink > 1 && inodeHash.count(st.st_ino)) {
-        debug("'%s' is already linked, with %d other file(s)", path, st.st_nlink - 2);
+        debug("'%s' is already linked, with %d other file(s)", PathFmt(path), st.st_nlink - 2);


Suggested change

debug("'%s' is already linked, with %d other file(s)", PathFmt(path), st.st_nlink - 2);

debug("%s is already linked, with %d other file(s)", PathFmt(path), st.st_nlink - 2);

xokdvium · 2026-02-24T19:58:09Z

            .hash;
    });
-    debug("'%1%' has hash '%2%'", path, hash.to_string(HashFormat::Nix32, true));
+    debug("'%s' has hash '%s'", PathFmt(path), hash.to_string(HashFormat::Nix32, true));


Suggested change

debug("'%s' has hash '%s'", PathFmt(path), hash.to_string(HashFormat::Nix32, true));

debug("%s has hash '%s'", PathFmt(path), hash.to_string(HashFormat::Nix32, true));

xokdvium · 2026-02-24T19:58:46Z

-    if (!isInStore(path))
-        throw BadStorePath("path '%1%' is not in the Nix store", path);
+    if (!isInStore(path.string()))
+        throw BadStorePath("path '%1%' is not in the Nix store", PathFmt(path));


Suggested change

throw BadStorePath("path '%1%' is not in the Nix store", PathFmt(path));

throw BadStorePath("path %1% is not in the Nix store", PathFmt(path));

Ericson2314 · 2026-02-24T20:00:13Z

Sorry I missed those pathfmt quotes @xokdvium

xokdvium · 2026-02-24T20:01:25Z

        }
    } catch (Error & e) {
-        e.addTrace({}, "writing file '%1%'", path);
+        e.addTrace({}, "writing file '%s'", PathFmt(path));


Suggested change

e.addTrace({}, "writing file '%s'", PathFmt(path));

e.addTrace({}, "writing file %s", PathFmt(path));

xokdvium · 2026-02-24T20:01:32Z

+    AutoCloseFD fd = toDescriptor(open(path.parent_path().c_str(), O_RDONLY, 0));
    if (!fd)
-        throw SysError("opening file '%1%'", path);
+        throw SysError("opening file '%s'", PathFmt(path));


Suggested change

throw SysError("opening file '%s'", PathFmt(path));

throw SysError("opening file %s", PathFmt(path));

xokdvium · 2026-02-24T20:01:40Z

+        AutoCloseFD fd = toDescriptor(open(path.string().c_str(), O_RDONLY, 0));
        if (!fd)
-            throw SysError("opening file '%1%'", path);
+            throw SysError("opening file '%s'", PathFmt(path));


Suggested change

throw SysError("opening file '%s'", PathFmt(path));

throw SysError("opening file %s", PathFmt(path));

xokdvium · 2026-02-24T20:01:48Z

            )
        == -1)
-        throw SysError("creating directory '%1%'", path);
+        throw SysError("creating directory '%s'", PathFmt(path));


Suggested change

throw SysError("creating directory '%s'", PathFmt(path));

throw SysError("creating directory %s", PathFmt(path));

xokdvium · 2026-02-24T20:02:13Z

+std::pair<AutoCloseFD, std::filesystem::path> createTempFile(const std::filesystem::path & prefix)
 {
-    Path tmpl(defaultTempDir().string() + "/" + prefix + ".XXXXXX");
+    std::filesystem::path tmpl(defaultTempDir() / (prefix.string() + ".XXXXXX"));


What if prefix is an absolute path? Please add an assert at least

Yeah good point, as of now prefix is always a hardcoded literal (like nix-shell) but an assert would be good here.

xokdvium · 2026-02-24T20:02:41Z

    static std::atomic<uint32_t> counter(std::random_device{}());
    auto tmpRoot = canonPath(root.empty() ? defaultTempDir().string() : root.string(), true);
-    return fmt("%1%/%2%-%3%-%4%", tmpRoot, suffix, getpid(), counter.fetch_add(1, std::memory_order_relaxed));
+    return tmpRoot / fmt("%s-%s-%s", suffix, getpid(), counter.fetch_add(1, std::memory_order_relaxed));


What if suffix is an absolute path? This will discard LHS

xokdvium · 2026-02-24T20:04:13Z

        {
            auto setUpdateLock = [&](auto && fileName) {
-                uploadLock = openLockFile(currentLoad + "/" + escapeUri(fileName) + ".upload-lock", true);
+                uploadLock = openLockFile(currentLoad / (escapeUri(fileName) + ".upload-lock"), true);


Can escapeUri return an absolute path?

no it replaces all instances of / with _

Drop single quotes around PathFmt format args in error and debug messages across gc.cc, local-store.cc, optimise-store.cc, store-api.cc, and file-system.cc; PathFmt already handles presentation so the extra quoting is redundant. Add an assert in createTempFile that prefix is not absolute, since operator/ silently discards LHS when RHS is absolute.

…5313

- file-system: drop redundant quotes around `PathFmt` and assert relative paths - libutil, libstore: fix mingw cross-compilation breakages

puffnfresh · 2026-03-01T20:53:39Z

    const auto & localSettings = config->getLocalSettings();
    const auto & gcSettings = localSettings.getGCSettings();
-    createDirs(gcRootsDir);
+    if (!pathExists(gcRootsDir)) {


This accidentally reverts 9799023

libutil: replace string-based Path with std::filesystem::path across core libraries

- file-system: drop redundant quotes around `PathFmt` and assert relative paths - libutil, libstore: fix mingw cross-compilation breakages

amaanq requested review from Ericson2314 and edolstra as code owners February 21, 2026 05:40

github-actions Bot added new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking c api Nix as a C library with a stable interface labels Feb 21, 2026

amaanq changed the title ~~No path libutil~~ libutil: replace string-based Path with std::filesystem::path across core libraries Feb 21, 2026

amaanq force-pushed the no-path-libutil branch 2 times, most recently from 1d54304 to 73960d1 Compare February 21, 2026 08:02