Factor out serialization for BuildResult

Worker Protocol:

Note that the worker protocol already had a serialization for
`BuildResult`; this was added in
a4604f1. It didn't have any versioning
support because at that time reusable seralizers were not away for the protocol
version. It could thus only be used for new messages also introduced in
that commit.

Now that we do support versioning in reusable serializers, we can expand
it to support all known versions and use it in many more places.

The exist test data becomes the version 1.29 tests: note that those
files' contents are unchanged. 1.28 and 1.27 tests are added to cover
the older code-paths.

The keyered build result test only has 1.29 because the keying was also
added in a4604f1; the older
serializations are always used unkeyed.

Serve Protocol:

Conversely, no attempt was made to factor out such a serializer for the
serve protocol, so our work there in this commit for that protocol
proceeds from scratch.

Author: Ericson2314

src/libstore/daemon.cc

@@ -635,16 +635,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
 
         auto res = store->buildDerivation(drvPath, drv, buildMode);
         logger->stopWork();
-        to << res.status << res.errorMsg;
-        if (GET_PROTOCOL_MINOR(clientVersion) >= 29) {
-            to << res.timesBuilt << res.isNonDeterministic << res.startTime << res.stopTime;
-        }
-        if (GET_PROTOCOL_MINOR(clientVersion) >= 28) {
-            DrvOutputs builtOutputs;
-            for (auto & [output, realisation] : res.builtOutputs)
-                builtOutputs.insert_or_assign(realisation.id, realisation);
-            WorkerProto::write(*store, wconn, builtOutputs);
-        }
+        WorkerProto::write(*store, wconn, res);
         break;
     }
 

src/libstore/legacy-ssh-store.cc

@@ -319,20 +319,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
 
         conn->to.flush();
 
-        BuildResult status;
-        status.status = (BuildResult::Status) readInt(conn->from);
-        conn->from >> status.errorMsg;
-
-        if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 3)
-            conn->from >> status.timesBuilt >> status.isNonDeterministic >> status.startTime >> status.stopTime;
-        if (GET_PROTOCOL_MINOR(conn->remoteVersion) >= 6) {
-            auto builtOutputs = ServeProto::Serialise<DrvOutputs>::read(*this, *conn);
-            for (auto && [output, realisation] : builtOutputs)
-                status.builtOutputs.insert_or_assign(
-                    std::move(output.outputName),
-                    std::move(realisation));
-        }
-        return status;
+        return ServeProto::Serialise<BuildResult>::read(*this, *conn);
     }
 
     void buildPaths(const std::vector<DerivedPath> & drvPaths, BuildMode buildMode, std::shared_ptr<Store> evalStore) override

src/libstore/remote-store.cc

@@ -788,20 +788,7 @@ BuildResult RemoteStore::buildDerivation(const StorePath & drvPath, const BasicD
     writeDerivation(conn->to, *this, drv);
     conn->to << buildMode;
     conn.processStderr();
-    BuildResult res;
-    res.status = (BuildResult::Status) readInt(conn->from);
-    conn->from >> res.errorMsg;
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 29) {
-        conn->from >> res.timesBuilt >> res.isNonDeterministic >> res.startTime >> res.stopTime;
-    }
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 28) {
-        auto builtOutputs = WorkerProto::Serialise<DrvOutputs>::read(*this, *conn);
-        for (auto && [output, realisation] : builtOutputs)
-            res.builtOutputs.insert_or_assign(
-                std::move(output.outputName),
-                std::move(realisation));
-    }
-    return res;
+    return WorkerProto::Serialise<BuildResult>::read(*this, *conn);
 }
 
 

src/libstore/serve-protocol.cc

@@ -2,6 +2,7 @@
 #include "util.hh"
 #include "path-with-outputs.hh"
 #include "store-api.hh"
+#include "build-result.hh"
 #include "serve-protocol.hh"
 #include "serve-protocol-impl.hh"
 #include "archive.hh"
@@ -12,4 +13,46 @@ namespace nix {
 
 /* protocol-specific definitions */
 
+BuildResult ServeProto::Serialise<BuildResult>::read(const Store & store, ServeProto::ReadConn conn)
+{
+    BuildResult status;
+    status.status = (BuildResult::Status) readInt(conn.from);
+    conn.from >> status.errorMsg;
+
+    if (GET_PROTOCOL_MINOR(conn.version) >= 3)
+        conn.from
+            >> status.timesBuilt
+            >> status.isNonDeterministic
+            >> status.startTime
+            >> status.stopTime;
+    if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
+        auto builtOutputs = ServeProto::Serialise<DrvOutputs>::read(store, conn);
+        for (auto && [output, realisation] : builtOutputs)
+            status.builtOutputs.insert_or_assign(
+                std::move(output.outputName),
+                std::move(realisation));
+    }
+    return status;
+}
+
+void ServeProto::Serialise<BuildResult>::write(const Store & store, ServeProto::WriteConn conn, const BuildResult & status)
+{
+    conn.to
+        << status.status
+        << status.errorMsg;
+
+    if (GET_PROTOCOL_MINOR(conn.version) >= 3)
+        conn.to
+            << status.timesBuilt
+            << status.isNonDeterministic
+            << status.startTime
+            << status.stopTime;
+    if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
+        DrvOutputs builtOutputs;
+        for (auto & [output, realisation] : status.builtOutputs)
+            builtOutputs.insert_or_assign(realisation.id, realisation);
+        ServeProto::write(store, conn, builtOutputs);
+    }
+}
+
 }

src/libstore/serve-protocol.hh

