Fix remote protocol unit tests

src/libstore-test-support/include/nix/store/tests/protocol.hh

@@ -64,12 +64,18 @@ public:
     }
 };
 
-#define VERSIONED_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \
+#define VERSIONED_READ_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \
     TEST_F(FIXTURE, NAME ## _read) { \
         readProtoTest(STEM, VERSION, VALUE); \
-    } \
+    }
+
+#define VERSIONED_WRITE_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \
     TEST_F(FIXTURE, NAME ## _write) { \
         writeProtoTest(STEM, VERSION, VALUE); \
     }
 
+#define VERSIONED_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \
+    VERSIONED_READ_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \
+    VERSIONED_WRITE_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE)
+
 }

src/libstore-tests/serve-protocol.cc

@@ -72,7 +72,7 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
-    drvOutput,
+    drvOutput_2_8,
     "drv-output-2.8",
     2 << 8 | 8,
     (std::tuple<DrvOutput, DrvOutput> {
@@ -90,7 +90,7 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
-    unkeyedRealisation,
+    unkeyedRealisation_2_8,
     "unkeyed-realisation-2.8",
     2 << 8 | 8,
     (UnkeyedRealisation {
@@ -100,7 +100,7 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
-    realisation,
+    realisation_2_8,
     "realisation-2.8",
     2 << 8 | 8,
     (Realisation {
@@ -166,9 +166,64 @@ VERSIONED_CHARACTERIZATION_TEST(
         t;
     }))
 
-VERSIONED_CHARACTERIZATION_TEST(
+/* We now do a lossy read which does not allow us to faithfully right
+   back, since we changed the data type. We still however want to test
+   that this read works, and so for that we have a one-way test. */
+VERSIONED_READ_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",
+                        {
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
+                        },
+                    },
+                    {
+                        "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,
+    buildResult_2_8,
     "build-result-2.8",
     2 << 8 | 8,
     ({

src/libstore-tests/worker-protocol.cc

@@ -140,7 +140,7 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
-    unkeyedRealisation,
+    unkeyedRealisation_1_39,
     "unkeyed-realisation-1.39",
     1 << 8 | 39,
     (UnkeyedRealisation {
@@ -150,7 +150,7 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
-    realisation,
+    realisation_1_39,
     "realisation-1.39",
     1 << 8 | 39,
     (Realisation {
@@ -187,7 +187,10 @@ VERSIONED_CHARACTERIZATION_TEST(
         t;
     }))
 
-VERSIONED_CHARACTERIZATION_TEST(
+/* We now do a lossy read which does not allow us to faithfully right
+   back, since we changed the data type. We still however want to test
+   that this read works, and so for that we have a one-way test. */
+VERSIONED_READ_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     buildResult_1_28,
     "build-result-1.28",
@@ -224,7 +227,8 @@ VERSIONED_CHARACTERIZATION_TEST(
         t;
     }))
 
-VERSIONED_CHARACTERIZATION_TEST(
+// See above note
+VERSIONED_READ_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     buildResult_1_29,
     "build-result-1.29",
@@ -268,7 +272,8 @@ VERSIONED_CHARACTERIZATION_TEST(
         t;
     }))
 
-VERSIONED_CHARACTERIZATION_TEST(
+// See above note
+VERSIONED_READ_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     buildResult_1_37,
     "build-result-1.37",
@@ -314,6 +319,52 @@ VERSIONED_CHARACTERIZATION_TEST(
         t;
     }))
 
+VERSIONED_CHARACTERIZATION_TEST(
+    WorkerProtoTest,
+    buildResult_1_39,
+    "build-result-1.39",
+    1 << 8 | 39,
+    ({
+        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",
+                        {
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
+                        },
+                    },
+                    {
+                        "bar",
+                        {
+                            .outPath = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar" },
+                        },
+                    },
+                },
+                .startTime = 30,
+                .stopTime = 50,
+                .cpuUser = std::chrono::microseconds(500s),
+                .cpuSystem = std::chrono::microseconds(604s),
+            },
+        };
+        t;
+    }))
+
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     keyedBuildResult_1_29,

src/libstore/include/nix/store/realisation.hh

@@ -119,15 +119,6 @@ struct Realisation : UnkeyedRealisation
  */
 typedef std::map<OutputName, UnkeyedRealisation> SingleDrvOutputs;
 
-/**
- * Collection type for multiple derivations' outputs' `Realisation`s.
- *
- * `DrvOutput` is used because in general the derivations are not all
- * the same, so we need to identify firstly which derivation, and
- * secondly which output of that derivation.
- */
-typedef std::map<DrvOutput, UnkeyedRealisation> DrvOutputs;
-
 /**
  * Filter a SingleDrvOutputs to include only specific output names
  *

src/libstore/nar-info-disk-cache.cc

@@ -93,7 +93,7 @@ public:
 
     Sync<State> _state;
 
-    NarInfoDiskCacheImpl(Path dbPath = getCacheDir() + "/binary-cache-v6.sqlite")
+    NarInfoDiskCacheImpl(Path dbPath = getCacheDir() + "/binary-cache-v7.sqlite")
     {
         auto state(_state.lock());
 

src/libstore/serve-protocol.cc

@@ -6,6 +6,7 @@
 #include "nix/store/serve-protocol-impl.hh"
 #include "nix/util/archive.hh"
 #include "nix/store/path-info.hh"
+#include "nix/util/json-utils.hh"
 
 #include <nlohmann/json.hpp>
 
@@ -25,12 +26,22 @@ BuildResult ServeProto::Serialise<BuildResult>::read(const StoreDirConfig & stor
             >> status.isNonDeterministic
             >> status.startTime
             >> status.stopTime;
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 8) {
         status.builtOutputs = ServeProto::Serialise<std::map<OutputName, UnkeyedRealisation>>::read(store, conn);
     } else if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
-        // We no longer support these types of realisations
-        (void) ServeProto::Serialise<StringMap>::read(store, conn);
+        for (auto & [output, realisation] : ServeProto::Serialise<StringMap>::read(store, conn)) {
+            size_t n = output.find("!");
+            if (n == output.npos)
+                throw Error("Invalid derivation output id %s", output);
+            status.builtOutputs.insert_or_assign(
+                output.substr(n + 1),
+                UnkeyedRealisation{StorePath{
+                    getString(valueAt(getObject(nlohmann::json::parse(realisation)), "outPath"))
+                }});
+        }
     }
+
     return status;
 }
 
@@ -46,6 +57,7 @@ void ServeProto::Serialise<BuildResult>::write(const StoreDirConfig & store, Ser
             << status.isNonDeterministic
             << status.startTime
             << status.stopTime;
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 8) {
         ServeProto::write(store, conn, status.builtOutputs);
     } else if (GET_PROTOCOL_MINOR(conn.version) >= 6) {
@@ -136,9 +148,9 @@ void ServeProto::Serialise<ServeProto::BuildOptions>::write(const StoreDirConfig
 
 UnkeyedRealisation ServeProto::Serialise<UnkeyedRealisation>::read(const StoreDirConfig & store, ReadConn conn)
 {
-    if (GET_PROTOCOL_MINOR(conn.version) < 39) {
-        throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+    if (GET_PROTOCOL_MINOR(conn.version) < 8) {
+        throw Error("daemon protocol %d.%d is too old (< 2.8) to understand build trace",
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
 
@@ -153,9 +165,9 @@ UnkeyedRealisation ServeProto::Serialise<UnkeyedRealisation>::read(const StoreDi
 
 void ServeProto::Serialise<UnkeyedRealisation>::write(const StoreDirConfig & store, WriteConn conn, const UnkeyedRealisation & info)
 {
-    if (GET_PROTOCOL_MINOR(conn.version) < 39) {
-        throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+    if (GET_PROTOCOL_MINOR(conn.version) < 8) {
+        throw Error("daemon protocol %d.%d is too old (< 2.8) to understand build trace",
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
     ServeProto::write(store, conn, info.outPath);
@@ -165,9 +177,9 @@ void ServeProto::Serialise<UnkeyedRealisation>::write(const StoreDirConfig & sto
 
 DrvOutput ServeProto::Serialise<DrvOutput>::read(const StoreDirConfig & store, ReadConn conn)
 {
-    if (GET_PROTOCOL_MINOR(conn.version) < 39) {
-        throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+    if (GET_PROTOCOL_MINOR(conn.version) < 8) {
+        throw Error("daemon protocol %d.%d is too old (< 2.8) to understand build trace",
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
 
@@ -182,9 +194,9 @@ DrvOutput ServeProto::Serialise<DrvOutput>::read(const StoreDirConfig & store, R
 
 void ServeProto::Serialise<DrvOutput>::write(const StoreDirConfig & store, WriteConn conn, const DrvOutput & info)
 {
-    if (GET_PROTOCOL_MINOR(conn.version) < 39) {
-        throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+    if (GET_PROTOCOL_MINOR(conn.version) < 8) {
+        throw Error("daemon protocol %d.%d is too old (< 2.8) to understand build trace",
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
     ServeProto::write(store, conn, info.drvPath);

src/libstore/worker-protocol.cc

@@ -6,6 +6,7 @@
 #include "nix/store/worker-protocol-impl.hh"
 #include "nix/util/archive.hh"
 #include "nix/store/path-info.hh"
+#include "nix/util/json-utils.hh"
 
 #include <chrono>
 #include <nlohmann/json.hpp>
@@ -124,7 +125,7 @@ void WorkerProto::Serialise<DerivedPath>::write(const StoreDirConfig & store, Wo
             [&](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_MAJOR(conn.version) >> 8,
                     GET_PROTOCOL_MINOR(conn.version));
             },
             [&](std::monostate) {
@@ -157,6 +158,7 @@ BuildResult WorkerProto::Serialise<BuildResult>::read(const StoreDirConfig & sto
     BuildResult res;
     res.status = static_cast<BuildResult::Status>(readInt(conn.from));
     conn.from >> res.errorMsg;
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
         conn.from
             >> res.timesBuilt
@@ -164,17 +166,27 @@ BuildResult WorkerProto::Serialise<BuildResult>::read(const StoreDirConfig & sto
             >> res.startTime
             >> res.stopTime;
     }
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 37) {
         res.cpuUser = WorkerProto::Serialise<std::optional<std::chrono::microseconds>>::read(store, conn);
         res.cpuSystem = WorkerProto::Serialise<std::optional<std::chrono::microseconds>>::read(store, conn);
     }
-    if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
-        auto builtOutputs = WorkerProto::Serialise<DrvOutputs>::read(store, conn);
-        for (auto && [output, realisation] : builtOutputs)
+
+    if (GET_PROTOCOL_MINOR(conn.version) >= 39) {
+        res.builtOutputs = WorkerProto::Serialise<std::map<OutputName, UnkeyedRealisation>>::read(store, conn);
+    } else if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
+        for (auto && [output, realisation] : WorkerProto::Serialise<StringMap>::read(store, conn)) {
+            size_t n = output.find("!");
+            if (n == output.npos)
+                throw Error("Invalid derivation output id %s", output);
             res.builtOutputs.insert_or_assign(
-                std::move(output.outputName),
-                std::move(realisation));
+                output.substr(n + 1),
+                UnkeyedRealisation{StorePath{
+                    getString(valueAt(getObject(nlohmann::json::parse(realisation)), "outPath"))
+                }});
+        }
     }
+
     return res;
 }
 
@@ -183,6 +195,7 @@ void WorkerProto::Serialise<BuildResult>::write(const StoreDirConfig & store, Wo
     conn.to
         << res.status
         << res.errorMsg;
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 29) {
         conn.to
             << res.timesBuilt
@@ -190,16 +203,17 @@ void WorkerProto::Serialise<BuildResult>::write(const StoreDirConfig & store, Wo
             << res.startTime
             << res.stopTime;
     }
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 37) {
         WorkerProto::write(store, conn, res.cpuUser);
         WorkerProto::write(store, conn, res.cpuSystem);
     }
-    if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
-        // Don't support those types of realisations anymore.
-        WorkerProto::write(store, conn, StringMap{});
-    }
+
     if (GET_PROTOCOL_MINOR(conn.version) >= 39) {
         WorkerProto::write(store, conn, res.builtOutputs);
+    } else if (GET_PROTOCOL_MINOR(conn.version) >= 28) {
+        // Don't support those types of realisations anymore.
+        WorkerProto::write(store, conn, StringMap{});
     }
 }
 
@@ -286,8 +300,8 @@ void WorkerProto::Serialise<WorkerProto::ClientHandshakeInfo>::write(const Store
 UnkeyedRealisation WorkerProto::Serialise<UnkeyedRealisation>::read(const StoreDirConfig & store, ReadConn conn)
 {
     if (GET_PROTOCOL_MINOR(conn.version) < 39) {
-        throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+        throw Error("daemon protocol %d.%d is too old (< 1.39) to understand build trace",
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
 
@@ -303,8 +317,8 @@ UnkeyedRealisation WorkerProto::Serialise<UnkeyedRealisation>::read(const StoreD
 void WorkerProto::Serialise<UnkeyedRealisation>::write(const StoreDirConfig & store, WriteConn conn, const UnkeyedRealisation & info)
 {
     if (GET_PROTOCOL_MINOR(conn.version) < 39) {
-        throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+        throw Error("daemon protocol %d.%d is too old (< 1.39) to understand build trace",
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
     WorkerProto::write(store, conn, info.outPath);
@@ -346,7 +360,7 @@ DrvOutput WorkerProto::Serialise<DrvOutput>::read(const StoreDirConfig & store,
 {
     if (GET_PROTOCOL_MINOR(conn.version) < 39) {
         throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
 
@@ -363,7 +377,7 @@ void WorkerProto::Serialise<DrvOutput>::write(const StoreDirConfig & store, Writ
 {
     if (GET_PROTOCOL_MINOR(conn.version) < 39) {
         throw Error("daemon protocol %d.%d is too old (< 1.29) to understand build trace",
-            GET_PROTOCOL_MAJOR(conn.version),
+            GET_PROTOCOL_MAJOR(conn.version) >> 8,
             GET_PROTOCOL_MINOR(conn.version));
     }
     WorkerProto::write(store, conn, info.drvPath);