From 2c7557481b4c9d5113a65cc7a75c8acc18031f4e Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 24 Jul 2020 21:02:51 +0000 Subject: [PATCH 01/10] `queryDerivationOutputMap` no longer assumes all outputs have a mapping This assumption is broken by CA derivations. Making a PR now to do the breaking daemon change as soon as possible (if it is already too late, we can bump protocol intead). --- src/libstore/build.cc | 10 +++++-- src/libstore/daemon.cc | 4 +-- src/libstore/local-store.cc | 12 +++++--- src/libstore/local-store.hh | 2 +- src/libstore/remote-store.cc | 31 +++++++------------- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 13 ++++++++- src/libstore/store-api.hh | 10 +++++-- src/libstore/worker-protocol.hh | 52 ++++++++++++++++++++++++++++++++- src/nix-env/nix-env.cc | 2 +- 10 files changed, 102 insertions(+), 36 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 62294a08c..3a55f24cb 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2756,8 +2756,12 @@ struct RestrictedStore : public LocalFSStore void queryReferrers(const StorePath & path, StorePathSet & referrers) override { } - OutputPathMap queryDerivationOutputMap(const StorePath & path) override - { throw Error("queryDerivationOutputMap"); } + std::map> queryDerivationOutputMap(const StorePath & path) override + { + if (!goal.isAllowed(path)) + throw InvalidPath("cannot query output map for unknown path '%s' in recursive Nix", printStorePath(path)); + return next->queryDerivationOutputMap(path); + } std::optional queryPathFromHashPart(const std::string & hashPart) override { throw Error("queryPathFromHashPart"); } @@ -4918,7 +4922,7 @@ void Worker::waitForInput() std::vector buffer(4096); for (auto & k : fds2) { if (pollStatus.at(fdToPollStatus.at(k)).revents) { - ssize_t rd = read(k, buffer.data(), buffer.size()); + ssize_t rd = ::read(k, buffer.data(), buffer.size()); // FIXME: is there a cleaner way to handle pt close // than EIO? Is this even standard? if (rd == 0 || (rd == -1 && errno == EIO)) { diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 7e16529a5..82833f5c3 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -325,9 +325,9 @@ static void performOp(TunnelLogger * logger, ref store, case wopQueryDerivationOutputMap: { auto path = store->parseStorePath(readString(from)); logger->startWork(); - OutputPathMap outputs = store->queryDerivationOutputMap(path); + auto outputs = store->queryDerivationOutputMap(path); logger->stopWork(); - writeOutputPathMap(*store, to, outputs); + write(*store, to, outputs); break; } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 9ea71170f..4db4f7d6a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -774,17 +774,21 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path) } -OutputPathMap LocalStore::queryDerivationOutputMap(const StorePath & path) +std::map> LocalStore::queryDerivationOutputMap(const StorePath & path) { - return retrySQLite([&]() { + std::map> outputs; + BasicDerivation drv = readDerivation(path); + for (auto & [outName, _] : drv.outputs) { + outputs.insert_or_assign(outName, std::nullopt); + } + return retrySQLite>>([&]() { auto state(_state.lock()); auto useQueryDerivationOutputs(state->stmtQueryDerivationOutputs.use() (queryValidPathId(*state, path))); - OutputPathMap outputs; while (useQueryDerivationOutputs.next()) - outputs.emplace( + outputs.insert_or_assign( useQueryDerivationOutputs.getStr(0), parseStorePath(useQueryDerivationOutputs.getStr(1)) ); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 355c2814f..56ef1d251 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -133,7 +133,7 @@ public: StorePathSet queryValidDerivers(const StorePath & path) override; - OutputPathMap queryDerivationOutputMap(const StorePath & path) override; + std::map> queryDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 9af4364b7..3b3cdbc02 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -39,30 +39,21 @@ void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths out << store.printStorePath(i); } -std::map readOutputPathMap(const Store & store, Source & from) + +StorePath read(const Store & store, Source & from, Proxy _) { - std::map pathMap; - auto rawInput = readStrings(from); - if (rawInput.size() % 2) - throw Error("got an odd number of elements from the daemon when trying to read a output path map"); - auto curInput = rawInput.begin(); - while (curInput != rawInput.end()) { - auto thisKey = *curInput++; - auto thisValue = *curInput++; - pathMap.emplace(thisKey, store.parseStorePath(thisValue)); - } - return pathMap; + auto path = readString(from); + return store.parseStorePath(path); } -void writeOutputPathMap(const Store & store, Sink & out, const std::map & pathMap) + +void write(const Store & store, Sink & out, const StorePath & storePath) { - out << 2*pathMap.size(); - for (auto & i : pathMap) { - out << i.first; - out << store.printStorePath(i.second); - } + auto path = store.printStorePath(storePath); + out << path; } + /* TODO: Separate these store impls into different files, give them better names */ RemoteStore::RemoteStore(const Params & params) : Store(params) @@ -445,12 +436,12 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) } -OutputPathMap RemoteStore::queryDerivationOutputMap(const StorePath & path) +std::map> RemoteStore::queryDerivationOutputMap(const StorePath & path) { auto conn(getConnection()); conn->to << wopQueryDerivationOutputMap << printStorePath(path); conn.processStderr(); - return readOutputPathMap(*this, conn->from); + return read(*this, conn->from, Proxy>> {}); } diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 3c1b78b6a..b39a8de48 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -51,7 +51,7 @@ public: StorePathSet queryDerivationOutputs(const StorePath & path) override; - OutputPathMap queryDerivationOutputMap(const StorePath & path) override; + std::map> queryDerivationOutputMap(const StorePath & path) 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 ab21b0bd5..a624fafdd 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -330,9 +330,20 @@ bool Store::PathInfoCacheValue::isKnownNow() return std::chrono::steady_clock::now() < time_point + ttl; } +OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) { + auto resp = queryDerivationOutputMap(path); + OutputPathMap result; + for (auto & [outName, optOutPath] : resp) { + if (!optOutPath) + throw Error("output '%s' has no store path mapped to it", outName); + result.insert_or_assign(outName, *optOutPath); + } + return result; +} + StorePathSet Store::queryDerivationOutputs(const StorePath & path) { - auto outputMap = this->queryDerivationOutputMap(path); + auto outputMap = this->queryDerivationOutputMapAssumeTotal(path); StorePathSet outputPaths; for (auto & i: outputMap) { outputPaths.emplace(std::move(i.second)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index d1cb2035f..a83a2ff10 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -423,10 +423,16 @@ public: /* Query the outputs of the derivation denoted by `path'. */ virtual StorePathSet queryDerivationOutputs(const StorePath & path); - /* Query the mapping outputName=>outputPath for the given derivation */ - virtual OutputPathMap queryDerivationOutputMap(const StorePath & path) + /* Query the mapping outputName => outputPath for the given derivation. All + outputs are mentioned so ones mising the mapping are mapped to + `std::nullopt`. */ + virtual std::map> queryDerivationOutputMap(const StorePath & path) { unsupported("queryDerivationOutputMap"); } + /* Query the mapping outputName=>outputPath for the given derivation. + Assume every output has a mapping and throw an exception otherwise. */ + OutputPathMap queryDerivationOutputMapAssumeTotal(const StorePath & path); + /* Query the full store path given the hash part of a valid store path, or empty if the path doesn't exist. */ virtual std::optional queryPathFromHashPart(const std::string & hashPart) = 0; diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index 8b538f6da..6a6e29640 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -70,6 +70,56 @@ template T readStorePaths(const Store & store, Source & from); void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths); -void writeOutputPathMap(const Store & store, Sink & out, const OutputPathMap & paths); +/* To guide overloading */ +template +struct Proxy {}; + +template +std::map read(const Store & store, Source & from, Proxy> _) +{ + std::map resMap; + auto size = (size_t)readInt(from); + while (size--) { + auto thisKey = readString(from); + resMap.insert_or_assign(std::move(thisKey), read(store, from, Proxy {})); + } + return resMap; +} + +template +void write(const Store & store, Sink & out, const std::map & resMap) +{ + out << resMap.size(); + for (auto & i : resMap) { + out << i.first; + write(store, out, i.second); + } +} + +template +std::optional read(const Store & store, Source & from, Proxy> _) +{ + auto tag = readNum(from); + switch (tag) { + case 0: + return std::nullopt; + case 1: + return read(store, from, Proxy {}); + default: + throw Error("got an invalid tag bit for std::optional: %#04x", tag); + } +} + +template +void write(const Store & store, Sink & out, const std::optional & optVal) +{ + out << (optVal ? 1 : 0); + if (optVal) + write(store, out, *optVal); +} + +StorePath read(const Store & store, Source & from, Proxy _); + +void write(const Store & store, Sink & out, const StorePath & storePath); } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index ddd036070..d36804658 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -381,7 +381,7 @@ static void queryInstSources(EvalState & state, if (path.isDerivation()) { elem.setDrvPath(state.store->printStorePath(path)); - auto outputs = state.store->queryDerivationOutputMap(path); + auto outputs = state.store->queryDerivationOutputMapAssumeTotal(path); elem.setOutPath(state.store->printStorePath(outputs.at("out"))); if (name.size() >= drvExtension.size() && string(name, name.size() - drvExtension.size()) == drvExtension) From 45b6fdb22b05352108b6470ae16d611431c06666 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 4 Aug 2020 22:10:13 +0000 Subject: [PATCH 02/10] Remove unused functions --- src/libstore/remote-store.cc | 16 ---------------- src/libstore/worker-protocol.hh | 2 -- 2 files changed, 18 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1200ab200..dc2efefeb 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -63,22 +63,6 @@ void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & } } -std::map readOutputPathMap(const Store & store, Source & from) -{ - std::map pathMap; - auto rawInput = readStrings(from); - if (rawInput.size() % 2) - throw Error("got an odd number of elements from the daemon when trying to read a output path map"); - auto curInput = rawInput.begin(); - while (curInput != rawInput.end()) { - auto thisKey = *curInput++; - auto thisValue = *curInput++; - pathMap.emplace(thisKey, store.parseStorePath(thisValue)); - } - return pathMap; -} - - void write(const Store & store, Sink & out, const StorePath & storePath) { auto path = store.printStorePath(storePath); diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index ad5854c85..117d3e1a4 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -126,6 +126,4 @@ StorePathCAMap readStorePathCAMap(const Store & store, Source & from); void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & paths); -void writeOutputPathMap(const Store & store, Sink & out, const OutputPathMap & paths); - } From 1dfcbebc95f1c74763770bb2bd7afdebe8804845 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 4 Aug 2020 22:28:10 +0000 Subject: [PATCH 03/10] Organize and format code a bit --- src/libstore/remote-store.cc | 17 +++++++++-------- src/libstore/store-api.cc | 16 ++++++++-------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index dc2efefeb..772979066 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -31,7 +31,6 @@ template<> StorePathSet readStorePaths(const Store & store, Source & from) return paths; } - void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths) { out << paths.size(); @@ -39,11 +38,6 @@ void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths out << store.printStorePath(i); } -StorePath read(const Store & store, Source & from, Proxy _) -{ - auto path = readString(from); - return store.parseStorePath(path); -} StorePathCAMap readStorePathCAMap(const Store & store, Source & from) { @@ -63,10 +57,17 @@ void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & } } + +StorePath read(const Store & store, Source & from, Proxy _) +{ + auto path = readString(from); + return store.parseStorePath(path); +} + void write(const Store & store, Sink & out, const StorePath & storePath) { - auto path = store.printStorePath(storePath); - out << path; + auto path = store.printStorePath(storePath); + out << path; } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 4c68709ef..e894d2b85 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -362,14 +362,14 @@ bool Store::PathInfoCacheValue::isKnownNow() } OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) { - auto resp = queryDerivationOutputMap(path); - OutputPathMap result; - for (auto & [outName, optOutPath] : resp) { - if (!optOutPath) - throw Error("output '%s' has no store path mapped to it", outName); - result.insert_or_assign(outName, *optOutPath); - } - return result; + auto resp = queryDerivationOutputMap(path); + OutputPathMap result; + for (auto & [outName, optOutPath] : resp) { + if (!optOutPath) + throw Error("output '%s' has no store path mapped to it", outName); + result.insert_or_assign(outName, *optOutPath); + } + return result; } StorePathSet Store::queryDerivationOutputs(const StorePath & path) From 16c98bf57c52dee59c06e0e8911800943a468962 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Tue, 4 Aug 2020 22:36:31 +0000 Subject: [PATCH 04/10] Get rid of some unneeded temporaries --- src/libstore/remote-store.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 772979066..63c19a450 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -60,14 +60,12 @@ void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & StorePath read(const Store & store, Source & from, Proxy _) { - auto path = readString(from); - return store.parseStorePath(path); + return store.parseStorePath(readString(from)); } void write(const Store & store, Sink & out, const StorePath & storePath) { - auto path = store.printStorePath(storePath); - out << path; + out << store.printStorePath(storePath); } From ed96e603e116123c1e44f5108b67b472b2c96538 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 5 Aug 2020 19:44:08 +0000 Subject: [PATCH 05/10] Proxy -> Phantom to match Rust Sorry, Haskell. --- src/libstore/remote-store.cc | 4 ++-- src/libstore/worker-protocol.hh | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 377c81ff4..2c0857466 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -58,7 +58,7 @@ void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & } -StorePath read(const Store & store, Source & from, Proxy _) +StorePath read(const Store & store, Source & from, Phantom _) { return store.parseStorePath(readString(from)); } @@ -461,7 +461,7 @@ std::map> RemoteStore::queryDerivationOutp auto conn(getConnection()); conn->to << wopQueryDerivationOutputMap << printStorePath(path); conn.processStderr(); - return read(*this, conn->from, Proxy>> {}); + return read(*this, conn->from, Phantom>> {}); } diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index 117d3e1a4..384b81f08 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -72,16 +72,16 @@ void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths /* To guide overloading */ template -struct Proxy {}; +struct Phantom {}; template -std::map read(const Store & store, Source & from, Proxy> _) +std::map read(const Store & store, Source & from, Phantom> _) { std::map resMap; auto size = (size_t)readInt(from); while (size--) { auto thisKey = readString(from); - resMap.insert_or_assign(std::move(thisKey), read(store, from, Proxy {})); + resMap.insert_or_assign(std::move(thisKey), read(store, from, Phantom {})); } return resMap; } @@ -97,14 +97,14 @@ void write(const Store & store, Sink & out, const std::map & resMap) } template -std::optional read(const Store & store, Source & from, Proxy> _) +std::optional read(const Store & store, Source & from, Phantom> _) { auto tag = readNum(from); switch (tag) { case 0: return std::nullopt; case 1: - return read(store, from, Proxy {}); + return read(store, from, Phantom {}); default: throw Error("got an invalid tag bit for std::optional: %#04x", tag); } @@ -118,7 +118,7 @@ void write(const Store & store, Sink & out, const std::optional & optVal) write(store, out, *optVal); } -StorePath read(const Store & store, Source & from, Proxy _); +StorePath read(const Store & store, Source & from, Phantom _); void write(const Store & store, Sink & out, const StorePath & storePath); From 6c66331d5c34d97092a9b5e2832fed73e0da3c87 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 5 Aug 2020 20:37:48 +0000 Subject: [PATCH 06/10] WIP: Put the worker protocol `read` and `write` in a namespace to disambig --- src/libstore/remote-store.cc | 6 +++++- src/libstore/worker-protocol.hh | 15 +++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 2c0857466..96e0aabba 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -58,6 +58,8 @@ void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & } +namespace worker_proto { + StorePath read(const Store & store, Source & from, Phantom _) { return store.parseStorePath(readString(from)); @@ -68,6 +70,8 @@ void write(const Store & store, Sink & out, const StorePath & storePath) out << store.printStorePath(storePath); } +} + /* TODO: Separate these store impls into different files, give them better names */ RemoteStore::RemoteStore(const Params & params) @@ -461,7 +465,7 @@ std::map> RemoteStore::queryDerivationOutp auto conn(getConnection()); conn->to << wopQueryDerivationOutputMap << printStorePath(path); conn.processStderr(); - return read(*this, conn->from, Phantom>> {}); + return worker_proto::read(*this, conn->from, Phantom>> {}); } diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index 384b81f08..fdd700668 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -74,6 +74,10 @@ void writeStorePaths(const Store & store, Sink & out, const StorePathSet & paths template struct Phantom {}; + +namespace worker_proto { +/* FIXME maybe move more stuff inside here */ + template std::map read(const Store & store, Source & from, Phantom> _) { @@ -81,7 +85,7 @@ std::map read(const Store & store, Source & from, Phantom {})); + resMap.insert_or_assign(std::move(thisKey), nix::worker_proto::read(store, from, Phantom {})); } return resMap; } @@ -92,7 +96,7 @@ void write(const Store & store, Sink & out, const std::map & resMap) out << resMap.size(); for (auto & i : resMap) { out << i.first; - write(store, out, i.second); + nix::worker_proto::write(store, out, i.second); } } @@ -104,7 +108,7 @@ std::optional read(const Store & store, Source & from, Phantom {}); + return nix::worker_proto::read(store, from, Phantom {}); default: throw Error("got an invalid tag bit for std::optional: %#04x", tag); } @@ -115,13 +119,16 @@ void write(const Store & store, Sink & out, const std::optional & optVal) { out << (optVal ? 1 : 0); if (optVal) - write(store, out, *optVal); + nix::worker_proto::write(store, out, *optVal); } StorePath read(const Store & store, Source & from, Phantom _); void write(const Store & store, Sink & out, const StorePath & storePath); +} + + StorePathCAMap readStorePathCAMap(const Store & store, Source & from); void writeStorePathCAMap(const Store & store, Sink & out, const StorePathCAMap & paths); From 0739d428e07b59b7d44bdbf7354dafaae09dba7c Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Wed, 5 Aug 2020 17:49:45 -0400 Subject: [PATCH 07/10] Solve template deduction problem We had to predeclare our template functions --- src/libstore/daemon.cc | 2 +- src/libstore/worker-protocol.hh | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index e18f793c8..a420dcab0 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -327,7 +327,7 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); auto outputs = store->queryDerivationOutputMap(path); logger->stopWork(); - write(*store, to, outputs); + nix::worker_proto::write(*store, to, outputs); break; } diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index fdd700668..c50995d7c 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -78,6 +78,19 @@ struct Phantom {}; namespace worker_proto { /* FIXME maybe move more stuff inside here */ +StorePath read(const Store & store, Source & from, Phantom _); +void write(const Store & store, Sink & out, const StorePath & storePath); + +template +std::map read(const Store & store, Source & from, Phantom> _); +template +void write(const Store & store, Sink & out, const std::map & resMap); +template +std::optional read(const Store & store, Source & from, Phantom> _); +template +void write(const Store & store, Sink & out, const std::optional & optVal); + + template std::map read(const Store & store, Source & from, Phantom> _) { @@ -122,9 +135,6 @@ void write(const Store & store, Sink & out, const std::optional & optVal) nix::worker_proto::write(store, out, *optVal); } -StorePath read(const Store & store, Source & from, Phantom _); - -void write(const Store & store, Sink & out, const StorePath & storePath); } From 8b175f58d1a25dd904fb0ffdf5b2b9501983dba0 Mon Sep 17 00:00:00 2001 From: Carlo Nucera Date: Wed, 5 Aug 2020 17:57:07 -0400 Subject: [PATCH 08/10] Simplify the namespace --- src/libstore/daemon.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index a420dcab0..9503915eb 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -327,7 +327,7 @@ static void performOp(TunnelLogger * logger, ref store, logger->startWork(); auto outputs = store->queryDerivationOutputMap(path); logger->stopWork(); - nix::worker_proto::write(*store, to, outputs); + worker_proto::write(*store, to, outputs); break; } From 47644e49ca252441fcc3ae57f5a01e7fc579ba8c Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 7 Aug 2020 17:05:14 +0000 Subject: [PATCH 09/10] Specialize `std::optional` so this is backwards compatible While I am cautious to break parametricity, I think it's OK in this cases---we're not about to try to do some crazy polymorphic protocol anytime soon. --- src/libstore/remote-store.cc | 14 ++++++++++++++ src/libstore/worker-protocol.hh | 7 +++++++ 2 files changed, 21 insertions(+) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 96e0aabba..005415666 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -70,6 +70,20 @@ void write(const Store & store, Sink & out, const StorePath & storePath) out << store.printStorePath(storePath); } + +template<> +std::optional read(const Store & store, Source & from, Phantom> _) +{ + auto s = readString(from); + return s == "" ? std::optional {} : store.parseStorePath(s); +} + +template<> +void write(const Store & store, Sink & out, const std::optional & storePathOpt) +{ + out << (storePathOpt ? store.printStorePath(*storePathOpt) : ""); +} + } diff --git a/src/libstore/worker-protocol.hh b/src/libstore/worker-protocol.hh index c50995d7c..b3576fbeb 100644 --- a/src/libstore/worker-protocol.hh +++ b/src/libstore/worker-protocol.hh @@ -90,6 +90,13 @@ std::optional read(const Store & store, Source & from, Phantom void write(const Store & store, Sink & out, const std::optional & optVal); +/* Specialization which uses and empty string for the empty case, taking + advantage of the fact StorePaths always serialize to a non-empty string. + This is done primarily for backwards compatability, so that StorePath <= + std::optional, where <= is the compatability partial order. + */ +template<> +void write(const Store & store, Sink & out, const std::optional & optVal); template std::map read(const Store & store, Source & from, Phantom> _) From 9a9d834dc7bde0a4eafa2f7412e7a2f0df8c3262 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 20 Aug 2020 14:12:51 +0000 Subject: [PATCH 10/10] Rename drv output querying functions - `queryDerivationOutputMapAssumeTotal` -> `queryPartialDerivationOutputMap` - `queryDerivationOutputMapAssumeTotal` -> `queryDerivationOutputMap` --- src/libstore/build.cc | 4 ++-- src/libstore/daemon.cc | 2 +- src/libstore/local-store.cc | 2 +- src/libstore/local-store.hh | 2 +- src/libstore/remote-store.cc | 2 +- src/libstore/remote-store.hh | 2 +- src/libstore/store-api.cc | 6 +++--- src/libstore/store-api.hh | 6 +++--- src/nix-env/nix-env.cc | 2 +- 9 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index b47aeff3b..5592b32eb 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2756,11 +2756,11 @@ struct RestrictedStore : public LocalFSStore void queryReferrers(const StorePath & path, StorePathSet & referrers) override { } - std::map> queryDerivationOutputMap(const StorePath & path) override + std::map> queryPartialDerivationOutputMap(const StorePath & path) override { if (!goal.isAllowed(path)) throw InvalidPath("cannot query output map for unknown path '%s' in recursive Nix", printStorePath(path)); - return next->queryDerivationOutputMap(path); + return next->queryPartialDerivationOutputMap(path); } std::optional queryPathFromHashPart(const std::string & hashPart) override diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 9503915eb..0580101a2 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -325,7 +325,7 @@ static void performOp(TunnelLogger * logger, ref store, case wopQueryDerivationOutputMap: { auto path = store->parseStorePath(readString(from)); logger->startWork(); - auto outputs = store->queryDerivationOutputMap(path); + auto outputs = store->queryPartialDerivationOutputMap(path); logger->stopWork(); worker_proto::write(*store, to, outputs); break; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e96091aae..218b56861 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -782,7 +782,7 @@ StorePathSet LocalStore::queryValidDerivers(const StorePath & path) } -std::map> LocalStore::queryDerivationOutputMap(const StorePath & path) +std::map> LocalStore::queryPartialDerivationOutputMap(const StorePath & path) { std::map> outputs; BasicDerivation drv = readDerivation(path); diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 627b1f557..5af12c2b2 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -133,7 +133,7 @@ public: StorePathSet queryValidDerivers(const StorePath & path) override; - std::map> queryDerivationOutputMap(const StorePath & path) override; + std::map> queryPartialDerivationOutputMap(const StorePath & path) override; std::optional queryPathFromHashPart(const std::string & hashPart) override; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 005415666..3ee907d1a 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -474,7 +474,7 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path) } -std::map> RemoteStore::queryDerivationOutputMap(const StorePath & path) +std::map> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path) { auto conn(getConnection()); conn->to << wopQueryDerivationOutputMap << printStorePath(path); diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh index 4b093ad3b..b319e774b 100644 --- a/src/libstore/remote-store.hh +++ b/src/libstore/remote-store.hh @@ -51,7 +51,7 @@ public: StorePathSet queryDerivationOutputs(const StorePath & path) override; - std::map> queryDerivationOutputMap(const StorePath & path) override; + std::map> queryPartialDerivationOutputMap(const StorePath & path) 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 e66c04df4..7e016ac2e 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -357,8 +357,8 @@ bool Store::PathInfoCacheValue::isKnownNow() return std::chrono::steady_clock::now() < time_point + ttl; } -OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) { - auto resp = queryDerivationOutputMap(path); +OutputPathMap Store::queryDerivationOutputMap(const StorePath & path) { + auto resp = queryPartialDerivationOutputMap(path); OutputPathMap result; for (auto & [outName, optOutPath] : resp) { if (!optOutPath) @@ -370,7 +370,7 @@ OutputPathMap Store::queryDerivationOutputMapAssumeTotal(const StorePath & path) StorePathSet Store::queryDerivationOutputs(const StorePath & path) { - auto outputMap = this->queryDerivationOutputMapAssumeTotal(path); + auto outputMap = this->queryDerivationOutputMap(path); StorePathSet outputPaths; for (auto & i: outputMap) { outputPaths.emplace(std::move(i.second)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 9003ab541..68d66be7c 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -343,12 +343,12 @@ public: /* Query the mapping outputName => outputPath for the given derivation. All outputs are mentioned so ones mising the mapping are mapped to `std::nullopt`. */ - virtual std::map> queryDerivationOutputMap(const StorePath & path) - { unsupported("queryDerivationOutputMap"); } + virtual std::map> queryPartialDerivationOutputMap(const StorePath & path) + { unsupported("queryPartialDerivationOutputMap"); } /* Query the mapping outputName=>outputPath for the given derivation. Assume every output has a mapping and throw an exception otherwise. */ - OutputPathMap queryDerivationOutputMapAssumeTotal(const StorePath & path); + OutputPathMap queryDerivationOutputMap(const StorePath & path); /* Query the full store path given the hash part of a valid store path, or empty if the path doesn't exist. */ diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index d36804658..ddd036070 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -381,7 +381,7 @@ static void queryInstSources(EvalState & state, if (path.isDerivation()) { elem.setDrvPath(state.store->printStorePath(path)); - auto outputs = state.store->queryDerivationOutputMapAssumeTotal(path); + auto outputs = state.store->queryDerivationOutputMap(path); elem.setOutPath(state.store->printStorePath(outputs.at("out"))); if (name.size() >= drvExtension.size() && string(name, name.size() - drvExtension.size()) == drvExtension)