@@ -16,6 +16,9 @@ namespace nix {
 class Store;
 struct Source;
 
+// items being serialised
+struct BuildResult;
+
 
 /**
  * The "serve protocol", used by ssh:// stores.
@@ -136,6 +139,9 @@ inline std::ostream & operator << (std::ostream & s, ServeProto::Command op)
         static void write(const Store & store, ServeProto::WriteConn conn, const T & t); \
     };
 
+template<>
+DECLARE_SERVE_SERIALISER(BuildResult);
+
 template<typename T>
 DECLARE_SERVE_SERIALISER(std::vector<T>);
 template<typename T>

src/libstore/tests/serve-protocol.cc

@@ -19,7 +19,7 @@ struct ServeProtoTest : VersionedProtoTest<ServeProto, serveProtoDir>
      * For serializers that don't care about the minimum version, we
      * used the oldest one: 1.0.
      */
-    ServeProto::Version defaultVersion = 1 << 8 | 0;
+    ServeProto::Version defaultVersion = 2 << 8 | 0;
 };
 
 VERSIONED_CHARACTERIZATION_TEST(
@@ -114,6 +114,117 @@ VERSIONED_CHARACTERIZATION_TEST(
         },
     }))
 
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    buildResult_2_2,
+    "build-result-2.2",
+    2 << 8 | 2,
+    ({
+        using namespace std::literals::chrono_literals;
+        std::tuple<BuildResult, BuildResult, BuildResult> t {
+            BuildResult {
+                .status = BuildResult::OutputRejected,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::NotDeterministic,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::Built,
+            },
+        };
+        t;
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    buildResult_2_3,
+    "build-result-2.3",
+    2 << 8 | 3,
+    ({
+        using namespace std::literals::chrono_literals;
+        std::tuple<BuildResult, BuildResult, BuildResult> t {
+            BuildResult {
+                .status = BuildResult::OutputRejected,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::NotDeterministic,
+                .errorMsg = "no idea why",
+                .timesBuilt = 3,
+                .isNonDeterministic = true,
+                .startTime = 30,
+                .stopTime = 50,
+            },
+            BuildResult {
+                .status = BuildResult::Built,
+                .startTime = 30,
+                .stopTime = 50,
+            },
+        };
+        t;
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    buildResult_2_6,
+    "build-result-2.6",
+    2 << 8 | 6,
+    ({
+        using namespace std::literals::chrono_literals;
+        std::tuple<BuildResult, BuildResult, BuildResult> t {
+            BuildResult {
+                .status = BuildResult::OutputRejected,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::NotDeterministic,
+                .errorMsg = "no idea why",
+                .timesBuilt = 3,
+                .isNonDeterministic = true,
+                .startTime = 30,
+                .stopTime = 50,
+            },
+            BuildResult {
+                .status = BuildResult::Built,
+                .timesBuilt = 1,
+                .builtOutputs = {
+                    {
+                        "foo",
+                        {
+                            .id = DrvOutput {
+                                .drvHash = Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="),
+                                .outputName = "foo",
+                            },
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
+                        },
+                    },
+                    {
+                        "bar",
+                        {
+                            .id = DrvOutput {
+                                .drvHash = Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="),
+                                .outputName = "bar",
+                            },
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar" },
+                        },
+                    },
+                },
+                .startTime = 30,
+                .stopTime = 50,
+#if 0
+                // These fields are not yet serialized.
+                // FIXME Include in next version of protocol or document
+                // why they are skipped.
+                .cpuUser = std::chrono::milliseconds(500s),
+                .cpuSystem = std::chrono::milliseconds(604s),
+#endif
+            },
+        };
+        t;
+    }))
+
 VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     vector,

src/libstore/tests/worker-protocol.cc

@@ -167,9 +167,77 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
-    buildResult,
-    "build-result",
-    defaultVersion,
+    buildResult_1_27,
+    "build-result-1.27",
+    1 << 8 | 27,
+    ({
+        using namespace std::literals::chrono_literals;
+        std::tuple<BuildResult, BuildResult, BuildResult> t {
+            BuildResult {
+                .status = BuildResult::OutputRejected,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::NotDeterministic,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::Built,
+            },
+        };
+        t;
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    WorkerProtoTest,
+    buildResult_1_28,
+    "build-result-1.28",
+    1 << 8 | 28,
+    ({
+        using namespace std::literals::chrono_literals;
+        std::tuple<BuildResult, BuildResult, BuildResult> t {
+            BuildResult {
+                .status = BuildResult::OutputRejected,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::NotDeterministic,
+                .errorMsg = "no idea why",
+            },
+            BuildResult {
+                .status = BuildResult::Built,
+                .builtOutputs = {
+                    {
+                        "foo",
+                        {
+                            .id = DrvOutput {
+                                .drvHash = Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="),
+                                .outputName = "foo",
+                            },
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
+                        },
+                    },
+                    {
+                        "bar",
+                        {
+                            .id = DrvOutput {
+                                .drvHash = Hash::parseSRI("sha256-b4afnqKCO9oWXgYHb9DeQ2berSwOjS27rSd9TxXDc/U="),
+                                .outputName = "bar",
+                            },
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar" },
+                        },
+                    },
+                },
+            },
+        };
+        t;
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    WorkerProtoTest,
+    buildResult_1_29,
+    "build-result-1.29",
+    1 << 8 | 29,
     ({
         using namespace std::literals::chrono_literals;
         std::tuple<BuildResult, BuildResult, BuildResult> t {
@@ -226,9 +294,9 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
-    keyedBuildResult,
-    "keyed-build-result",
-    defaultVersion,
+    keyedBuildResult_1_29,
+    "keyed-build-result-1.29",
+    1 << 8 | 29,
     ({
         using namespace std::literals::chrono_literals;
         std::tuple<KeyedBuildResult, KeyedBuildResult/*, KeyedBuildResult*/> t {