Pass all settings to build-remote

This ensures that command line flags such as --builders get passed
correctly.

src/build-remote/build-remote.cc

@@ -46,16 +46,23 @@ int main (int argc, char * * argv)
         unsetenv("DISPLAY");
         unsetenv("SSH_ASKPASS");
 
-        if (argc != 6)
+        if (argc != 2)
             throw UsageError("called without required arguments");
 
-        auto store = openStore().cast<LocalStore>();
+        verbosity = (Verbosity) std::stoll(argv[1]);
 
-        auto localSystem = argv[1];
-        settings.maxSilentTime = std::stoll(argv[2]);
-        settings.buildTimeout = std::stoll(argv[3]);
-        verbosity = (Verbosity) std::stoll(argv[4]);
-        settings.builders = argv[5];
+        FdSource source(STDIN_FILENO);
+
+        /* Read the parent's settings. */
+        while (readInt(source)) {
+            auto name = readString(source);
+            auto value = readString(source);
+            settings.set(name, value);
+        }
+
+        settings.maxBuildJobs.set("1"); // hack to make tests with local?root= work
+
+        auto store = openStore().cast<LocalStore>();
 
         /* It would be more appropriate to use $XDG_RUNTIME_DIR, since
            that gets cleared on reboot, but it wouldn't work on macOS. */
@@ -74,18 +81,20 @@ int main (int argc, char * * argv)
 
         string drvPath;
         string storeUri;
