Merge pull request #9572 from obsidiansystems/serve-proto-build-options

Create `ServeProto::BuildOptions` and a serializer for it

src/libstore/legacy-ssh-store.cc

@@ -275,20 +275,14 @@ private:
 
     void putBuildSettings(Connection & conn)
     {
-        conn.to
-            << settings.maxSilentTime
-            << settings.buildTimeout;
-        if (GET_PROTOCOL_MINOR(conn.remoteVersion) >= 2)
-            conn.to
-                << settings.maxLogSize;
-        if (GET_PROTOCOL_MINOR(conn.remoteVersion) >= 3)
-            conn.to
-                << 0 // buildRepeat hasn't worked for ages anyway
-                << 0;
-
-        if (GET_PROTOCOL_MINOR(conn.remoteVersion) >= 7) {
-            conn.to << ((int) settings.keepFailed);
-        }
+        ServeProto::write(*this, conn, ServeProto::BuildOptions {
+            .maxSilentTime = settings.maxSilentTime,
+            .buildTimeout = settings.buildTimeout,
+            .maxLogSize = settings.maxLogSize,
+            .nrRepeats = 0, // buildRepeat hasn't worked for ages anyway
+            .enforceDeterminism = 0,
+            .keepFailed = settings.keepFailed,
+        });
     }
 
 public:

src/libstore/serve-protocol.cc

@@ -98,4 +98,40 @@ void ServeProto::Serialise<UnkeyedValidPathInfo>::write(const StoreDirConfig & s
             << info.sigs;
 }
 
+
+ServeProto::BuildOptions ServeProto::Serialise<ServeProto::BuildOptions>::read(const StoreDirConfig & store, ReadConn conn)
+{
+    BuildOptions options;
+    options.maxSilentTime = readInt(conn.from);
+    options.buildTimeout = readInt(conn.from);
+    if (GET_PROTOCOL_MINOR(conn.version) >= 2)
+        options.maxLogSize = readNum<unsigned long>(conn.from);
+    if (GET_PROTOCOL_MINOR(conn.version) >= 3) {
+        options.nrRepeats = readInt(conn.from);
+        options.enforceDeterminism = readInt(conn.from);
+    }
+    if (GET_PROTOCOL_MINOR(conn.version) >= 7) {
+        options.keepFailed = (bool) readInt(conn.from);
+    }
+    return options;
+}
+
+void ServeProto::Serialise<ServeProto::BuildOptions>::write(const StoreDirConfig & store, WriteConn conn, const ServeProto::BuildOptions & options)
+{
+    conn.to
+        << options.maxSilentTime
+        << options.buildTimeout;
+    if (GET_PROTOCOL_MINOR(conn.version) >= 2)
+        conn.to
+            << options.maxLogSize;
+    if (GET_PROTOCOL_MINOR(conn.version) >= 3)
+        conn.to
+            << options.nrRepeats
+            << options.enforceDeterminism;
+
+    if (GET_PROTOCOL_MINOR(conn.version) >= 7) {
+        conn.to << ((int) options.keepFailed);
+    }
+}
+
 }

src/libstore/serve-protocol.hh

@@ -87,6 +87,13 @@ struct ServeProto
     {
         ServeProto::Serialise<T>::write(store, conn, t);
     }
+
+    /**
+     * Options for building shared between
+     * `ServeProto::Command::BuildPaths` and
+     * `ServeProto::Command::BuildDerivation`.
+     */
+    struct BuildOptions;
 };
 
 enum struct ServeProto::Command : uint64_t
@@ -102,6 +109,22 @@ enum struct ServeProto::Command : uint64_t
     AddToStoreNar = 9,
 };
 
