* Simplify communication with the hook a bit (don't use file

descriptors 3/4, just use stdin/stderr).

doc/manual/env-common.xml

@@ -151,12 +151,12 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
   <para>On the basis of this information, and whatever persistent
   state the build hook keeps about other machines and their current
   load, it has to decide what to do with the build.  It should print
-  out on file descriptor 3 one of the following responses (terminated
+  out on standard error one of the following responses (terminated by
-  by a newline, <literal>"\n"</literal>):
+  a newline, <literal>"\n"</literal>):
 
   <variablelist>
 
-    <varlistentry><term><literal>decline</literal></term>
+    <varlistentry><term><literal># decline</literal></term>
 
       <listitem><para>The build hook is not willing or able to perform
       the build; the calling Nix process should do the build itself,
@@ -164,7 +164,7 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
 
     </varlistentry>
 
-    <varlistentry><term><literal>postpone</literal></term>
+    <varlistentry><term><literal># postpone</literal></term>
 
       <listitem><para>The build hook cannot perform the build now, but
       can do so in the future (e.g., because all available build slots
@@ -174,7 +174,7 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
 
     </varlistentry>
 
-    <varlistentry><term><literal>accept</literal></term>
+    <varlistentry><term><literal># accept</literal></term>
 
       <listitem><para>The build hook has accepted the
       build.</para></listitem>
@@ -185,37 +185,12 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
 
   </para>
 
-  <para>If the build hook accepts the build, it is possible that it is
+  <para>After sending <literal># accept</literal>, the hook should
-  no longer necessary to do the build because some other process has
+  read one line from standard input, which will be the string
-  performed the build in the meantime.  To prevent races, the hook
+  <literal>okay</literal>.  It can then proceed with the build.
-  must read from file descriptor 4 a single line that tells it whether
+  Before sending <literal>okay</literal>, Nix will store in the hook’s
-  to continue:
+  current directory a number of text files that contain information
-
+  about the derivation:
-  <variablelist>
-
-    <varlistentry><term><literal>cancel</literal></term>
-
-      <listitem><para>The build has already been done, so the hook
-      should exit.</para></listitem>
-
-    </varlistentry>
-  
-    <varlistentry><term><literal>okay</literal></term>
-
-      <listitem><para>The hook should proceed with the build.  At this
-      point, the calling Nix process has acquired locks on the output
-      path, so no other Nix process will perform the
-      build.</para></listitem>
-
-    </varlistentry>
-
-  </variablelist>
-
-  </para>
-
-  <para>If the hook has been told to proceed, Nix will store in the
-  hook’s current directory a number of text files that contain
-  information about the derivation:
 
   <variablelist>
 
@@ -255,7 +230,9 @@ $ mount -o bind /mnt/otherdisk/nix /nix</screen>
   <para>The hook should copy the inputs to the remote machine,
   register the validity of the inputs, perform the remote build, and
   copy the outputs back to the local machine.  An exit code other than
-  <literal>0</literal> indicates that the hook has failed.</para>
+  <literal>0</literal> indicates that the hook has failed.  An exit
+  code equal to 100 means that the remote build failed (as opposed to,
+  e.g., a network error).</para>
 
   </listitem>
 

scripts/build-remote.pl.in

@@ -29,9 +29,7 @@ $maxSilentTime = 0 unless defined $maxSilentTime;
 
 sub sendReply {
     my $reply = shift;
-    open OUT, ">&3" or die;
+    print STDERR "# $reply\n";
-    print OUT "$reply\n";
-    close OUT;
 }
 
 sub decline {
@@ -121,11 +119,8 @@ if (!defined $machine) {
 
 # Yes we did, accept.
 sendReply "accept";
-open IN, "<&4" or die;
+my $x = <STDIN>;
-my $x = <IN>;
 chomp $x;
-#print "got $x\n";  
-close IN;
 
 if ($x ne "okay") {
     exit 0;

src/libstore/build.cc

@@ -660,7 +660,6 @@ private:
 
     /* Pipes for talking to the build hook (if any). */
     Pipe toHook;
-    Pipe fromHook;
 
     /* Whether we're currently doing a chroot build. */
     bool useChroot;
@@ -1207,49 +1206,6 @@ void DerivationGoal::buildDone()
 }
 
 
-static string readLine(int fd)
-{
-    string s;
-    while (1) {
-        checkInterrupt();
-        char ch;
-        ssize_t rd = read(fd, &ch, 1);
-        if (rd == -1) {
-            if (errno != EINTR)
-                throw SysError("reading a line");
-        } else if (rd == 0)
-            throw Error("unexpected EOF reading a line");
-        else {
-            if (ch == '\n') return s;
-            s += ch;
-        }
-    }
-}
-
-
-static void writeLine(int fd, string s)
-{
-    s += '\n';
-    writeFull(fd, (const unsigned char *) s.c_str(), s.size());
-}
-
-
-/* !!! ugly hack */
-static void drain(int fd)
-{
-    unsigned char buffer[1024];
-    while (1) {
-        checkInterrupt();
-        ssize_t rd = read(fd, buffer, sizeof buffer);
-        if (rd == -1) {
-            if (errno != EINTR)
-                throw SysError("draining");
-        } else if (rd == 0) break;
-        else writeToStderr(buffer, rd);
-    }
-}
-
-
 DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 {
     if (!useBuildHook) return rpDecline;
@@ -1266,7 +1222,6 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
     /* Create the communication pipes. */
     toHook.create();
-    fromHook.create();
 
     /* Fork the hook. */
     pid = fork();
@@ -1310,16 +1265,20 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
     worker.childStarted(shared_from_this(),
         pid, singleton<set<int> >(logPipe.readSide), false);
 
-    fromHook.writeSide.close();
     toHook.readSide.close();
 
     /* Read the first line of input, which should be a word indicating
-       whether the hook wishes to perform the build.  !!! potential
+       whether the hook wishes to perform the build. */
-       for deadlock here: we should also read from the child's logger
-       pipe. */
     string reply;
     try {
-        reply = readLine(fromHook.readSide);
+        while (true) {
+            string s = readLine(logPipe.readSide);
+            if (string(s, 0, 2) == "# ") {
+                reply = string(s, 2);
+                break;
+            }
+            handleChildOutput(logPipe.readSide, s + "\n");
+        }
     } catch (Error & e) {
         terminateBuildHook(true);
         throw;
@@ -1335,7 +1294,7 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
     else if (reply == "accept") {
 
-        printMsg(lvlInfo, format("running hook to build path(s) %1%")
+        printMsg(lvlInfo, format("using hook to build path(s) %1%")
             % showPaths(outputPaths(drv.outputs)));
         
         /* Write the information that the hook needs to perform the
@@ -1379,11 +1338,11 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
         /* Tell the hook to proceed. */
         writeLine(toHook.writeSide, "okay");
+        toHook.writeSide.close();
 
-        if (printBuildTrace) {
+        if (printBuildTrace)
             printMsg(lvlError, format("@ build-started %1% %2% %3% %4%")
                 % drvPath % drv.outputs["out"].path % drv.platform % logFile);
-        }
         
         return rpAccept;
     }
@@ -1394,7 +1353,6 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
 void DerivationGoal::terminateBuildHook(bool kill)
 {
-    /* !!! drain stdout of hook */
     debug("terminating build hook");
     pid_t savedPid = pid;
     if (kill)
@@ -1404,10 +1362,8 @@ void DerivationGoal::terminateBuildHook(bool kill)
     /* `false' means don't wake up waiting goals, since we want to
        keep this build slot ourselves. */
     worker.childTerminated(savedPid, false);
-    fromHook.readSide.close();
     toHook.writeSide.close();
     fdLogFile.close();
-    drain(logPipe.readSide);
     logPipe.readSide.close();
     deleteTmpDir(true); /* get rid of the hook's temporary directory */
 }
@@ -1993,24 +1949,14 @@ void DerivationGoal::initChild()
         throw SysError(format("changing into `%1%'") % tmpDir);
 
     /* When running a hook, dup the communication pipes. */
-    bool inHook = fromHook.writeSide.isOpen();
+    if (usingBuildHook) {
-    if (inHook) {
-        fromHook.readSide.close();
-        if (dup2(fromHook.writeSide, 3) == -1)
-            throw SysError("dupping from-hook write side");
-
         toHook.writeSide.close();
-        if (dup2(toHook.readSide, 4) == -1)
+        if (dup2(toHook.readSide, STDIN_FILENO) == -1)
             throw SysError("dupping to-hook read side");
     }
 
     /* Close all other file descriptors. */
-    set<int> exceptions;
+    closeMostFDs(set<int>());
-    if (inHook) {
-        exceptions.insert(3);
-        exceptions.insert(4);
-    }
-    closeMostFDs(exceptions);
 }
 
 

src/libutil/util.cc

@@ -229,6 +229,33 @@ void writeFile(const Path & path, const string & s)
 }
 
 
+string readLine(int fd)
+{
+    string s;
+    while (1) {
+        checkInterrupt();
+        char ch;
+        ssize_t rd = read(fd, &ch, 1);
+        if (rd == -1) {
+            if (errno != EINTR)
+                throw SysError("reading a line");
+        } else if (rd == 0)
+            throw Error("unexpected EOF reading a line");
+        else {
+            if (ch == '\n') return s;
+            s += ch;
+        }
+    }
+}
+
+
+void writeLine(int fd, string s)
+{
+    s += '\n';
+    writeFull(fd, (const unsigned char *) s.c_str(), s.size());
+}
+
+
 static void _computePathSize(const Path & path,
     unsigned long long & bytes, unsigned long long & blocks)
 {

src/libutil/util.hh

@@ -60,6 +60,12 @@ string readFile(const Path & path);
 /* Write a string to a file. */
 void writeFile(const Path & path, const string & s);
 
+/* Read a line from a file descriptor. */
+string readLine(int fd);
+
+/* Write a line to a file descriptor. */
+void writeLine(int fd, string s);
+
 /* Compute the sum of the sizes of all files in `path'. */
 void computePathSize(const Path & path,
     unsigned long long & bytes, unsigned long long & blocks);

tests/build-hook.hook.sh

@@ -11,11 +11,11 @@ outPath=$(sed 's/Derive(\[("out",\"\([^\"]*\)\".*/\1/' $drv)
 echo "output path is $outPath" >&2
 
 if $(echo $outPath | grep -q input-1); then
-    echo "accept" >&3
+    echo "# accept" >&2
-    read x <&4
+    read x
     echo "got $x"
     mkdir $outPath
     echo "BAR" > $outPath/foo
 else
-    echo "decline" >&3
+    echo "# decline" >&2
 fi