Factor out serialization for BuildResult

Worker Protocol:

Note that the worker protocol already had a serialization for
`BuildResult`; this was added in
a4604f1928. 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 a4604f19284254ac98f19a13ff7c2216de7fe176; 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.

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 @@ public:
 
         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 {

src/libstore/worker-protocol.cc

@@ -103,17 +103,21 @@ BuildResult WorkerProto::Serialise<BuildResult>::read(const Store & store, Worke
 {
     BuildResult res;
     res.status = (BuildResult::Status) readInt(conn.from);
+    conn.from >> res.errorMsg;
+    if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
         conn.from
-        >> res.errorMsg
             >> res.timesBuilt
             >> res.isNonDeterministic
             >> res.startTime
             >> res.stopTime;
+    }
+    if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
         auto builtOutputs = WorkerProto::Serialise<DrvOutputs>::read(store, conn);
         for (auto && [output, realisation] : builtOutputs)
             res.builtOutputs.insert_or_assign(
                 std::move(output.outputName),
                 std::move(realisation));
+    }
     return res;
 }
 
@@ -121,15 +125,20 @@ void WorkerProto::Serialise<BuildResult>::write(const Store & store, WorkerProto
 {
     conn.to
         << res.status
-        << res.errorMsg
+        << res.errorMsg;
+    if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
+        conn.to
             << res.timesBuilt
             << res.isNonDeterministic
             << res.startTime
             << res.stopTime;
+    }
+    if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
         DrvOutputs builtOutputs;
         for (auto & [output, realisation] : res.builtOutputs)
             builtOutputs.insert_or_assign(realisation.id, realisation);
         WorkerProto::write(store, conn, builtOutputs);
+    }
 }
 
 

src/nix-store/nix-store.cc

@@ -959,17 +959,7 @@ static void opServe(Strings opFlags, Strings opArgs)
                 MonitorFdHup monitor(in.fd);
                 auto status = store->buildDerivation(drvPath, drv);
 
-                out << status.status << status.errorMsg;
-
-                if (GET_PROTOCOL_MINOR(clientVersion) >= 3)
-                    out << status.timesBuilt << status.isNonDeterministic << status.startTime << status.stopTime;
-                if (GET_PROTOCOL_MINOR(clientVersion) >= 6) {
-                    DrvOutputs builtOutputs;
-                    for (auto & [output, realisation] : status.builtOutputs)
-                        builtOutputs.insert_or_assign(realisation.id, realisation);
-                    ServeProto::write(*store, wconn, builtOutputs);
-                }
-
+                ServeProto::write(*store, wconn, status);
                 break;
             }