From 3a2bbe7f8ad7ec8b2896ff5e666b8f5525691c6f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sat, 28 Mar 2009 19:29:55 +0000 Subject: [PATCH] * Simplify communication with the hook a bit (don't use file descriptors 3/4, just use stdin/stderr). --- doc/manual/env-common.xml | 51 +++++++--------------- scripts/build-remote.pl.in | 9 +--- src/libstore/build.cc | 86 +++++++------------------------------- src/libutil/util.cc | 27 ++++++++++++ src/libutil/util.hh | 6 +++ tests/build-hook.hook.sh | 6 +-- 6 files changed, 68 insertions(+), 117 deletions(-) diff --git a/doc/manual/env-common.xml b/doc/manual/env-common.xml index d6ebbf654..d67ef714d 100644 --- a/doc/manual/env-common.xml +++ b/doc/manual/env-common.xml @@ -151,12 +151,12 @@ $ mount -o bind /mnt/otherdisk/nix /nix 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 - by a newline, "\n"): + out on standard error one of the following responses (terminated by + a newline, "\n"): - decline + # decline 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 - postpone + # postpone 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 - accept + # accept The build hook has accepted the build. @@ -185,37 +185,12 @@ $ mount -o bind /mnt/otherdisk/nix /nix - If the build hook accepts the build, it is possible that it is - no longer necessary to do the build because some other process has - performed the build in the meantime. To prevent races, the hook - must read from file descriptor 4 a single line that tells it whether - to continue: - - - - cancel - - The build has already been done, so the hook - should exit. - - - - okay - - 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. - - - - - - - - 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: + After sending # accept, the hook should + read one line from standard input, which will be the string + okay. It can then proceed with the build. + Before sending okay, Nix will store in the hook’s + current directory a number of text files that contain information + about the derivation: @@ -255,7 +230,9 @@ $ mount -o bind /mnt/otherdisk/nix /nix 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 - 0 indicates that the hook has failed. + 0 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). diff --git a/scripts/build-remote.pl.in b/scripts/build-remote.pl.in index 1c4f97440..91b57d5f2 100755 --- a/scripts/build-remote.pl.in +++ b/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 OUT "$reply\n"; - close OUT; + print STDERR "# $reply\n"; } sub decline { @@ -121,11 +119,8 @@ if (!defined $machine) { # Yes we did, accept. sendReply "accept"; -open IN, "<&4" or die; -my $x = ; +my $x = ; chomp $x; -#print "got $x\n"; -close IN; if ($x ne "okay") { exit 0; diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 92bf1cab0..12061bee7 100644 --- a/src/libstore/build.cc +++ b/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 >(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 - for deadlock here: we should also read from the child's logger - pipe. */ + whether the hook wishes to perform the build. */ 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 @@ -1377,13 +1336,13 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook() writeStringToFile(referencesFN, makeValidityRegistration(allInputs, true, false)); - /* Tell the hook to proceed. */ + /* 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 (inHook) { - fromHook.readSide.close(); - if (dup2(fromHook.writeSide, 3) == -1) - throw SysError("dupping from-hook write side"); - + if (usingBuildHook) { 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 exceptions; - if (inHook) { - exceptions.insert(3); - exceptions.insert(4); - } - closeMostFDs(exceptions); + closeMostFDs(set()); } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 02dd53a17..1cb94215e 100644 --- a/src/libutil/util.cc +++ b/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) { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 33b06ca95..5744e5692 100644 --- a/src/libutil/util.hh +++ b/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); diff --git a/tests/build-hook.hook.sh b/tests/build-hook.hook.sh index 45536f8de..18eba283e 100755 --- a/tests/build-hook.hook.sh +++ b/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 - read x <&4 + echo "# accept" >&2 + read x echo "got $x" mkdir $outPath echo "BAR" > $outPath/foo else - echo "decline" >&3 + echo "# decline" >&2 fi