Report substituter errors to clients of the Nix daemon

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 3f7d3ef45f4a..94c446e37a4c 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 f20324d4e3d0..bd63ce55d095 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<int>());
             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<int>(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<int>(run.from);
+            while (nrRefs--) {
+                Path p = readLine(run.from);
+                assertStorePath(p);
+                info.references.insert(p);
+            }
+            info.downloadSize = getIntLine<long long>(run.from);
+            info.narSize = getIntLine<long long>(run.from);
+        } catch (EndOfFile e) {
+            throw Error(format("substituter `%1%' failed: %2%") % substituter % chomp(drainFD(run.error)));
         }
-        info.downloadSize = getIntLine<long long>(run.from);
-        info.narSize = getIntLine<long long>(run.from);
     }
 }
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 7cf9fc18d207..3cb016e9cafd 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 689fc543af31..086574058aff 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 9b8656f70485..16633a0835a3 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);