[bug#45018] [PATCH v2 2/6] daemon: Use 'Agent' to spawn 'guix substitute --query'.

[Ludovic Courtès]

* nix/libstore/local-store.hh (RunningSubstituter): Remove.
(LocalStore)[runningSubstituter]: Change to unique_ptr<Agent>.
[setSubstituterEnv, didSetSubstituterEnv]: Remove.
[getLineFromSubstituter, getIntLineFromSubstituter]: Take an 'Agent'.
* nix/libstore/local-store.cc (LocalStore::~LocalStore): Remove
reference to 'runningSubstituter'.
(LocalStore::setSubstituterEnv, LocalStore::startSubstituter): Remove.
(LocalStore::getLineFromSubstituter): Adjust to 'run' being an 'Agent'.
(LocalStore::querySubstitutablePaths): Spawn substituter agent if
needed.  Adjust to 'Agent' interface.
(LocalStore::querySubstitutablePathInfos): Likewise.
* nix/libstore/build.cc (SubstitutionGoal::tryToRun): Remove call to
'setSubstituterEnv' and add 'setenv' call for "_NIX_OPTIONS" instead.
(SubstitutionGoal::finished): Remove 'readLine' call for 'dummy'.
* guix/scripts/substitute.scm (%allow-unauthenticated-substitutes?):
Remove second argument to 'make-parameter'.
(process-query): Call 'warn-about-missing-authentication'
when (%allow-unauthenticated-substitutes?) is #t.
(guix-substitute): Wrap body in 'parameterize'.  Set 'guix-warning-port'
too.  No longer exit when 'substitute-urls' returns the empty list.  No
longer print newline initially.
* tests/substitute.scm (test-quit): Parameterize 'current-error-port' to
account for the port changes in 'guix-substitute'.
---
 guix/scripts/substitute.scm | 122 ++++++++++++++--------------
 nix/libstore/build.cc       |   6 +-
 nix/libstore/local-store.cc | 157 +++++++++---------------------------
 nix/libstore/local-store.hh |  22 +----
 tests/substitute.scm        |   3 +-
 5 files changed, 104 insertions(+), 206 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index adc6852321..c756b27999 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -124,11 +124,7 @@ disabled!~%"))
   ;; purposes, and should be avoided otherwise.
   (make-parameter
    (and=> (getenv "GUIX_ALLOW_UNAUTHENTICATED_SUBSTITUTES")
-          (cut string-ci=? <> "yes"))
-   (lambda (value)
-     (when value
-       (warn-about-missing-authentication))
-     value)))
+          (cut string-ci=? <> "yes"))))
 
 (define %narinfo-ttl
   ;; Number of seconds during which cached narinfo lookups are considered
@@ -893,6 +889,9 @@ authorized substitutes."
   (define (valid? obj)
     (valid-narinfo? obj acl))
 
+  (when (%allow-unauthenticated-substitutes?)
+    (warn-about-missing-authentication))
+
   (match (string-tokenize command)
     (("have" paths ..1)
      ;; Return the subset of PATHS available in CACHE-URLS.
@@ -1137,68 +1136,67 @@ default value."
       ((= string->number number) (> number 0))
       (_ #f)))
 
-  (mkdir-p %narinfo-cache-directory)
-  (maybe-remove-expired-cache-entries %narinfo-cache-directory
-                                      cached-narinfo-files
-                                      #:entry-expiration
-                                      cached-narinfo-expiration-time
-                                      #:cleanup-period
-                                      
%narinfo-expired-cache-entry-removal-delay)
-  (check-acl-initialized)
+  ;; The daemon's agent code opens file descriptor 4 for us and this is where
+  ;; stderr should go.
+  (parameterize ((current-error-port (match args
+                                       (("--query") (fdopen 4 "wl"))
+                                       (_ (current-error-port)))))
+    ;; Redirect diagnostics to file descriptor 4 as well.
+    (guix-warning-port (current-error-port))
 
-  ;; Starting from commit 22144afa in Nix, we are allowed to bail out directly
-  ;; when we know we cannot substitute, but we must emit a newline on stdout
-  ;; when everything is alright.
-  (when (null? (substitute-urls))
-    (exit 0))
+    (mkdir-p %narinfo-cache-directory)
+    (maybe-remove-expired-cache-entries %narinfo-cache-directory
+                                        cached-narinfo-files
+                                        #:entry-expiration
+                                        cached-narinfo-expiration-time
+                                        #:cleanup-period
+                                        
%narinfo-expired-cache-entry-removal-delay)
+    (check-acl-initialized)
 
-  ;; Say hello (see above.)
-  (newline)
-  (force-output (current-output-port))
+    ;; Sanity-check SUBSTITUTE-URLS so we can provide a meaningful error
+    ;; message.
+    (for-each validate-uri (substitute-urls))
 
-  ;; Sanity-check SUBSTITUTE-URLS so we can provide a meaningful error message.
-  (for-each validate-uri (substitute-urls))
+    ;; Attempt to install the client's locale so that messages are suitably
+    ;; translated.  LC_CTYPE must be a UTF-8 locale; it's the case by default
+    ;; so don't change it.
+    (match (or (find-daemon-option "untrusted-locale")
+               (find-daemon-option "locale"))
+      (#f     #f)
+      (locale (false-if-exception (setlocale LC_MESSAGES locale))))
 
-  ;; Attempt to install the client's locale so that messages are suitably
-  ;; translated.  LC_CTYPE must be a UTF-8 locale; it's the case by default so
-  ;; don't change it.
-  (match (or (find-daemon-option "untrusted-locale")
-             (find-daemon-option "locale"))
-    (#f     #f)
-    (locale (false-if-exception (setlocale LC_MESSAGES locale))))
+    (catch 'system-error
+      (lambda ()
+        (set-thread-name "guix substitute"))
+      (const #t))                                 ;GNU/Hurd lacks 'prctl'
 
-  (catch 'system-error
-    (lambda ()
-      (set-thread-name "guix substitute"))
-    (const #t))                                   ;GNU/Hurd lacks 'prctl'
-
-  (with-networking
-   (with-error-handling                           ; for signature errors
-     (match args
-       (("--query")
-        (let ((acl (current-acl)))
-          (let loop ((command (read-line)))
-            (or (eof-object? command)
-                (begin
-                  (process-query command
-                                 #:cache-urls (substitute-urls)
-                                 #:acl acl)
-                  (loop (read-line)))))))
-       (("--substitute" store-path destination)
-        ;; Download STORE-PATH and add store it as a Nar in file DESTINATION.
-        ;; Specify the number of columns of the terminal so the progress
-        ;; report displays nicely.
-        (parameterize ((current-terminal-columns (client-terminal-columns)))
-          (process-substitution store-path destination
-                                #:cache-urls (substitute-urls)
-                                #:acl (current-acl)
-                                #:print-build-trace? print-build-trace?)))
-       ((or ("-V") ("--version"))
-        (show-version-and-exit "guix substitute"))
-       (("--help")
-        (show-help))
-       (opts
-        (leave (G_ "~a: unrecognized options~%") opts))))))
+    (with-networking
+     (with-error-handling                         ; for signature errors
+       (match args
+         (("--query")
+          (let ((acl (current-acl)))
+            (let loop ((command (read-line)))
+              (or (eof-object? command)
+                  (begin
+                    (process-query command
+                                   #:cache-urls (substitute-urls)
+                                   #:acl acl)
+                    (loop (read-line)))))))
+         (("--substitute" store-path destination)
+          ;; Download STORE-PATH and store it as a Nar in file DESTINATION.
+          ;; Specify the number of columns of the terminal so the progress
+          ;; report displays nicely.
+          (parameterize ((current-terminal-columns (client-terminal-columns)))
+            (process-substitution store-path destination
+                                  #:cache-urls (substitute-urls)
+                                  #:acl (current-acl)
+                                  #:print-build-trace? print-build-trace?)))
+         ((or ("-V") ("--version"))
+          (show-version-and-exit "guix substitute"))
+         (("--help")
+          (show-help))
+         (opts
+          (leave (G_ "~a: unrecognized options~%") opts)))))))
 
 ;;; Local Variables:
 ;;; eval: (put 'with-timeout 'scheme-indent-function 1)
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 8413819114..7e9ab3f39c 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2986,8 +2986,6 @@ void SubstitutionGoal::tryToRun()
     if (pathExists(destPath))
         deletePath(destPath);
 
-    worker.store.setSubstituterEnv();
-
     /* Fill in the arguments. */
     Strings args;
     args.push_back("guix");
@@ -2999,6 +2997,9 @@ void SubstitutionGoal::tryToRun()
     /* Fork the substitute program. */
     pid = startProcess([&]() {
 
+       /* Communicate substitute-urls & co. to 'guix substitute'.  */
+        setenv("_NIX_OPTIONS", settings.pack().c_str(), 1);
+
         commonChildInit(logPipe);
 
         if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1)
@@ -3041,7 +3042,6 @@ void SubstitutionGoal::finished()
     logPipe.readSide.close();
 
     /* Get the hash info from stdout. */
-    string dummy = readLine(outPipe.readSide);
     string expectedHashStr = statusOk(status) ? readLine(outPipe.readSide) : 
"";
     outPipe.readSide.close();
 
diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc
index 8c479002ec..4219573a56 100644
--- a/nix/libstore/local-store.cc
+++ b/nix/libstore/local-store.cc
@@ -57,7 +57,6 @@ void checkStoreNotSymlink()
 
 
 LocalStore::LocalStore(bool reserveSpace)
-    : didSetSubstituterEnv(false)
 {
     schemaPath = settings.nixDBPath + "/schema";
 
@@ -182,21 +181,6 @@ LocalStore::LocalStore(bool reserveSpace)
 
 LocalStore::~LocalStore()
 {
-    try {
-       if (runningSubstituter) {
-           RunningSubstituter &i = *runningSubstituter;
-            if (!i.disabled) {
-               i.to.close();
-               i.from.close();
-               i.error.close();
-               if (i.pid != -1)
-                   i.pid.wait(true);
-           }
-        }
-    } catch (...) {
-        ignoreException();
-    }
-
     try {
         if (fdTempRoots != -1) {
             fdTempRoots.close();
@@ -796,96 +780,31 @@ Path LocalStore::queryPathFromHashPart(const string & 
hashPart)
     });
 }
 
-
-void LocalStore::setSubstituterEnv()
-{
-    if (didSetSubstituterEnv) return;
-
-    /* Pass configuration options (including those overridden with
-       --option) to substituters. */
-    setenv("_NIX_OPTIONS", settings.pack().c_str(), 1);
-
-    didSetSubstituterEnv = true;
-}
-
-
-void LocalStore::startSubstituter(RunningSubstituter & run)
-{
-    if (run.disabled || run.pid != -1) return;
-
-    debug(format("starting substituter program `%1% substitute'")
-         % settings.guixProgram);
-
-    Pipe toPipe, fromPipe, errorPipe;
-
-    toPipe.create();
-    fromPipe.create();
-    errorPipe.create();
-
-    setSubstituterEnv();
-
-    run.pid = startProcess([&]() {
-        if (dup2(toPipe.readSide, STDIN_FILENO) == -1)
-            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");
-        execl(settings.guixProgram.c_str(), "guix", "substitute", "--query", 
NULL);
-        throw SysError(format("executing `%1%'") % settings.guixProgram);
-    });
-
-    run.to = toPipe.writeSide.borrow();
-    run.from = run.fromBuf.fd = fromPipe.readSide.borrow();
-    run.error = errorPipe.readSide.borrow();
-
-    toPipe.readSide.close();
-    fromPipe.writeSide.close();
-    errorPipe.writeSide.close();
-
-    /* The substituter may exit right away if it's disabled in any way
-       (e.g. copy-from-other-stores.pl will exit if no other stores
-       are configured). */
-    try {
-        getLineFromSubstituter(run);
-    } catch (EndOfFile & e) {
-        run.to.close();
-        run.from.close();
-        run.error.close();
-        run.disabled = true;
-        if (run.pid.wait(true) != 0) throw;
-    }
-}
-
-
 /* Read a line from the substituter's stdout, while also processing
    its stderr. */
-string LocalStore::getLineFromSubstituter(RunningSubstituter & run)
+string LocalStore::getLineFromSubstituter(Agent & run)
 {
     string res, err;
 
-    /* We might have stdout data left over from the last time. */
-    if (run.fromBuf.hasData()) goto haveData;
-
     while (1) {
         checkInterrupt();
 
         fd_set fds;
         FD_ZERO(&fds);
-        FD_SET(run.from, &fds);
-        FD_SET(run.error, &fds);
+        FD_SET(run.fromAgent.readSide, &fds);
+        FD_SET(run.builderOut.readSide, &fds);
 
         /* Wait for data to appear on the substituter's stdout or
            stderr. */
-        if (select(run.from > run.error ? run.from + 1 : run.error + 1, &fds, 
0, 0, 0) == -1) {
+        if (select(std::max(run.fromAgent.readSide, run.builderOut.readSide) + 
1, &fds, 0, 0, 0) == -1) {
             if (errno == EINTR) continue;
             throw SysError("waiting for input from the substituter");
         }
 
         /* Completely drain stderr before dealing with stdout. */
-        if (FD_ISSET(run.error, &fds)) {
+        if (FD_ISSET(run.builderOut.readSide, &fds)) {
             char buf[4096];
-            ssize_t n = read(run.error, (unsigned char *) buf, sizeof(buf));
+            ssize_t n = read(run.builderOut.readSide, (unsigned char *) buf, 
sizeof(buf));
             if (n == -1) {
                 if (errno == EINTR) continue;
                 throw SysError("reading from substituter's stderr");
@@ -903,23 +822,20 @@ string 
LocalStore::getLineFromSubstituter(RunningSubstituter & run)
         }
 
         /* Read from stdout until we get a newline or the buffer is empty. */
-        else if (run.fromBuf.hasData() || FD_ISSET(run.from, &fds)) {
-        haveData:
-            do {
-                unsigned char c;
-                run.fromBuf(&c, 1);
-                if (c == '\n') {
-                    if (!err.empty()) printMsg(lvlError, "substitute: " + err);
-                    return res;
-                }
-                res += c;
-            } while (run.fromBuf.hasData());
+        else if (FD_ISSET(run.fromAgent.readSide, &fds)) {
+           unsigned char c;
+           readFull(run.fromAgent.readSide, (unsigned char *) &c, 1);
+           if (c == '\n') {
+               if (!err.empty()) printMsg(lvlError, "substitute: " + err);
+               return res;
+           }
+           res += c;
         }
     }
 }
 
 
-template<class T> T LocalStore::getIntLineFromSubstituter(RunningSubstituter & 
run)
+template<class T> T LocalStore::getIntLineFromSubstituter(Agent & run)
 {
     string s = getLineFromSubstituter(run);
     T res;
@@ -935,27 +851,26 @@ PathSet LocalStore::querySubstitutablePaths(const PathSet 
& paths)
     if (!settings.useSubstitutes || paths.empty()) return res;
 
     if (!runningSubstituter) {
-       std::unique_ptr<RunningSubstituter>fresh(new RunningSubstituter);
+       const Strings args = { "substitute", "--query" };
+       const std::map<string, string> env = { { "_NIX_OPTIONS", 
settings.pack() } };
+       std::unique_ptr<Agent> fresh(new Agent(settings.guixProgram, args, 
env));
        runningSubstituter.swap(fresh);
     }
 
-    RunningSubstituter & run = *runningSubstituter;
-    startSubstituter(run);
+    Agent & run = *runningSubstituter;
 
-    if (!run.disabled) {
-       string s = "have ";
-       foreach (PathSet::const_iterator, j, paths)
-           if (res.find(*j) == res.end()) { s += *j; s += " "; }
-       writeLine(run.to, s);
-       while (true) {
-           /* 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. */
-           Path path = getLineFromSubstituter(run);
-           if (path == "") break;
-           res.insert(path);
-       }
+    string s = "have ";
+    foreach (PathSet::const_iterator, j, paths)
+       if (res.find(*j) == res.end()) { s += *j; s += " "; }
+    writeLine(run.toAgent.writeSide, s);
+    while (true) {
+       /* 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. */
+       Path path = getLineFromSubstituter(run);
+       if (path == "") break;
+       res.insert(path);
     }
 
     return res;
@@ -967,18 +882,18 @@ void LocalStore::querySubstitutablePathInfos(PathSet & 
paths, SubstitutablePathI
     if (!settings.useSubstitutes) return;
 
     if (!runningSubstituter) {
-       std::unique_ptr<RunningSubstituter>fresh(new RunningSubstituter);
+       const Strings args = { "substitute", "--query" };
+       const std::map<string, string> env = { { "_NIX_OPTIONS", 
settings.pack() } };
+       std::unique_ptr<Agent> fresh(new Agent(settings.guixProgram, args, 
env));
        runningSubstituter.swap(fresh);
     }
 
-    RunningSubstituter & run = *runningSubstituter;
-    startSubstituter(run);
-    if (run.disabled) return;
+    Agent & run = *runningSubstituter;
 
     string s = "info ";
     foreach (PathSet::const_iterator, i, paths)
         if (infos.find(*i) == infos.end()) { s += *i; s += " "; }
-    writeLine(run.to, s);
+    writeLine(run.toAgent.writeSide, s);
 
     while (true) {
         Path path = getLineFromSubstituter(run);
diff --git a/nix/libstore/local-store.hh b/nix/libstore/local-store.hh
index 2e48cf03e6..57d15bac7e 100644
--- a/nix/libstore/local-store.hh
+++ b/nix/libstore/local-store.hh
@@ -38,21 +38,11 @@ struct OptimiseStats
 };
 
 
-struct RunningSubstituter
-{
-    Pid pid;
-    AutoCloseFD to, from, error;
-    FdSource fromBuf;
-    bool disabled;
-    RunningSubstituter() : disabled(false) { };
-};
-
-
 class LocalStore : public StoreAPI
 {
 private:
     /* The currently running substituter or empty.  */
-    std::unique_ptr<RunningSubstituter> runningSubstituter;
+    std::unique_ptr<Agent> runningSubstituter;
 
     Path linksDir;
 
@@ -178,8 +168,6 @@ public:
 
     void markContentsGood(const Path & path);
 
-    void setSubstituterEnv();
-
     void createUser(const std::string & userName, uid_t userId);
 
 private:
@@ -213,8 +201,6 @@ private:
     /* Cache for pathContentsGood(). */
     std::map<Path, bool> pathContentsGoodCache;
 
-    bool didSetSubstituterEnv;
-
     /* The file to which we write our temporary roots. */
     Path fnTempRoots;
     AutoCloseFD fdTempRoots;
@@ -262,11 +248,9 @@ private:
 
     void removeUnusedLinks(const GCState & state);
 
-    void startSubstituter(RunningSubstituter & runningSubstituter);
+    string getLineFromSubstituter(Agent & run);
 
-    string getLineFromSubstituter(RunningSubstituter & run);
-
-    template<class T> T getIntLineFromSubstituter(RunningSubstituter & run);
+    template<class T> T getIntLineFromSubstituter(Agent & run);
 
     Path createTempDirInStore();
 
diff --git a/tests/substitute.scm b/tests/substitute.scm
index 6560612c40..bd5b6305b0 100644
--- a/tests/substitute.scm
+++ b/tests/substitute.scm
@@ -47,7 +47,8 @@ it writes to GUIX-WARNING-PORT a messages that matches 
ERROR-RX."
   (test-equal name
     '(1 #t)
     (let ((error-output (open-output-string)))
-      (parameterize ((guix-warning-port error-output))
+      (parameterize ((current-error-port error-output)
+                     (guix-warning-port error-output))
         (catch 'quit
           (lambda ()
             exp
-- 
2.29.2