From a97d6d89d8961a94f593b2e3797fa7e3ca583fc9 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 22 Sep 2025 14:49:29 -0400 Subject: [PATCH] Create a second `Store::getFSAccessor` for a single store object This is sometimes easier / more performant to implement, and independently it is also a more convenient interface for many callers. The existing store-wide `getFSAccessor` is only used for - `nix why-depends` - the evaluator I hope we can get rid of it for those, too, and then we have the option of getting rid of the store-wide method. Co-authored-by: Sergei Zimmerman --- src/libfetchers/fetchers.cc | 4 ++-- src/libfetchers/github.cc | 6 ++---- .../include/nix/fetchers/meson.build | 1 - .../include/nix/fetchers/store-path-accessor.hh | 14 -------------- src/libfetchers/mercurial.cc | 4 ++-- src/libfetchers/meson.build | 1 - src/libfetchers/path.cc | 3 +-- src/libfetchers/store-path-accessor.cc | 11 ----------- src/libfetchers/tarball.cc | 3 +-- src/libstore/binary-cache-store.cc | 12 +++++++++++- src/libstore/dummy-store.cc | 9 ++++++++- .../include/nix/store/binary-cache-store.hh | 8 ++++++++ .../include/nix/store/legacy-ssh-store.hh | 7 ++++++- .../include/nix/store/local-fs-store.hh | 1 + .../include/nix/store/remote-fs-accessor.hh | 5 +++++ src/libstore/include/nix/store/remote-store.hh | 9 +++++++++ src/libstore/include/nix/store/store-api.hh | 12 +++++++++++- .../include/nix/store/uds-remote-store.hh | 5 +++++ src/libstore/local-fs-store.cc | 17 +++++++++++++++++ src/libstore/remote-fs-accessor.cc | 16 +++++++++------- src/libstore/remote-store.cc | 12 +++++++++++- src/libstore/ssh-store.cc | 5 +++++ src/libstore/store-api.cc | 5 ++--- src/nix/cat.cc | 5 ++++- src/nix/ls.cc | 5 ++++- src/nix/nix-store/nix-store.cc | 2 +- 26 files changed, 125 insertions(+), 57 deletions(-) delete mode 100644 src/libfetchers/include/nix/fetchers/store-path-accessor.hh delete mode 100644 src/libfetchers/store-path-accessor.cc diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index a6b5e295a..d40e97aa9 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -3,7 +3,6 @@ #include "nix/util/source-path.hh" #include "nix/fetchers/fetch-to-store.hh" #include "nix/util/json-utils.hh" -#include "nix/fetchers/store-path-accessor.hh" #include "nix/fetchers/fetch-settings.hh" #include @@ -332,7 +331,8 @@ std::pair, Input> Input::getAccessorUnchecked(ref sto debug("using substituted/cached input '%s' in '%s'", to_string(), store->printStorePath(storePath)); - auto accessor = makeStorePathAccessor(store, storePath); + // We just ensured the store object was there + auto accessor = ref{store->getFSAccessor(storePath)}; accessor->fingerprint = getFingerprint(store); diff --git a/src/libfetchers/github.cc b/src/libfetchers/github.cc index 15a19021d..3b723d7d8 100644 --- a/src/libfetchers/github.cc +++ b/src/libfetchers/github.cc @@ -398,9 +398,8 @@ struct GitHubInputScheme : GitArchiveInputScheme Headers headers = makeHeadersWithAuthTokens(*input.settings, host, input); - auto accessor = store->getFSAccessor(); auto downloadResult = downloadFile(store, *input.settings, url, "source", headers); - auto json = nlohmann::json::parse(accessor->readFile(CanonPath(downloadResult.storePath.to_string()))); + auto json = nlohmann::json::parse(store->getFSAccessor(downloadResult.storePath)->readFile(CanonPath::root)); return RefInfo{ .rev = Hash::parseAny(std::string{json["sha"]}, HashAlgorithm::SHA1), @@ -473,9 +472,8 @@ struct GitLabInputScheme : GitArchiveInputScheme Headers headers = makeHeadersWithAuthTokens(*input.settings, host, input); - auto accessor = store->getFSAccessor(); auto downloadResult = downloadFile(store, *input.settings, url, "source", headers); - auto json = nlohmann::json::parse(accessor->readFile(CanonPath(downloadResult.storePath.to_string()))); + auto json = nlohmann::json::parse(store->getFSAccessor(downloadResult.storePath)->readFile(CanonPath::root)); if (json.is_array() && json.size() >= 1 && json[0]["id"] != nullptr) { return RefInfo{.rev = Hash::parseAny(std::string(json[0]["id"]), HashAlgorithm::SHA1)}; diff --git a/src/libfetchers/include/nix/fetchers/meson.build b/src/libfetchers/include/nix/fetchers/meson.build index fcd446a6d..a313b1e0b 100644 --- a/src/libfetchers/include/nix/fetchers/meson.build +++ b/src/libfetchers/include/nix/fetchers/meson.build @@ -11,6 +11,5 @@ headers = files( 'git-utils.hh', 'input-cache.hh', 'registry.hh', - 'store-path-accessor.hh', 'tarball.hh', ) diff --git a/src/libfetchers/include/nix/fetchers/store-path-accessor.hh b/src/libfetchers/include/nix/fetchers/store-path-accessor.hh deleted file mode 100644 index a107293f8..000000000 --- a/src/libfetchers/include/nix/fetchers/store-path-accessor.hh +++ /dev/null @@ -1,14 +0,0 @@ -#pragma once - -#include "nix/util/source-path.hh" - -namespace nix { - -class StorePath; -class Store; - -ref makeStorePathAccessor(ref store, const StorePath & storePath); - -SourcePath getUnfilteredRootPath(CanonPath path); - -} // namespace nix diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index 641b3d6a8..bf460d9c6 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -6,7 +6,6 @@ #include "nix/util/tarfile.hh" #include "nix/store/store-api.hh" #include "nix/util/url-parts.hh" -#include "nix/fetchers/store-path-accessor.hh" #include "nix/fetchers/fetch-settings.hh" #include @@ -331,7 +330,8 @@ struct MercurialInputScheme : InputScheme auto storePath = fetchToStore(store, input); - auto accessor = makeStorePathAccessor(store, storePath); + // We just added it, it should be there. + auto accessor = ref{store->getFSAccessor(storePath)}; accessor->setPathDisplay("«" + input.to_string() + "»"); diff --git a/src/libfetchers/meson.build b/src/libfetchers/meson.build index 070c82b8c..5b53a147b 100644 --- a/src/libfetchers/meson.build +++ b/src/libfetchers/meson.build @@ -50,7 +50,6 @@ sources = files( 'mercurial.cc', 'path.cc', 'registry.cc', - 'store-path-accessor.cc', 'tarball.cc', ) diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index b66459fb9..3c4b9c06d 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -1,7 +1,6 @@ #include "nix/fetchers/fetchers.hh" #include "nix/store/store-api.hh" #include "nix/util/archive.hh" -#include "nix/fetchers/store-path-accessor.hh" #include "nix/fetchers/cache.hh" #include "nix/fetchers/fetch-to-store.hh" #include "nix/fetchers/fetch-settings.hh" @@ -153,7 +152,7 @@ struct PathInputScheme : InputScheme if (!input.getLastModified()) input.attrs.insert_or_assign("lastModified", uint64_t(mtime)); - return {makeStorePathAccessor(store, *storePath), std::move(input)}; + return {ref{store->getFSAccessor(*storePath)}, std::move(input)}; } std::optional getFingerprint(ref store, const Input & input) const override diff --git a/src/libfetchers/store-path-accessor.cc b/src/libfetchers/store-path-accessor.cc deleted file mode 100644 index 65160e311..000000000 --- a/src/libfetchers/store-path-accessor.cc +++ /dev/null @@ -1,11 +0,0 @@ -#include "nix/fetchers/store-path-accessor.hh" -#include "nix/store/store-api.hh" - -namespace nix { - -ref makeStorePathAccessor(ref store, const StorePath & storePath) -{ - return projectSubdirSourceAccessor(store->getFSAccessor(), storePath.to_string()); -} - -} // namespace nix diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index b55837c9e..31d5ab460 100644 --- a/src/libfetchers/tarball.cc +++ b/src/libfetchers/tarball.cc @@ -6,7 +6,6 @@ #include "nix/util/archive.hh" #include "nix/util/tarfile.hh" #include "nix/util/types.hh" -#include "nix/fetchers/store-path-accessor.hh" #include "nix/store/store-api.hh" #include "nix/fetchers/git-utils.hh" #include "nix/fetchers/fetch-settings.hh" @@ -354,7 +353,7 @@ struct FileInputScheme : CurlInputScheme auto narHash = store->queryPathInfo(file.storePath)->narHash; input.attrs.insert_or_assign("narHash", narHash.to_string(HashFormat::SRI, true)); - auto accessor = makeStorePathAccessor(store, file.storePath); + auto accessor = ref{store->getFSAccessor(file.storePath)}; accessor->setPathDisplay("«" + input.to_string() + "»"); diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index d5184b1bf..badfb4b14 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -539,11 +539,21 @@ void BinaryCacheStore::registerDrvOutput(const Realisation & info) upsertFile(filePath, static_cast(info).dump(), "application/json"); } -ref BinaryCacheStore::getFSAccessor(bool requireValidPath) +ref BinaryCacheStore::getRemoteFSAccessor(bool requireValidPath) { return make_ref(ref(shared_from_this()), requireValidPath, config.localNarCache); } +ref BinaryCacheStore::getFSAccessor(bool requireValidPath) +{ + return getRemoteFSAccessor(requireValidPath); +} + +std::shared_ptr BinaryCacheStore::getFSAccessor(const StorePath & storePath, bool requireValidPath) +{ + return getRemoteFSAccessor(requireValidPath)->accessObject(storePath); +} + void BinaryCacheStore::addSignatures(const StorePath & storePath, const StringSet & sigs) { /* Note: this is inherently racy since there is no locking on diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index 367cdb5d2..4b485ca66 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -276,7 +276,14 @@ struct DummyStore : virtual Store callback(nullptr); } - virtual ref getFSAccessor(bool requireValidPath) override + std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath) override + { + std::shared_ptr res; + contents.cvisit(path, [&](const auto & kv) { res = kv.second.contents.get_ptr(); }); + return res; + } + + ref getFSAccessor(bool requireValidPath) override { return wholeStoreView; } diff --git a/src/libstore/include/nix/store/binary-cache-store.hh b/src/libstore/include/nix/store/binary-cache-store.hh index 908500b42..c316b1199 100644 --- a/src/libstore/include/nix/store/binary-cache-store.hh +++ b/src/libstore/include/nix/store/binary-cache-store.hh @@ -12,6 +12,7 @@ namespace nix { struct NarInfo; +class RemoteFSAccessor; struct BinaryCacheStoreConfig : virtual StoreConfig { @@ -136,6 +137,11 @@ private: CheckSigsFlag checkSigs, std::function mkInfo); + /** + * Same as `getFSAccessor`, but with a more preceise return type. + */ + ref getRemoteFSAccessor(bool requireValidPath = true); + public: bool isValidPathUncached(const StorePath & path) override; @@ -175,6 +181,8 @@ public: ref getFSAccessor(bool requireValidPath = true) override; + std::shared_ptr getFSAccessor(const StorePath &, bool requireValidPath = true) override; + void addSignatures(const StorePath & storePath, const StringSet & sigs) override; std::optional getBuildLogExact(const StorePath & path) override; diff --git a/src/libstore/include/nix/store/legacy-ssh-store.hh b/src/libstore/include/nix/store/legacy-ssh-store.hh index ac31506d0..75751e2d1 100644 --- a/src/libstore/include/nix/store/legacy-ssh-store.hh +++ b/src/libstore/include/nix/store/legacy-ssh-store.hh @@ -142,7 +142,12 @@ public: unsupported("ensurePath"); } - virtual ref getFSAccessor(bool requireValidPath) override + ref getFSAccessor(bool requireValidPath) override + { + unsupported("getFSAccessor"); + } + + std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath) override { unsupported("getFSAccessor"); } diff --git a/src/libstore/include/nix/store/local-fs-store.hh b/src/libstore/include/nix/store/local-fs-store.hh index 84777f3d7..f7d6d65b1 100644 --- a/src/libstore/include/nix/store/local-fs-store.hh +++ b/src/libstore/include/nix/store/local-fs-store.hh @@ -68,6 +68,7 @@ struct LocalFSStore : virtual Store, virtual GcStore, virtual LogStore void narFromPath(const StorePath & path, Sink & sink) override; ref getFSAccessor(bool requireValidPath = true) override; + std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath = true) override; /** * Creates symlink from the `gcRoot` to the `storePath` and diff --git a/src/libstore/include/nix/store/remote-fs-accessor.hh b/src/libstore/include/nix/store/remote-fs-accessor.hh index fa0555d9b..9e1999cc0 100644 --- a/src/libstore/include/nix/store/remote-fs-accessor.hh +++ b/src/libstore/include/nix/store/remote-fs-accessor.hh @@ -27,6 +27,11 @@ class RemoteFSAccessor : public SourceAccessor public: + /** + * @return nullptr if the store does not contain any object at that path. + */ + std::shared_ptr accessObject(const StorePath & path); + RemoteFSAccessor( ref store, bool requireValidPath = true, const /* FIXME: use std::optional */ Path & cacheDir = ""); diff --git a/src/libstore/include/nix/store/remote-store.hh b/src/libstore/include/nix/store/remote-store.hh index 76591cf93..1aaf29d37 100644 --- a/src/libstore/include/nix/store/remote-store.hh +++ b/src/libstore/include/nix/store/remote-store.hh @@ -16,6 +16,7 @@ struct FdSink; struct FdSource; template class Pool; +class RemoteFSAccessor; struct RemoteStoreConfig : virtual StoreConfig { @@ -176,10 +177,18 @@ protected: virtual ref getFSAccessor(bool requireValidPath = true) override; + virtual std::shared_ptr + getFSAccessor(const StorePath & path, bool requireValidPath = true) override; + virtual void narFromPath(const StorePath & path, Sink & sink) override; private: + /** + * Same as the default implemenation of `RemoteStore::getFSAccessor`, but with a more preceise return type. + */ + ref getRemoteFSAccessor(bool requireValidPath = true); + std::atomic_bool failed{false}; void copyDrvsFromEvalStore(const std::vector & paths, std::shared_ptr evalStore); diff --git a/src/libstore/include/nix/store/store-api.hh b/src/libstore/include/nix/store/store-api.hh index 2519002b3..6d3f6b8d0 100644 --- a/src/libstore/include/nix/store/store-api.hh +++ b/src/libstore/include/nix/store/store-api.hh @@ -717,10 +717,20 @@ public: }; /** - * @return An object to access files in the Nix store. + * @return An object to access files in the Nix store, across all + * store objects. */ virtual ref getFSAccessor(bool requireValidPath = true) = 0; + /** + * @return An object to access files for a specific store object in + * the Nix store. + * + * @return nullptr if the store doesn't contain an object at the + * givine path. + */ + virtual std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath = true) = 0; + /** * Repair the contents of the given path by redownloading it using * a substituter (if available). diff --git a/src/libstore/include/nix/store/uds-remote-store.hh b/src/libstore/include/nix/store/uds-remote-store.hh index 37c239796..fe6e486f4 100644 --- a/src/libstore/include/nix/store/uds-remote-store.hh +++ b/src/libstore/include/nix/store/uds-remote-store.hh @@ -61,6 +61,11 @@ struct UDSRemoteStore : virtual IndirectRootStore, virtual RemoteStore return LocalFSStore::getFSAccessor(requireValidPath); } + std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath = true) override + { + return LocalFSStore::getFSAccessor(path, requireValidPath); + } + void narFromPath(const StorePath & path, Sink & sink) override { LocalFSStore::narFromPath(path, sink); diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index e0f07b91b..66ae85d89 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -91,6 +91,23 @@ ref LocalFSStore::getFSAccessor(bool requireValidPath) ref(std::dynamic_pointer_cast(shared_from_this())), requireValidPath); } +std::shared_ptr LocalFSStore::getFSAccessor(const StorePath & path, bool requireValidPath) +{ + auto absPath = std::filesystem::path{config.realStoreDir.get()} / path.to_string(); + if (requireValidPath) { + /* Only return non-null if the store object is a fully-valid + member of the store. */ + if (!isValidPath(path)) + return nullptr; + } else { + /* Return non-null as long as the some file system data exists, + even if the store object is not fully registered. */ + if (!pathExists(absPath)) + return nullptr; + } + return std::make_shared(std::move(absPath)); +} + void LocalFSStore::narFromPath(const StorePath & path, Sink & sink) { if (!isValidPath(path)) diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 12c810eca..e6715cbdf 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -51,15 +51,17 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std: std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPath & path) { - auto [storePath, restPath_] = store->toStorePath(store->storeDir + path.abs()); - auto restPath = CanonPath(restPath_); - + auto [storePath, restPath] = store->toStorePath(store->storeDir + path.abs()); if (requireValidPath && !store->isValidPath(storePath)) throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); + return {ref{accessObject(storePath)}, CanonPath{restPath}}; +} +std::shared_ptr RemoteFSAccessor::accessObject(const StorePath & storePath) +{ auto i = nars.find(std::string(storePath.hashPart())); if (i != nars.end()) - return {i->second, restPath}; + return i->second; std::string listing; Path cacheFile; @@ -90,7 +92,7 @@ std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPat }); nars.emplace(storePath.hashPart(), narAccessor); - return {narAccessor, restPath}; + return narAccessor; } catch (SystemError &) { } @@ -98,14 +100,14 @@ std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPat try { auto narAccessor = makeNarAccessor(nix::readFile(cacheFile)); nars.emplace(storePath.hashPart(), narAccessor); - return {narAccessor, restPath}; + return narAccessor; } catch (SystemError &) { } } StringSink sink; store->narFromPath(storePath, sink); - return {addToCache(storePath.hashPart(), std::move(sink.s)), restPath}; + return addToCache(storePath.hashPart(), std::move(sink.s)); } std::optional RemoteFSAccessor::maybeLstat(const CanonPath & path) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index b918871fa..bb7425081 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -794,9 +794,19 @@ void RemoteStore::narFromPath(const StorePath & path, Sink & sink) conn->narFromPath(*this, &conn.daemonException, path, [&](Source & source) { copyNAR(conn->from, sink); }); } +ref RemoteStore::getRemoteFSAccessor(bool requireValidPath) +{ + return make_ref(ref(shared_from_this()), requireValidPath); +} + ref RemoteStore::getFSAccessor(bool requireValidPath) { - return make_ref(ref(shared_from_this())); + return getRemoteFSAccessor(requireValidPath); +} + +std::shared_ptr RemoteStore::getFSAccessor(const StorePath & path, bool requireValidPath) +{ + return getRemoteFSAccessor(requireValidPath)->accessObject(path); } void RemoteStore::ConnectionHandle::withFramedSink(std::function fun) diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index dafe14fea..a7e28017f 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -151,6 +151,11 @@ struct MountedSSHStore : virtual SSHStore, virtual LocalFSStore return LocalFSStore::getFSAccessor(requireValidPath); } + std::shared_ptr getFSAccessor(const StorePath & path, bool requireValidPath) override + { + return LocalFSStore::getFSAccessor(path, requireValidPath); + } + std::optional getBuildLogExact(const StorePath & path) override { return LocalFSStore::getBuildLogExact(path); diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 17748ec53..a0b06db54 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1120,10 +1120,9 @@ Derivation Store::derivationFromPath(const StorePath & drvPath) static Derivation readDerivationCommon(Store & store, const StorePath & drvPath, bool requireValidPath) { - auto accessor = store.getFSAccessor(requireValidPath); + auto accessor = store.getFSAccessor(drvPath, requireValidPath); try { - return parseDerivation( - store, accessor->readFile(CanonPath(drvPath.to_string())), Derivation::nameFromPath(drvPath)); + return parseDerivation(store, accessor->readFile(CanonPath::root), Derivation::nameFromPath(drvPath)); } catch (FormatError & e) { throw Error("error parsing derivation '%s': %s", store.printStorePath(drvPath), e.msg()); } diff --git a/src/nix/cat.cc b/src/nix/cat.cc index 276e01f5d..145336723 100644 --- a/src/nix/cat.cc +++ b/src/nix/cat.cc @@ -41,7 +41,10 @@ struct CmdCatStore : StoreCommand, MixCat void run(ref store) override { auto [storePath, rest] = store->toStorePath(path); - cat(store->getFSAccessor(), CanonPath{storePath.to_string()} / CanonPath{rest}); + auto accessor = store->getFSAccessor(storePath); + if (!accessor) + throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); + cat(ref{std::move(accessor)}, CanonPath{rest}); } }; diff --git a/src/nix/ls.cc b/src/nix/ls.cc index dcc46fa14..4952d5243 100644 --- a/src/nix/ls.cc +++ b/src/nix/ls.cc @@ -115,7 +115,10 @@ struct CmdLsStore : StoreCommand, MixLs void run(ref store) override { auto [storePath, rest] = store->toStorePath(path); - list(store->getFSAccessor(), CanonPath{storePath.to_string()} / CanonPath{rest}); + auto accessor = store->getFSAccessor(storePath); + if (!accessor) + throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); + list(ref{std::move(accessor)}, CanonPath{rest}); } }; diff --git a/src/nix/nix-store/nix-store.cc b/src/nix/nix-store/nix-store.cc index 5f85e06f0..f8078426c 100644 --- a/src/nix/nix-store/nix-store.cc +++ b/src/nix/nix-store/nix-store.cc @@ -603,7 +603,7 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise) #endif if (!hashGiven) { HashResult hash = hashPath( - {store->getFSAccessor(false), CanonPath{info->path.to_string()}}, + {ref{store->getFSAccessor(info->path, false)}}, FileSerialisationMethod::NixArchive, HashAlgorithm::SHA256); info->narHash = hash.hash;