-        for (string line; getline(cin, line);) {
-            auto tokens = tokenizeString<std::vector<string>>(line);
-            auto sz = tokens.size();
-            if (sz != 3 && sz != 4)
-                throw Error("invalid build hook line '%1%'", line);
-            auto amWilling = tokens[0] == "1";
-            auto neededSystem = tokens[1];
-            drvPath = tokens[2];
-            auto requiredFeatures = sz == 3 ?
-                std::set<string>{} :
-                tokenizeString<std::set<string>>(tokens[3], ",");
-            auto canBuildLocally = amWilling && (neededSystem == localSystem);
+
+        while (true) {
+
+            try {
+                auto s = readString(source);
+                if (s != "try") return;
+            } catch (EndOfFile &) { return; }
+
+            auto amWilling = readInt(source);
+            auto neededSystem = readString(source);
+            source >> drvPath;
+            auto requiredFeatures = readStrings<std::set<std::string>>(source);
+
+            auto canBuildLocally = amWilling && (neededSystem == settings.thisSystem);
 
             /* Error ignored here, will be caught later */
             mkdir(currentLoad.c_str(), 0777);
@@ -100,7 +109,7 @@ int main (int argc, char * * argv)
                 Machine * bestMachine = nullptr;
                 unsigned long long bestLoad = 0;
                 for (auto & m : machines) {
-                    debug("considering building on '%s'", m.storeUri);
+                    debug("considering building on remote machine '%s'", m.storeUri);
 
                     if (m.enabled && std::find(m.systemTypes.begin(),
                             m.systemTypes.end(),
@@ -184,15 +193,9 @@ int main (int argc, char * * argv)
 
 connected:
         std::cerr << "# accept\n";
-        string line;
-        if (!getline(cin, line))
-            throw Error("hook caller didn't send inputs");
 
-        auto inputs = tokenizeString<PathSet>(line);
-        if (!getline(cin, line))
-            throw Error("hook caller didn't send outputs");
-
-        auto outputs = tokenizeString<PathSet>(line);
+        auto inputs = readStrings<PathSet>(source);
+        auto outputs = readStrings<PathSet>(source);
 
         AutoCloseFD uploadLock = openLockFile(currentLoad + "/" + escapeUri(storeUri) + ".upload-lock", true);
 

src/libstore/build.cc

@@ -606,6 +606,8 @@ struct HookInstance
     /* The process ID of the hook. */
     Pid pid;
 
+    FdSink sink;
+
     HookInstance();
 
     ~HookInstance();
@@ -642,11 +644,7 @@ HookInstance::HookInstance()
 
         Strings args = {
             baseNameOf(settings.buildHook),
-            settings.thisSystem,
-            std::to_string(settings.maxSilentTime),
-            std::to_string(settings.buildTimeout),
             std::to_string(verbosity),
-            settings.builders
         };
 
         execv(settings.buildHook.get().c_str(), stringsToCharPtrs(args).data());
@@ -657,6 +655,11 @@ HookInstance::HookInstance()
     pid.setSeparatePG(true);
     fromHook.writeSide = -1;
     toHook.readSide = -1;
+
+    sink = FdSink(toHook.writeSide.get());
+    for (auto & setting : settings.getSettings())
+        sink << 1 << setting.first << setting.second;
+    sink << 0;
 }
 
 
@@ -1633,9 +1636,13 @@ HookReply DerivationGoal::tryBuildHook()
         for (auto & i : features) checkStoreName(i); /* !!! abuse */
 
         /* Send the request to the hook. */
-        writeLine(worker.hook->toHook.writeSide.get(), (format("%1% %2% %3% %4%")
-                % (worker.getNrLocalBuilds() < settings.maxBuildJobs ? "1" : "0")
-                % drv->platform % drvPath % concatStringsSep(",", features)).str());
+        worker.hook->sink
+            << "try"
+            << (worker.getNrLocalBuilds() < settings.maxBuildJobs ? 1 : 0)
+            << drv->platform
+            << drvPath
+            << features;
+        worker.hook->sink.flush();
 
         /* Read the first line of input, which should be a word indicating
            whether the hook wishes to perform the build. */
@@ -1680,12 +1687,13 @@ HookReply DerivationGoal::tryBuildHook()
 
     /* Tell the hook all the inputs that have to be copied to the
        remote system. */
-    writeLine(hook->toHook.writeSide.get(), concatStringsSep(" ", inputPaths));
+    hook->sink << inputPaths;
 
     /* Tell the hooks the missing outputs that have to be copied back
        from the remote system. */
-    writeLine(hook->toHook.writeSide.get(), concatStringsSep(" ", missingPaths));
+    hook->sink << missingPaths;
 
+    hook->sink = FdSink();
     hook->toHook.writeSide = -1;
 
     /* Create the log file and pipe. */
@@ -3986,7 +3994,7 @@ void Worker::run(const Goals & _topGoals)
         else {
             if (awake.empty() && 0 == settings.maxBuildJobs) throw Error(
                 "unable to start any build; either increase '--max-jobs' "
-                "or enable distributed builds");
+                "or enable remote builds");
             assert(!awake.empty());
         }
     }

src/libutil/serialise.hh

@@ -92,7 +92,17 @@ struct FdSink : BufferedSink
     FdSink() : fd(-1) { }
     FdSink(int fd) : fd(fd) { }
     FdSink(FdSink&&) = default;
-    FdSink& operator=(FdSink&&) = default;
+
+    FdSink& operator=(FdSink && s)
+    {
+        flush();
+        fd = s.fd;
+        s.fd = -1;
+        warn = s.warn;
+        written = s.written;
+        return *this;
+    }
+
     ~FdSink();
 
     void write(const unsigned char * data, size_t len) override;
@@ -112,6 +122,16 @@ struct FdSource : BufferedSource
 
     FdSource() : fd(-1) { }
     FdSource(int fd) : fd(fd) { }
+    FdSource(FdSource&&) = default;
+
+    FdSource& operator=(FdSource && s)
+    {
+        fd = s.fd;
+        s.fd = -1;
+        read = s.read;
+        return *this;
+    }
+
     size_t readUnbuffered(unsigned char * data, size_t len) override;
     bool good() override;
 private:

tests/build-hook.hook.sh

@@ -1,23 +0,0 @@
-#! /bin/sh
-
-#set -x
-
-while read x y drv rest; do
-
-    echo "HOOK for $drv" >&2
-
-    outPath=`sed 's/Derive(\[("out",\"\([^\"]*\)\".*/\1/' $drv`
-
-    echo "output path is $outPath" >&2
-
-    if `echo $outPath | grep -q input-1`; then
-        echo "# accept" >&2
-        read inputs
-        read outputs
-        mkdir $outPath
-        echo "BAR" > $outPath/foo
-    else
-        echo "# decline" >&2
-    fi
-
-done

tests/build-hook.sh

@@ -1,10 +0,0 @@
-source common.sh
-
-clearStore
-
-outPath=$(nix-build build-hook.nix --no-out-link --option build-hook $(pwd)/build-hook.hook.sh)
-
-echo "output path is $outPath"
-
-text=$(cat "$outPath"/foobar)
-if test "$text" != "BARBAR"; then exit 1; fi

tests/build-remote.sh

@@ -9,16 +9,11 @@ chmod -R u+w $TEST_ROOT/store0 || true
 chmod -R u+w $TEST_ROOT/store1 || true
 rm -rf $TEST_ROOT/store0 $TEST_ROOT/store1
 
-# FIXME: --option is not passed to build-remote, so have to create a config file.
-export NIX_CONF_DIR=$TEST_ROOT/etc2
-mkdir -p $NIX_CONF_DIR
-echo "
-sandbox-paths = /nix/store
-sandbox-build-dir = /build-tmp
-" > $NIX_CONF_DIR/nix.conf
+nix build -f build-hook.nix -o $TEST_ROOT/result --max-jobs 0 \
+  --sandbox-paths /nix/store --sandbox-build-dir /build-tmp \
+  --builders "local?root=$TEST_ROOT/store0; local?root=$TEST_ROOT/store1 - - 1 1 foo"
 
-outPath=$(nix-build build-hook.nix --no-out-link -j0 \
-  --option builders "local?root=$TEST_ROOT/store0; local?root=$TEST_ROOT/store1 - - 1 1 foo")
+outPath=$TEST_ROOT/result
 
 cat $outPath/foobar | grep FOOBAR
 

tests/local.mk

@@ -3,7 +3,7 @@ check:
 
 nix_tests = \
   init.sh hash.sh lang.sh add.sh simple.sh dependencies.sh \
-  build-hook.sh gc.sh gc-concurrent.sh \
+  gc.sh gc-concurrent.sh \
   referrers.sh user-envs.sh logging.sh nix-build.sh misc.sh fixed.sh \
   gc-runtime.sh check-refs.sh filter-source.sh \
   remote-store.sh export.sh export-graph.sh \