From c770a2422a47526d5eb336af6af4292df68dad2b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 Aug 2012 11:19:24 -0400 Subject: [PATCH] Report substituter errors to clients of the Nix daemon --- scripts/download-from-binary-cache.pl.in | 10 ++-- src/libstore/local-store.cc | 58 +++++++++++++++--------- src/libstore/local-store.hh | 2 +- src/libutil/util.cc | 9 +++- src/libutil/util.hh | 4 ++ 5 files changed, 53 insertions(+), 30 deletions(-) diff --git a/scripts/download-from-binary-cache.pl.in b/scripts/download-from-binary-cache.pl.in index 3f7d3ef45..94c446e37 100644 --- a/scripts/download-from-binary-cache.pl.in +++ b/scripts/download-from-binary-cache.pl.in @@ -184,13 +184,9 @@ sub getAvailableCaches { my @trustedUrls = (@urls, strToList($Nix::Config::config{"trusted-binary-caches"} // "")); @urls = (); foreach my $url (@untrustedUrls) { - if (any { $url eq $_ } @trustedUrls) { - push @urls, $url; - } else { - # FIXME: should die here, but we currently can't - # deliver error messages to clients. - warn "warning: binary cache ‘$url’ is not trusted (please add it to ‘trusted-binary-caches’ in $Nix::Config::confDir/nix.conf)\n"; - } + die "binary cache ‘$url’ is not trusted (please add it to ‘trusted-binary-caches’ in $Nix::Config::confDir/nix.conf)\n" + unless any { $url eq $_ } @trustedUrls; + push @urls, $url; } } diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index f20324d4e..bd63ce55d 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -909,10 +909,11 @@ void LocalStore::startSubstituter(const Path & substituter, RunningSubstituter & debug(format("starting substituter program `%1%'") % substituter); - Pipe toPipe, fromPipe; + Pipe toPipe, fromPipe, errorPipe; toPipe.create(); fromPipe.create(); + errorPipe.create(); run.pid = fork(); @@ -940,6 +941,8 @@ void LocalStore::startSubstituter(const Path & substituter, RunningSubstituter & throw SysError("dupping stdin"); if (dup2(fromPipe.writeSide, STDOUT_FILENO) == -1) throw SysError("dupping stdout"); + if (dup2(errorPipe.writeSide, STDERR_FILENO) == -1) + throw SysError("dupping stderr"); closeMostFDs(set()); execl(substituter.c_str(), substituter.c_str(), "--query", NULL); throw SysError(format("executing `%1%'") % substituter); @@ -953,6 +956,7 @@ void LocalStore::startSubstituter(const Path & substituter, RunningSubstituter & run.to = toPipe.writeSide.borrow(); run.from = fromPipe.readSide.borrow(); + run.error = errorPipe.readSide.borrow(); } @@ -973,13 +977,21 @@ PathSet LocalStore::querySubstitutablePaths(const PathSet & paths) RunningSubstituter & run(runningSubstituters[*i]); startSubstituter(*i, run); string s = "have "; - foreach (PathSet::const_iterator, i, paths) - if (res.find(*i) == res.end()) { s += *i; s += " "; } + foreach (PathSet::const_iterator, j, paths) + if (res.find(*j) == res.end()) { s += *j; s += " "; } writeLine(run.to, s); while (true) { - Path path = readLine(run.from); - if (path == "") break; - res.insert(path); + /* FIXME: we only read stderr when an error occurs, so + substituters should only write (short) messages to + stderr when they fail. I.e. they shouldn't write debug + output. */ + try { + Path path = readLine(run.from); + if (path == "") break; + res.insert(path); + } catch (EndOfFile e) { + throw Error(format("substituter `%1%' failed: %2%") % *i % chomp(drainFD(run.error))); + } } } return res; @@ -998,22 +1010,26 @@ void LocalStore::querySubstitutablePathInfos(const Path & substituter, writeLine(run.to, s); while (true) { - Path path = readLine(run.from); - if (path == "") break; - if (paths.find(path) == paths.end()) - throw Error(format("got unexpected path `%1%' from substituter") % path); - paths.erase(path); - SubstitutablePathInfo & info(infos[path]); - info.deriver = readLine(run.from); - if (info.deriver != "") assertStorePath(info.deriver); - int nrRefs = getIntLine(run.from); - while (nrRefs--) { - Path p = readLine(run.from); - assertStorePath(p); - info.references.insert(p); + try { + Path path = readLine(run.from); + if (path == "") break; + if (paths.find(path) == paths.end()) + throw Error(format("got unexpected path `%1%' from substituter") % path); + paths.erase(path); + SubstitutablePathInfo & info(infos[path]); + info.deriver = readLine(run.from); + if (info.deriver != "") assertStorePath(info.deriver); + int nrRefs = getIntLine(run.from); + while (nrRefs--) { + Path p = readLine(run.from); + assertStorePath(p); + info.references.insert(p); + } + info.downloadSize = getIntLine(run.from); + info.narSize = getIntLine(run.from); + } catch (EndOfFile e) { + throw Error(format("substituter `%1%' failed: %2%") % substituter % chomp(drainFD(run.error))); } - info.downloadSize = getIntLine(run.from); - info.narSize = getIntLine(run.from); } } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 7cf9fc18d..3cb016e9c 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -45,7 +45,7 @@ struct OptimiseStats struct RunningSubstituter { Pid pid; - AutoCloseFD to, from; + AutoCloseFD to, from, error; }; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 689fc543a..086574058 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -253,7 +253,7 @@ string readLine(int fd) if (errno != EINTR) throw SysError("reading a line"); } else if (rd == 0) - throw Error("unexpected EOF reading a line"); + throw EndOfFile("unexpected EOF reading a line"); else { if (ch == '\n') return s; s += ch; @@ -1015,6 +1015,13 @@ string concatStringsSep(const string & sep, const Strings & ss) } +string chomp(const string & s) +{ + size_t i = s.find_last_not_of(" \n\r\t"); + return i == string::npos ? "" : string(s, 0, i); +} + + string statusToString(int status) { if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 9b8656f70..16633a083 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -292,6 +292,10 @@ Strings tokenizeString(const string & s, const string & separators = " \t\n\r"); string concatStringsSep(const string & sep, const Strings & ss); +/* Remove trailing whitespace from a string. */ +string chomp(const string & s); + + /* Convert the exit status of a child as returned by wait() into an error string. */ string statusToString(int status);