From 4372738efedb371bbc03fe9b63cb51d03debf052 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Fri, 26 May 2023 14:48:11 -0400 Subject: [PATCH] Systematize the worker protocol derived path serialiser It was some ad-hoc functions to account for versions, while the already factored-out serializer just supported the latest version. Now, we can fold that version-specific logic into the factored out one, and so we do. --- src/libstore/daemon.cc | 18 ++-------- src/libstore/remote-store.cc | 33 ++---------------- src/libstore/tests/worker-protocol.cc | 29 +++++++++++++-- src/libstore/worker-protocol.cc | 26 ++++++++++++-- .../worker-protocol/derived-path-1.29.bin | Bin 0 -> 184 bytes ...derived-path.bin => derived-path-1.30.bin} | Bin 6 files changed, 56 insertions(+), 50 deletions(-) create mode 100644 unit-test-data/libstore/worker-protocol/derived-path-1.29.bin rename unit-test-data/libstore/worker-protocol/{derived-path.bin => derived-path-1.30.bin} (100%) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index e117b507f..27c0b6431 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -261,18 +261,6 @@ struct ClientSettings } }; -static std::vector readDerivedPaths(Store & store, WorkerProto::Version clientVersion, WorkerProto::ReadConn conn) -{ - std::vector reqs; - if (GET_PROTOCOL_MINOR(clientVersion) >= 30) { - reqs = WorkerProto::Serialise>::read(store, conn); - } else { - for (auto & s : readStrings(conn.from)) - reqs.push_back(parsePathWithOutputs(store, s).toDerivedPath()); - } - return reqs; -} - static void performOp(TunnelLogger * logger, ref store, TrustedFlag trusted, RecursiveFlag recursive, WorkerProto::Version clientVersion, Source & from, BufferedSink & to, WorkerProto::Op op) @@ -538,7 +526,7 @@ static void performOp(TunnelLogger * logger, ref store, } case WorkerProto::Op::BuildPaths: { - auto drvs = readDerivedPaths(*store, clientVersion, rconn); + auto drvs = WorkerProto::Serialise::read(*store, rconn); BuildMode mode = bmNormal; if (GET_PROTOCOL_MINOR(clientVersion) >= 15) { mode = (BuildMode) readInt(from); @@ -563,7 +551,7 @@ static void performOp(TunnelLogger * logger, ref store, } case WorkerProto::Op::BuildPathsWithResults: { - auto drvs = readDerivedPaths(*store, clientVersion, rconn); + auto drvs = WorkerProto::Serialise::read(*store, rconn); BuildMode mode = bmNormal; mode = (BuildMode) readInt(from); @@ -938,7 +926,7 @@ static void performOp(TunnelLogger * logger, ref store, } case WorkerProto::Op::QueryMissing: { - auto targets = readDerivedPaths(*store, clientVersion, rconn); + auto targets = WorkerProto::Serialise::read(*store, rconn); logger->startWork(); StorePathSet willBuild, willSubstitute, unknown; uint64_t downloadSize, narSize; diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index 1704bfbb0..482c2ae01 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -655,33 +655,6 @@ void RemoteStore::queryRealisationUncached(const DrvOutput & id, } catch (...) { return callback.rethrow(); } } -static void writeDerivedPaths(RemoteStore & store, RemoteStore::Connection & conn, const std::vector & reqs) -{ - if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 30) { - WorkerProto::write(store, conn, reqs); - } else { - Strings ss; - for (auto & p : reqs) { - auto sOrDrvPath = StorePathWithOutputs::tryFromDerivedPath(p); - std::visit(overloaded { - [&](const StorePathWithOutputs & s) { - ss.push_back(s.to_string(store)); - }, - [&](const StorePath & drvPath) { - throw Error("trying to request '%s', but daemon protocol %d.%d is too old (< 1.29) to request a derivation file", - store.printStorePath(drvPath), - GET_PROTOCOL_MAJOR(conn.daemonVersion), - GET_PROTOCOL_MINOR(conn.daemonVersion)); - }, - [&](std::monostate) { - throw Error("wanted to build a derivation that is itself a build product, but the legacy 'ssh://' protocol doesn't support that. Try using 'ssh-ng://'"); - }, - }, sOrDrvPath); - } - conn.to << ss; - } -} - void RemoteStore::copyDrvsFromEvalStore( const std::vector & paths, std::shared_ptr evalStore) @@ -711,7 +684,7 @@ void RemoteStore::buildPaths(const std::vector & drvPaths, BuildMod auto conn(getConnection()); conn->to << WorkerProto::Op::BuildPaths; assert(GET_PROTOCOL_MINOR(conn->daemonVersion) >= 13); - writeDerivedPaths(*this, *conn, drvPaths); + WorkerProto::write(*this, *conn, drvPaths); if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 15) conn->to << buildMode; else @@ -735,7 +708,7 @@ std::vector RemoteStore::buildPathsWithResults( if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 34) { conn->to << WorkerProto::Op::BuildPathsWithResults; - writeDerivedPaths(*this, *conn, paths); + WorkerProto::write(*this, *conn, paths); conn->to << buildMode; conn.processStderr(); return WorkerProto::Serialise>::read(*this, *conn); @@ -929,7 +902,7 @@ void RemoteStore::queryMissing(const std::vector & targets, // to prevent a deadlock. goto fallback; conn->to << WorkerProto::Op::QueryMissing; - writeDerivedPaths(*this, *conn, targets); + WorkerProto::write(*this, *conn, targets); conn.processStderr(); willBuild = WorkerProto::Serialise::read(*this, *conn); willSubstitute = WorkerProto::Serialise::read(*this, *conn); diff --git a/src/libstore/tests/worker-protocol.cc b/src/libstore/tests/worker-protocol.cc index b10192cc1..b3e857bc4 100644 --- a/src/libstore/tests/worker-protocol.cc +++ b/src/libstore/tests/worker-protocol.cc @@ -69,9 +69,32 @@ VERSIONED_CHARACTERIZATION_TEST( VERSIONED_CHARACTERIZATION_TEST( WorkerProtoTest, - derivedPath, - "derived-path", - defaultVersion, + derivedPath_1_29, + "derived-path-1.29", + 1 << 8 | 29, + (std::tuple { + DerivedPath::Opaque { + .path = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" }, + }, + DerivedPath::Built { + .drvPath = makeConstantStorePathRef(StorePath { + "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar.drv", + }), + .outputs = OutputsSpec::All { }, + }, + DerivedPath::Built { + .drvPath = makeConstantStorePathRef(StorePath { + "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar.drv", + }), + .outputs = OutputsSpec::Names { "x", "y" }, + }, + })) + +VERSIONED_CHARACTERIZATION_TEST( + WorkerProtoTest, + derivedPath_1_30, + "derived-path-1.30", + 1 << 8 | 30, (std::tuple { DerivedPath::Opaque { .path = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" }, diff --git a/src/libstore/worker-protocol.cc b/src/libstore/worker-protocol.cc index 415e66f16..0f3f20ca5 100644 --- a/src/libstore/worker-protocol.cc +++ b/src/libstore/worker-protocol.cc @@ -51,12 +51,34 @@ void WorkerProto::Serialise>::write(const Store & sto DerivedPath WorkerProto::Serialise::read(const Store & store, WorkerProto::ReadConn conn) { auto s = readString(conn.from); - return DerivedPath::parseLegacy(store, s); + if (GET_PROTOCOL_MINOR(conn.version) >= 30) { + return DerivedPath::parseLegacy(store, s); + } else { + return parsePathWithOutputs(store, s).toDerivedPath(); + } } void WorkerProto::Serialise::write(const Store & store, WorkerProto::WriteConn conn, const DerivedPath & req) { - conn.to << req.to_string_legacy(store); + if (GET_PROTOCOL_MINOR(conn.version) >= 30) { + conn.to << req.to_string_legacy(store); + } else { + auto sOrDrvPath = StorePathWithOutputs::tryFromDerivedPath(req); + std::visit(overloaded { + [&](const StorePathWithOutputs & s) { + conn.to << s.to_string(store); + }, + [&](const StorePath & drvPath) { + throw Error("trying to request '%s', but daemon protocol %d.%d is too old (< 1.29) to request a derivation file", + store.printStorePath(drvPath), + GET_PROTOCOL_MAJOR(conn.version), + GET_PROTOCOL_MINOR(conn.version)); + }, + [&](std::monostate) { + throw Error("wanted to build a derivation that is itself a build product, but protocols do not support that. Try upgrading the Nix on the other end of this connection"); + }, + }, sOrDrvPath); + } } diff --git a/unit-test-data/libstore/worker-protocol/derived-path-1.29.bin b/unit-test-data/libstore/worker-protocol/derived-path-1.29.bin new file mode 100644 index 0000000000000000000000000000000000000000..05ea7678aa058344a5ca4cb1e9e7333e9cf8fe20 GIT binary patch literal 184 zcmdOAfB^lx%nJSDlKi4n{dB`}^NdR4LR_?NT7Eu*F&X-j5{vXwipsz`&B@oVSfNwN F001Y$HlP3i literal 0 HcmV?d00001 diff --git a/unit-test-data/libstore/worker-protocol/derived-path.bin b/unit-test-data/libstore/worker-protocol/derived-path-1.30.bin similarity index 100% rename from unit-test-data/libstore/worker-protocol/derived-path.bin rename to unit-test-data/libstore/worker-protocol/derived-path-1.30.bin