From 7b30293d389ea75ee24ec1f2a4a4187f175757ab Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Jul 2023 15:50:25 -0400 Subject: [PATCH 1/3] Tighten `#include`s: `DerivedPath` doesn't care about `Realisation` --- src/libcmd/built-path.hh | 1 + src/libstore/derived-path.hh | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcmd/built-path.hh b/src/libcmd/built-path.hh index c563a46e9..744e8090b 100644 --- a/src/libcmd/built-path.hh +++ b/src/libcmd/built-path.hh @@ -1,4 +1,5 @@ #include "derived-path.hh" +#include "realisation.hh" namespace nix { diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index 6ea80c92e..7a4261ce0 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -3,7 +3,6 @@ #include "util.hh" #include "path.hh" -#include "realisation.hh" #include "outputs-spec.hh" #include "comparator.hh" From f62543fe1cc544a5684af327f63a1aeb1bdeba94 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 20 Jul 2023 09:58:25 -0400 Subject: [PATCH 2/3] Remove unneeded copy It appeared in 8eb73a87245acf9d93dc401831b629981864fa58 (by me!) without justification. --- src/libstore/local-store.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e69460e6c..892ff2295 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1022,9 +1022,8 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path) std::map> -LocalStore::queryPartialDerivationOutputMap(const StorePath & path_) +LocalStore::queryPartialDerivationOutputMap(const StorePath & path) { - auto path = path_; auto outputs = retrySQLite>>([&]() { auto state(_state.lock()); std::map> outputs; From 6bc98c7fba9f783414692fcef41d90ed80928b6c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 19 Jul 2023 14:52:35 -0400 Subject: [PATCH 3/3] Give `queryPartialDerivationOutputMap` an `evalStore` parameter This makes it more useful. In general, the derivation will be in one store, and the realisation info is in another. This also helps us avoid duplication. See how `resolveDerivedPath` is now simpler because it uses `queryPartialDerivationOutputMap`. In #8369 we get more flavors of derived path, and need more code to resolve them all, and this problem only gets worse. The fact that we need a new method to deal with the multiple dispatch is unfortunate, but this generally relates to the fact that `Store` is a sub-par interface, too bulky/unwieldy and conflating separate concerns. Solving that is out of scope of this PR. This is part of the RFC 92 work. See tracking issue #6316 --- src/libstore/build/local-derivation-goal.cc | 6 ++- src/libstore/local-store.cc | 19 +------- src/libstore/local-store.hh | 2 +- src/libstore/misc.cc | 53 +++++++++------------ src/libstore/realisation.hh | 9 +++- src/libstore/remote-store.cc | 33 ++++++++----- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 34 +++++++++++-- src/libstore/store-api.hh | 15 +++++- 9 files changed, 103 insertions(+), 70 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2935b9da9..0d982bbff 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1251,11 +1251,13 @@ struct RestrictedStore : public virtual RestrictedStoreConfig, public virtual Lo void queryReferrers(const StorePath & path, StorePathSet & referrers) override { } - std::map> queryPartialDerivationOutputMap(const StorePath & path) override + std::map> queryPartialDerivationOutputMap( + const StorePath & path, + Store * evalStore = nullptr) override { if (!goal.isAllowed(path)) throw InvalidPath("cannot query output map for unknown path '%s' in recursive Nix", printStorePath(path)); - return next->queryPartialDerivationOutputMap(path); + return next->queryPartialDerivationOutputMap(path, evalStore); } std::optional queryPathFromHashPart(const std::string & hashPart) override diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 892ff2295..010e52019 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1022,9 +1022,9 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path) std::map> -LocalStore::queryPartialDerivationOutputMap(const StorePath & path) +LocalStore::queryStaticPartialDerivationOutputMap(const StorePath & path) { - auto outputs = retrySQLite>>([&]() { + return retrySQLite>>([&]() { auto state(_state.lock()); std::map> outputs; uint64_t drvId; @@ -1036,21 +1036,6 @@ LocalStore::queryPartialDerivationOutputMap(const StorePath & path) return outputs; }); - - if (!experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) - return outputs; - - auto drv = readInvalidDerivation(path); - auto drvHashes = staticOutputHashes(*this, drv); - for (auto& [outputName, hash] : drvHashes) { - auto realisation = queryRealisation(DrvOutput{hash, outputName}); - if (realisation) - outputs.insert_or_assign(outputName, realisation->outPath); - else - outputs.insert({outputName, std::nullopt}); - } - - return outputs; } std::optional LocalStore::queryPathFromHashPart(const std::string & hashPart) diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 8a3b0b43f..d20e9cc4f 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -165,7 +165,7 @@ public: StorePathSet queryValidDerivers(const StorePath & path) override; - std::map> queryPartialDerivationOutputMap(const StorePath & path) override; + std::map> queryStaticPartialDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 50336c779..14160dc8b 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -310,43 +310,34 @@ std::map drvOutputReferences( OutputPathMap resolveDerivedPath(Store & store, const DerivedPath::Built & bfd, Store * evalStore_) { - auto & evalStore = evalStore_ ? *evalStore_ : store; + auto outputsOpt_ = store.queryPartialDerivationOutputMap(bfd.drvPath, evalStore_); - OutputPathMap outputs; - auto drv = evalStore.readDerivation(bfd.drvPath); - auto outputHashes = staticOutputHashes(store, drv); - auto drvOutputs = drv.outputsAndOptPaths(store); - auto outputNames = std::visit(overloaded { + auto outputsOpt = std::visit(overloaded { [&](const OutputsSpec::All &) { - StringSet names; - for (auto & [outputName, _] : drv.outputs) - names.insert(outputName); - return names; + // Keep all outputs + return std::move(outputsOpt_); }, [&](const OutputsSpec::Names & names) { - return static_cast>(names); + // Get just those mentioned by name + std::map> outputsOpt; + for (auto & output : names) { + auto * pOutputPathOpt = get(outputsOpt_, output); + if (!pOutputPathOpt) + throw Error( + "the derivation '%s' doesn't have an output named '%s'", + store.printStorePath(bfd.drvPath), output); + outputsOpt.insert_or_assign(output, std::move(*pOutputPathOpt)); + } + return outputsOpt; }, }, bfd.outputs.raw()); - for (auto & output : outputNames) { - auto outputHash = get(outputHashes, output); - if (!outputHash) - throw Error( - "the derivation '%s' doesn't have an output named '%s'", - store.printStorePath(bfd.drvPath), output); - if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { - DrvOutput outputId { *outputHash, output }; - auto realisation = store.queryRealisation(outputId); - if (!realisation) - throw MissingRealisation(outputId); - outputs.insert_or_assign(output, realisation->outPath); - } else { - // If ca-derivations isn't enabled, assume that - // the output path is statically known. - auto drvOutput = get(drvOutputs, output); - assert(drvOutput); - assert(drvOutput->second); - outputs.insert_or_assign(output, *drvOutput->second); - } + + OutputPathMap outputs; + for (auto & [outputName, outputPathOpt] : outputsOpt) { + if (!outputPathOpt) + throw MissingRealisation(store.printStorePath(bfd.drvPath), outputName); + auto & outputPath = *outputPathOpt; + outputs.insert_or_assign(outputName, outputPath); } return outputs; } diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 2a093c128..0548b30c1 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -5,6 +5,7 @@ #include "hash.hh" #include "path.hh" +#include "derived-path.hh" #include #include "comparator.hh" #include "crypto.hh" @@ -143,9 +144,13 @@ class MissingRealisation : public Error { public: MissingRealisation(DrvOutput & outputId) - : Error( "cannot operate on an output of the " + : MissingRealisation(outputId.outputName, outputId.strHash()) + {} + MissingRealisation(std::string_view drv, std::string outputName) + : Error( "cannot operate on output '%s' of the " "unbuilt derivation '%s'", - outputId.to_string()) + outputName, + drv) {} }; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1e2104e1f..bfe2258a4 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -378,27 +378,36 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) } -std::map> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path) +std::map> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path, Store * evalStore_) { if (GET_PROTOCOL_MINOR(getProtocol()) >= 0x16) { - auto conn(getConnection()); - conn->to << WorkerProto::Op::QueryDerivationOutputMap << printStorePath(path); - conn.processStderr(); - return WorkerProto::Serialise>>::read(*this, *conn); + if (!evalStore_) { + auto conn(getConnection()); + conn->to << WorkerProto::Op::QueryDerivationOutputMap << printStorePath(path); + conn.processStderr(); + return WorkerProto::Serialise>>::read(*this, *conn); + } else { + auto & evalStore = *evalStore_; + auto outputs = evalStore.queryStaticPartialDerivationOutputMap(path); + // union with the first branch overriding the statically-known ones + // when non-`std::nullopt`. + for (auto && [outputName, optPath] : queryPartialDerivationOutputMap(path, nullptr)) { + if (optPath) + outputs.insert_or_assign(std::move(outputName), std::move(optPath)); + else + outputs.insert({std::move(outputName), std::nullopt}); + } + return outputs; + } } else { + auto & evalStore = evalStore_ ? *evalStore_ : *this; // Fallback for old daemon versions. // For floating-CA derivations (and their co-dependencies) this is an // under-approximation as it only returns the paths that can be inferred // from the derivation itself (and not the ones that are known because // the have been built), but as old stores don't handle floating-CA // derivations this shouldn't matter - auto derivation = readDerivation(path); - auto outputsWithOptPaths = derivation.outputsAndOptPaths(*this); - std::map> ret; - for (auto & [outputName, outputAndPath] : outputsWithOptPaths) { - ret.emplace(outputName, outputAndPath.second); - } - return ret; + return evalStore.queryStaticPartialDerivationOutputMap(path); } } diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index cb7a71acf..b12f5437f 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -63,7 +63,7 @@ public: StorePathSet queryDerivationOutputs(const StorePath & path) override; - std::map> queryPartialDerivationOutputMap(const StorePath & path) override; + std::map> queryPartialDerivationOutputMap(const StorePath & path, Store * evalStore = nullptr) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; StorePathSet querySubstitutablePaths(const StorePathSet & paths) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 5bee1af9f..a581edcc0 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -496,22 +496,50 @@ bool Store::PathInfoCacheValue::isKnownNow() return std::chrono::steady_clock::now() < time_point + ttl; } -std::map> Store::queryPartialDerivationOutputMap(const StorePath & path) +std::map> Store::queryStaticPartialDerivationOutputMap(const StorePath & path) { std::map> outputs; auto drv = readInvalidDerivation(path); - for (auto& [outputName, output] : drv.outputsAndOptPaths(*this)) { + for (auto & [outputName, output] : drv.outputsAndOptPaths(*this)) { outputs.emplace(outputName, output.second); } return outputs; } +std::map> Store::queryPartialDerivationOutputMap( + const StorePath & path, + Store * evalStore_) +{ + auto & evalStore = evalStore_ ? *evalStore_ : *this; + + auto outputs = evalStore.queryStaticPartialDerivationOutputMap(path); + + if (!experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) + return outputs; + + auto drv = evalStore.readInvalidDerivation(path); + auto drvHashes = staticOutputHashes(*this, drv); + for (auto & [outputName, hash] : drvHashes) { + auto realisation = queryRealisation(DrvOutput{hash, outputName}); + if (realisation) { + outputs.insert_or_assign(outputName, realisation->outPath); + } else { + // queryStaticPartialDerivationOutputMap is not guaranteed + // to return std::nullopt for outputs which are not + // statically known. + outputs.insert({outputName, std::nullopt}); + } + } + + return outputs; +} + OutputPathMap Store::queryDerivationOutputMap(const StorePath & path) { auto resp = queryPartialDerivationOutputMap(path); OutputPathMap result; for (auto & [outName, optOutPath] : resp) { if (!optOutPath) - throw Error("output '%s' of derivation '%s' has no store path mapped to it", outName, printStorePath(path)); + throw MissingRealisation(printStorePath(path), outName); result.insert_or_assign(outName, *optOutPath); } return result; diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 14a862eef..7b412d2dd 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -425,7 +425,20 @@ public: * derivation. All outputs are mentioned so ones mising the mapping * are mapped to `std::nullopt`. */ - virtual std::map> queryPartialDerivationOutputMap(const StorePath & path); + virtual std::map> queryPartialDerivationOutputMap( + const StorePath & path, + Store * evalStore = nullptr); + + /** + * Like `queryPartialDerivationOutputMap` but only considers + * statically known output paths (i.e. those that can be gotten from + * the derivation itself. + * + * Just a helper function for implementing + * `queryPartialDerivationOutputMap`. + */ + virtual std::map> queryStaticPartialDerivationOutputMap( + const StorePath & path); /** * Query the mapping outputName=>outputPath for the given derivation.