+
+struct ServeProto::BuildOptions {
+    /**
+     * Default value in this and every other field is so tests pass when
+     * testing older deserialisers which do not set all the fields.
+     */
+    time_t maxSilentTime = -1;
+    time_t buildTimeout = -1;
+    size_t maxLogSize = -1;
+    size_t nrRepeats = -1;
+    bool enforceDeterminism = -1;
+    bool keepFailed = -1;
+
+    bool operator == (const ServeProto::BuildOptions &) const = default;
+};
+
 /**
  * Convenience for sending operation codes.
  *
@@ -144,6 +167,8 @@ template<>
 DECLARE_SERVE_SERIALISER(BuildResult);
 template<>
 DECLARE_SERVE_SERIALISER(UnkeyedValidPathInfo);
+template<>
+DECLARE_SERVE_SERIALISER(ServeProto::BuildOptions);
 
 template<typename T>
 DECLARE_SERVE_SERIALISER(std::vector<T>);

src/nix-store/nix-store.cc

@@ -835,27 +835,33 @@ static void opServe(Strings opFlags, Strings opArgs)
         verbosity = lvlError;
         settings.keepLog = false;
         settings.useSubstitutes = false;
-        settings.maxSilentTime = readInt(in);
-        settings.buildTimeout = readInt(in);
+
+        auto options = ServeProto::Serialise<ServeProto::BuildOptions>::read(*store, rconn);
+
+        // Only certain feilds get initialized based on the protocol
+        // version. This is why not all the code below is unconditional.
+        // See how the serialization logic in
+        // `ServeProto::Serialise<ServeProto::BuildOptions>` matches
+        // these conditions.
+        settings.maxSilentTime = options.maxSilentTime;
+        settings.buildTimeout = options.buildTimeout;
         if (GET_PROTOCOL_MINOR(clientVersion) >= 2)
-            settings.maxLogSize = readNum<unsigned long>(in);
+            settings.maxLogSize = options.maxLogSize;
         if (GET_PROTOCOL_MINOR(clientVersion) >= 3) {
-            auto nrRepeats = readInt(in);
-            if (nrRepeats != 0) {
+            if (options.nrRepeats != 0) {
                 throw Error("client requested repeating builds, but this is not currently implemented");
             }
-            // Ignore 'enforceDeterminism'. It used to be true by
-            // default, but also only never had any effect when
-            // `nrRepeats == 0`.  We have already asserted that
-            // `nrRepeats` in fact is 0, so we can safely ignore this
-            // without doing something other than what the client
-            // asked for.
-            readInt(in);
-
+            // Ignore 'options.enforceDeterminism'.
+            //
+            // It used to be true by default, but also only never had
+            // any effect when `nrRepeats == 0`.  We have already
+            // checked that `nrRepeats` in fact is 0, so we can safely
+            // ignore this without doing something other than what the
+            // client asked for.
             settings.runDiffHook = true;
         }
         if (GET_PROTOCOL_MINOR(clientVersion) >= 7) {
-            settings.keepFailed = (bool) readInt(in);
+            settings.keepFailed = options.keepFailed;
         }
     };
 

tests/unit/libstore/serve-protocol.cc

@@ -302,6 +302,54 @@ VERSIONED_CHARACTERIZATION_TEST(
         }),
     }))
 
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    build_options_2_1,
+    "build-options-2.1",
+    2 << 8 | 1,
+    (ServeProto::BuildOptions {
+        .maxSilentTime = 5,
+        .buildTimeout = 6,
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    build_options_2_2,
+    "build-options-2.2",
+    2 << 8 | 2,
+    (ServeProto::BuildOptions {
+        .maxSilentTime = 5,
+        .buildTimeout = 6,
+        .maxLogSize = 7,
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    build_options_2_3,
+    "build-options-2.3",
+    2 << 8 | 3,
+    (ServeProto::BuildOptions {
+        .maxSilentTime = 5,
+        .buildTimeout = 6,
+        .maxLogSize = 7,
+        .nrRepeats = 8,
+        .enforceDeterminism = true,
+    }))
+
+VERSIONED_CHARACTERIZATION_TEST(
+    ServeProtoTest,
+    build_options_2_7,
+    "build-options-2.7",
+    2 << 8 | 7,
+    (ServeProto::BuildOptions {
+        .maxSilentTime = 5,
+        .buildTimeout = 6,
+        .maxLogSize = 7,
+        .nrRepeats = 8,
+        .enforceDeterminism = false,
+        .keepFailed = true,
+    }))
+
 VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     vector,