From 53a743b8c52fbef715d8c6830bb4f1b6667bff62 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 4 Jul 2025 15:39:47 +0200 Subject: [PATCH 1/4] queryMissing(): Return a struct ...instead of having a bunch of pass-by-reference arguments. --- src/libmain/shared.cc | 6 ++--- src/libstore/build/worker.cc | 4 +-- src/libstore/daemon.cc | 12 ++++----- .../include/nix/store/remote-store.hh | 4 +-- src/libstore/include/nix/store/store-api.hh | 16 +++++++++--- src/libstore/misc.cc | 26 ++++++++----------- src/libstore/remote-store.cc | 18 ++++++------- src/libstore/restricted-store.cc | 24 +++++++---------- src/libstore/store-api.cc | 9 +++---- src/nix-build/nix-build.cc | 7 ++--- src/nix-store/nix-store.cc | 12 +++------ 11 files changed, 59 insertions(+), 79 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index d9e8059f7..fa6b06682 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -46,10 +46,8 @@ void printGCWarning() void printMissing(ref store, const std::vector & paths, Verbosity lvl) { - uint64_t downloadSize, narSize; - StorePathSet willBuild, willSubstitute, unknown; - store->queryMissing(paths, willBuild, willSubstitute, unknown, downloadSize, narSize); - printMissing(store, willBuild, willSubstitute, unknown, downloadSize, narSize, lvl); + auto missing = store->queryMissing(paths); + printMissing(store, missing.willBuild, missing.willSubstitute, missing.unknown, missing.downloadSize, missing.narSize, lvl); } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index dd3692f41..bab31acf9 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -289,9 +289,7 @@ void Worker::run(const Goals & _topGoals) } /* Call queryMissing() to efficiently query substitutes. */ - StorePathSet willBuild, willSubstitute, unknown; - uint64_t downloadSize, narSize; - store.queryMissing(topPaths, willBuild, willSubstitute, unknown, downloadSize, narSize); + store.queryMissing(topPaths); debug("entered goal loop"); diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 4bca75228..b946ccbb5 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -949,14 +949,12 @@ static void performOp(TunnelLogger * logger, ref store, case WorkerProto::Op::QueryMissing: { auto targets = WorkerProto::Serialise::read(*store, rconn); logger->startWork(); - StorePathSet willBuild, willSubstitute, unknown; - uint64_t downloadSize, narSize; - store->queryMissing(targets, willBuild, willSubstitute, unknown, downloadSize, narSize); + auto missing = store->queryMissing(targets); logger->stopWork(); - WorkerProto::write(*store, wconn, willBuild); - WorkerProto::write(*store, wconn, willSubstitute); - WorkerProto::write(*store, wconn, unknown); - conn.to << downloadSize << narSize; + WorkerProto::write(*store, wconn, missing.willBuild); + WorkerProto::write(*store, wconn, missing.willSubstitute); + WorkerProto::write(*store, wconn, missing.unknown); + conn.to << missing.downloadSize << missing.narSize; break; } diff --git a/src/libstore/include/nix/store/remote-store.hh b/src/libstore/include/nix/store/remote-store.hh index dd2396fe3..18c02456f 100644 --- a/src/libstore/include/nix/store/remote-store.hh +++ b/src/libstore/include/nix/store/remote-store.hh @@ -149,9 +149,7 @@ struct RemoteStore : void addSignatures(const StorePath & storePath, const StringSet & sigs) override; - void queryMissing(const std::vector & targets, - StorePathSet & willBuild, StorePathSet & willSubstitute, StorePathSet & unknown, - uint64_t & downloadSize, uint64_t & narSize) override; + MissingPaths queryMissing(const std::vector & targets) override; void addBuildLog(const StorePath & drvPath, std::string_view log) override; diff --git a/src/libstore/include/nix/store/store-api.hh b/src/libstore/include/nix/store/store-api.hh index 1648b13c1..0933caa68 100644 --- a/src/libstore/include/nix/store/store-api.hh +++ b/src/libstore/include/nix/store/store-api.hh @@ -71,6 +71,18 @@ struct KeyedBuildResult; typedef std::map> StorePathCAMap; +/** + * Information about what paths will be built or substituted, returned + * by Store::queryMissing(). + */ +struct MissingPaths +{ + StorePathSet willBuild; + StorePathSet willSubstitute; + StorePathSet unknown; + uint64_t downloadSize{0}; + uint64_t narSize{0}; +}; /** * About the class hierarchy of the store types: @@ -694,9 +706,7 @@ public: * derivations that will be built, and the set of output paths that * will be substituted. */ - virtual void queryMissing(const std::vector & targets, - StorePathSet & willBuild, StorePathSet & willSubstitute, StorePathSet & unknown, - uint64_t & downloadSize, uint64_t & narSize); + virtual MissingPaths queryMissing(const std::vector & targets); /** * Sort a set of paths topologically under the references diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index dabae647f..7c97dbc57 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -98,23 +98,17 @@ const ContentAddress * getDerivationCA(const BasicDerivation & drv) return nullptr; } -void Store::queryMissing(const std::vector & targets, - StorePathSet & willBuild_, StorePathSet & willSubstitute_, StorePathSet & unknown_, - uint64_t & downloadSize_, uint64_t & narSize_) +MissingPaths Store::queryMissing(const std::vector & targets) { Activity act(*logger, lvlDebug, actUnknown, "querying info about missing paths"); - downloadSize_ = narSize_ = 0; - // FIXME: make async. ThreadPool pool(fileTransferSettings.httpConnections); struct State { std::unordered_set done; - StorePathSet & unknown, & willSubstitute, & willBuild; - uint64_t & downloadSize; - uint64_t & narSize; + MissingPaths res; }; struct DrvState @@ -125,7 +119,7 @@ void Store::queryMissing(const std::vector & targets, DrvState(size_t left) : left(left) { } }; - Sync state_(State{{}, unknown_, willSubstitute_, willBuild_, downloadSize_, narSize_}); + Sync state_; std::function doPath; @@ -143,7 +137,7 @@ void Store::queryMissing(const std::vector & targets, auto mustBuildDrv = [&](const StorePath & drvPath, const Derivation & drv) { { auto state(state_.lock()); - state->willBuild.insert(drvPath); + state->res.willBuild.insert(drvPath); } for (const auto & [inputDrv, inputNode] : drv.inputDrvs.map) { @@ -203,7 +197,7 @@ void Store::queryMissing(const std::vector & targets, if (!isValidPath(drvPath)) { // FIXME: we could try to substitute the derivation. auto state(state_.lock()); - state->unknown.insert(drvPath); + state->res.unknown.insert(drvPath); return; } @@ -282,7 +276,7 @@ void Store::queryMissing(const std::vector & targets, if (infos.empty()) { auto state(state_.lock()); - state->unknown.insert(bo.path); + state->res.unknown.insert(bo.path); return; } @@ -291,9 +285,9 @@ void Store::queryMissing(const std::vector & targets, { auto state(state_.lock()); - state->willSubstitute.insert(bo.path); - state->downloadSize += info->second.downloadSize; - state->narSize += info->second.narSize; + state->res.willSubstitute.insert(bo.path); + state->res.downloadSize += info->second.downloadSize; + state->res.narSize += info->second.narSize; } for (auto & ref : info->second.references) @@ -306,6 +300,8 @@ void Store::queryMissing(const std::vector & targets, pool.enqueue(std::bind(doPath, path)); pool.process(); + + return std::move(state_.lock()->res); } diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 3151f319c..1b8bad048 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -855,9 +855,7 @@ void RemoteStore::addSignatures(const StorePath & storePath, const StringSet & s } -void RemoteStore::queryMissing(const std::vector & targets, - StorePathSet & willBuild, StorePathSet & willSubstitute, StorePathSet & unknown, - uint64_t & downloadSize, uint64_t & narSize) +MissingPaths RemoteStore::queryMissing(const std::vector & targets) { { auto conn(getConnection()); @@ -868,16 +866,16 @@ void RemoteStore::queryMissing(const std::vector & targets, conn->to << WorkerProto::Op::QueryMissing; WorkerProto::write(*this, *conn, targets); conn.processStderr(); - willBuild = WorkerProto::Serialise::read(*this, *conn); - willSubstitute = WorkerProto::Serialise::read(*this, *conn); - unknown = WorkerProto::Serialise::read(*this, *conn); - conn->from >> downloadSize >> narSize; - return; + MissingPaths res; + res.willBuild = WorkerProto::Serialise::read(*this, *conn); + res.willSubstitute = WorkerProto::Serialise::read(*this, *conn); + res.unknown = WorkerProto::Serialise::read(*this, *conn); + conn->from >> res.downloadSize >> res.narSize; + return res; } fallback: - return Store::queryMissing(targets, willBuild, willSubstitute, - unknown, downloadSize, narSize); + return Store::queryMissing(targets); } diff --git a/src/libstore/restricted-store.cc b/src/libstore/restricted-store.cc index 0485f5584..69435122a 100644 --- a/src/libstore/restricted-store.cc +++ b/src/libstore/restricted-store.cc @@ -143,13 +143,7 @@ struct RestrictedStore : public virtual IndirectRootStore, public virtual GcStor unsupported("addSignatures"); } - void queryMissing( - const std::vector & targets, - StorePathSet & willBuild, - StorePathSet & willSubstitute, - StorePathSet & unknown, - uint64_t & downloadSize, - uint64_t & narSize) override; + MissingPaths queryMissing(const std::vector & targets) override; virtual std::optional getBuildLogExact(const StorePath & path) override { @@ -306,19 +300,14 @@ std::vector RestrictedStore::buildPathsWithResults( return results; } -void RestrictedStore::queryMissing( - const std::vector & targets, - StorePathSet & willBuild, - StorePathSet & willSubstitute, - StorePathSet & unknown, - uint64_t & downloadSize, - uint64_t & narSize) +MissingPaths RestrictedStore::queryMissing(const std::vector & targets) { /* This is slightly impure since it leaks information to the client about what paths will be built/substituted or are already present. Probably not a big deal. */ std::vector allowed; + StorePathSet unknown; for (auto & req : targets) { if (goal.isAllowed(req)) allowed.emplace_back(req); @@ -326,7 +315,12 @@ void RestrictedStore::queryMissing( unknown.insert(pathPartOfReq(req)); } - next->queryMissing(allowed, willBuild, willSubstitute, unknown, downloadSize, narSize); + auto res = next->queryMissing(allowed); + + for (auto & p : unknown) + res.unknown.insert(p); + + return res; } } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index e8988127e..730a22593 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -794,15 +794,12 @@ void Store::substitutePaths(const StorePathSet & paths) for (auto & path : paths) if (!path.isDerivation()) paths2.emplace_back(DerivedPath::Opaque{path}); - uint64_t downloadSize, narSize; - StorePathSet willBuild, willSubstitute, unknown; - queryMissing(paths2, - willBuild, willSubstitute, unknown, downloadSize, narSize); + auto missing = queryMissing(paths2); - if (!willSubstitute.empty()) + if (!missing.willSubstitute.empty()) try { std::vector subs; - for (auto & p : willSubstitute) subs.emplace_back(DerivedPath::Opaque{p}); + for (auto & p : missing.willSubstitute) subs.emplace_back(DerivedPath::Opaque{p}); buildPaths(subs); } catch (Error & e) { logWarning(e.info()); diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 3313c02aa..120fd4af6 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -422,13 +422,10 @@ static void main_nix_build(int argc, char * * argv) auto buildPaths = [&](const std::vector & paths) { /* Note: we do this even when !printMissing to efficiently fetch binary cache data. */ - uint64_t downloadSize, narSize; - StorePathSet willBuild, willSubstitute, unknown; - store->queryMissing(paths, - willBuild, willSubstitute, unknown, downloadSize, narSize); + auto missing = store->queryMissing(paths); if (settings.printMissing) - printMissing(ref(store), willBuild, willSubstitute, unknown, downloadSize, narSize); + printMissing(ref(store), missing.willBuild, missing.willSubstitute, missing.unknown, missing.downloadSize, missing.narSize); if (!dryRun) store->buildPaths(paths, buildMode, evalStore); diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 9acdf4554..ad921f227 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -146,23 +146,19 @@ static void opRealise(Strings opFlags, Strings opArgs) for (auto & i : opArgs) paths.push_back(followLinksToStorePathWithOutputs(*store, i)); - uint64_t downloadSize, narSize; - StorePathSet willBuild, willSubstitute, unknown; - store->queryMissing( - toDerivedPaths(paths), - willBuild, willSubstitute, unknown, downloadSize, narSize); + auto missing = store->queryMissing(toDerivedPaths(paths)); /* Filter out unknown paths from `paths`. */ if (ignoreUnknown) { std::vector paths2; for (auto & i : paths) - if (!unknown.count(i.path)) paths2.push_back(i); + if (!missing.unknown.count(i.path)) paths2.push_back(i); paths = std::move(paths2); - unknown = StorePathSet(); + missing.unknown = StorePathSet(); } if (settings.printMissing) - printMissing(ref(store), willBuild, willSubstitute, unknown, downloadSize, narSize); + printMissing(ref(store), missing.willBuild, missing.willSubstitute, missing.unknown, missing.downloadSize, missing.narSize); if (dryRun) return; From 3a636205c59415addef1be8d85662e2f82794005 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 4 Jul 2025 16:27:08 +0200 Subject: [PATCH 2/4] printMissing(): Take a MissingPaths argument --- src/libmain/include/nix/main/shared.hh | 8 +++--- src/libmain/shared.cc | 34 +++++++++++++------------- src/nix-build/nix-build.cc | 2 +- src/nix-store/nix-store.cc | 2 +- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/libmain/include/nix/main/shared.hh b/src/libmain/include/nix/main/shared.hh index 2ff57135b..4d4b816e7 100644 --- a/src/libmain/include/nix/main/shared.hh +++ b/src/libmain/include/nix/main/shared.hh @@ -35,15 +35,17 @@ void printVersion(const std::string & programName); void printGCWarning(); class Store; +struct MissingPaths; void printMissing( ref store, const std::vector & paths, Verbosity lvl = lvlInfo); -void printMissing(ref store, const StorePathSet & willBuild, - const StorePathSet & willSubstitute, const StorePathSet & unknown, - uint64_t downloadSize, uint64_t narSize, Verbosity lvl = lvlInfo); +void printMissing( + ref store, + const MissingPaths & missing, + Verbosity lvl = lvlInfo); std::string getArg(const std::string & opt, Strings::iterator & i, const Strings::iterator & end); diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index fa6b06682..1472345a4 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -46,41 +46,41 @@ void printGCWarning() void printMissing(ref store, const std::vector & paths, Verbosity lvl) { - auto missing = store->queryMissing(paths); - printMissing(store, missing.willBuild, missing.willSubstitute, missing.unknown, missing.downloadSize, missing.narSize, lvl); + printMissing(store, store->queryMissing(paths), lvl); } -void printMissing(ref store, const StorePathSet & willBuild, - const StorePathSet & willSubstitute, const StorePathSet & unknown, - uint64_t downloadSize, uint64_t narSize, Verbosity lvl) +void printMissing( + ref store, + const MissingPaths & missing, + Verbosity lvl) { - if (!willBuild.empty()) { - if (willBuild.size() == 1) + if (!missing.willBuild.empty()) { + if (missing.willBuild.size() == 1) printMsg(lvl, "this derivation will be built:"); else - printMsg(lvl, "these %d derivations will be built:", willBuild.size()); - auto sorted = store->topoSortPaths(willBuild); + printMsg(lvl, "these %d derivations will be built:", missing.willBuild.size()); + auto sorted = store->topoSortPaths(missing.willBuild); reverse(sorted.begin(), sorted.end()); for (auto & i : sorted) printMsg(lvl, " %s", store->printStorePath(i)); } - if (!willSubstitute.empty()) { - const float downloadSizeMiB = downloadSize / (1024.f * 1024.f); - const float narSizeMiB = narSize / (1024.f * 1024.f); - if (willSubstitute.size() == 1) { + if (!missing.willSubstitute.empty()) { + const float downloadSizeMiB = missing.downloadSize / (1024.f * 1024.f); + const float narSizeMiB = missing.narSize / (1024.f * 1024.f); + if (missing.willSubstitute.size() == 1) { printMsg(lvl, "this path will be fetched (%.2f MiB download, %.2f MiB unpacked):", downloadSizeMiB, narSizeMiB); } else { printMsg(lvl, "these %d paths will be fetched (%.2f MiB download, %.2f MiB unpacked):", - willSubstitute.size(), + missing.willSubstitute.size(), downloadSizeMiB, narSizeMiB); } std::vector willSubstituteSorted = {}; - std::for_each(willSubstitute.begin(), willSubstitute.end(), + std::for_each(missing.willSubstitute.begin(), missing.willSubstitute.end(), [&](const StorePath &p) { willSubstituteSorted.push_back(&p); }); std::sort(willSubstituteSorted.begin(), willSubstituteSorted.end(), [](const StorePath *lhs, const StorePath *rhs) { @@ -93,10 +93,10 @@ void printMissing(ref store, const StorePathSet & willBuild, printMsg(lvl, " %s", store->printStorePath(*p)); } - if (!unknown.empty()) { + if (!missing.unknown.empty()) { printMsg(lvl, "don't know how to build these paths%s:", (settings.readOnlyMode ? " (may be caused by read-only store access)" : "")); - for (auto & i : unknown) + for (auto & i : missing.unknown) printMsg(lvl, " %s", store->printStorePath(i)); } } diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 120fd4af6..98f12e3cd 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -425,7 +425,7 @@ static void main_nix_build(int argc, char * * argv) auto missing = store->queryMissing(paths); if (settings.printMissing) - printMissing(ref(store), missing.willBuild, missing.willSubstitute, missing.unknown, missing.downloadSize, missing.narSize); + printMissing(ref(store), missing); if (!dryRun) store->buildPaths(paths, buildMode, evalStore); diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index ad921f227..faa02a699 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -158,7 +158,7 @@ static void opRealise(Strings opFlags, Strings opArgs) } if (settings.printMissing) - printMissing(ref(store), missing.willBuild, missing.willSubstitute, missing.unknown, missing.downloadSize, missing.narSize); + printMissing(ref(store), missing); if (dryRun) return; From 1df17735f569932b1dd167fb3d14c3706b487eb8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 4 Jul 2025 16:32:37 +0200 Subject: [PATCH 3/4] nix-build: Drop unnecessary call to queryMissing() This is already done by Worker::run(). --- src/nix-build/nix-build.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 98f12e3cd..185188e83 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -420,12 +420,8 @@ static void main_nix_build(int argc, char * * argv) state->maybePrintStats(); auto buildPaths = [&](const std::vector & paths) { - /* Note: we do this even when !printMissing to efficiently - fetch binary cache data. */ - auto missing = store->queryMissing(paths); - if (settings.printMissing) - printMissing(ref(store), missing); + printMissing(ref(store), paths); if (!dryRun) store->buildPaths(paths, buildMode, evalStore); From 5c9592194c3824b8d1f9da1ddc4d6b1c099bbc89 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 4 Jul 2025 17:07:18 +0200 Subject: [PATCH 4/4] nix flake check: Skip substitutable derivations Since `nix flake check` doesn't produce a `result` symlink, it doesn't actually need to build/substitute derivations that are already known to have succeeded, i.e. that are substitutable. This can speed up CI jobs in cases where the derivations have already been built by other jobs. For instance, a command like nix flake check github:NixOS/hydra/aa62c7f7db31753f0cde690f8654dd1907fc0ce2 should no longer build anything because the outputs are already in cache.nixos.org. --- src/nix/flake.cc | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 35e96e493..444d5707b 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -833,8 +833,31 @@ struct CmdFlakeCheck : FlakeCommand if (build && !drvPaths.empty()) { Activity act(*logger, lvlInfo, actUnknown, fmt("running %d flake checks", drvPaths.size())); - store->buildPaths(drvPaths); + + auto missing = store->queryMissing(drvPaths); + + /* This command doesn't need to actually substitute + derivation outputs if they're missing but + substitutable. So filter out derivations that are + substitutable or already built. */ + std::vector toBuild; + for (auto & path : drvPaths) { + std::visit(overloaded { + [&](const DerivedPath::Built & bfd) { + auto drvPathP = std::get_if(&*bfd.drvPath); + if (!drvPathP || missing.willBuild.contains(drvPathP->path)) + toBuild.push_back(path); + }, + [&](const DerivedPath::Opaque & bo) { + if (!missing.willSubstitute.contains(bo.path)) + toBuild.push_back(path); + }, + }, path.raw()); + } + + store->buildPaths(toBuild); } + if (hasErrors) throw Error("some errors were encountered during the evaluation");