[bug#37302] [PATCH 5/7] daemon: Run 'guix substitute' directly and assume a single substituter.

["Ludovic Courtès" <ludo@gnu.org>]

The daemon had a mechanism that allows it to handle a list of
substituters and try them sequentially; this removes it.

* nix/scripts/substitute.in: Remove.
* nix/local.mk (nodist_pkglibexec_SCRIPTS): Remove.
* config-daemon.ac: Don't output 'nix/scripts/substitute'.
* nix/libstore/build.cc (SubstitutionGoal)[subs, sub, hasSubstitute]:
Remove.
[tryNext]: Make private.
(SubstitutionGoal::SubstitutionGoal, SubstitutionGoal::init): Remove now
unneeded initializers.
(SubstitutionGoal::tryNext): Adjust to assume a single substituter: call
'amDone' upfront when we couldn't find substitutes.
(SubstitutionGoal::tryToRun): Adjust to run 'guix substitute' via
'settings.guixProgram'.
(SubstitutionGoal::finished): Call 'amDone(ecFailed)' upon failure
instead of setting 'state' to 'tryNext'.
* nix/libstore/globals.hh (Settings)[substituters]: Remove.
* nix/libstore/local-store.cc (LocalStore::~LocalStore): Adjust to
handle a single substituter.
(LocalStore::startSubstituter): Remove 'path' parameter.  Adjust to
invoke 'settings.guixProgram'.  Don't refer to 'run.program', which no
longer exists.
(LocalStore::querySubstitutablePaths): Adjust for 'runningSubstituters'
being a singleton instead of a list.
(LocalStore::querySubstitutablePathInfos): Likewise, and remove
'substituter' parameter.
* nix/libstore/local-store.hh (RunningSubstituter)[program]: Remove.
(LocalStore)[runningSubstituters]: Remove.
[runningSubstituter]: New field.
[querySubstitutablePathInfos]: Remove 'substituter' parameter.
[startSubstituter]: Remove 'substituter' parameter.
* nix/nix-daemon/guix-daemon.cc (main): Remove references to
'settings.substituters'.
* nix/nix-daemon/nix-daemon.cc (performOp): Ignore the user's
"build-use-substitutes" value when 'settings.useSubstitutes' is false.
---
 config-daemon.ac              |  2 -
 nix/libstore/build.cc         | 52 +++++++------------
 nix/libstore/globals.hh       |  5 --
 nix/libstore/local-store.cc   | 97 +++++++++++++++++++----------------
 nix/libstore/local-store.hh   | 12 ++---
 nix/local.mk                  |  3 --
 nix/nix-daemon/guix-daemon.cc | 11 +---
 nix/nix-daemon/nix-daemon.cc  |  8 ++-
 nix/scripts/substitute.in     | 11 ----
 9 files changed, 85 insertions(+), 116 deletions(-)
 delete mode 100644 nix/scripts/substitute.in

diff --git a/config-daemon.ac b/config-daemon.ac
index 3d92e8f778..bf94815966 100644
--- a/config-daemon.ac
+++ b/config-daemon.ac
@@ -148,8 +148,6 @@ if test "x$guix_build_daemon" = "xyes"; then
   AC_SUBST([GUIX_TEST_ROOT])
 
   GUIX_CHECK_LOCALSTATEDIR
-  AC_CONFIG_FILES([nix/scripts/substitute],
-    [chmod +x nix/scripts/substitute])
 fi
 
 AM_CONDITIONAL([HAVE_LIBBZ2], [test "x$HAVE_LIBBZ2" = "xyes"])
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 9f1f88933a..ad53b81413 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -2863,15 +2863,6 @@ private:
     /* The store path that should be realised through a substitute. */
     Path storePath;
 
-    /* The remaining substituters. */
-    Paths subs;
-
-    /* The current substituter. */
-    Path sub;
-
-    /* Whether any substituter can realise this path */
-    bool hasSubstitute;
-
     /* Path info returned by the substituter's query info operation. */
     SubstitutablePathInfo info;
 
@@ -2897,6 +2888,8 @@ private:
     typedef void (SubstitutionGoal::*GoalState)();
     GoalState state;
 
+    void tryNext();
+
 public:
     SubstitutionGoal(const Path & storePath, Worker & worker, bool repair = false);
     ~SubstitutionGoal();
@@ -2914,7 +2907,6 @@ public:
 
     /* The states. */
     void init();
-    void tryNext();
     void gotInfo();
     void referencesValid();
     void tryToRun();
@@ -2930,7 +2922,6 @@ public:
 
 SubstitutionGoal::SubstitutionGoal(const Path & storePath, Worker & worker, bool repair)
     : Goal(worker)
-    , hasSubstitute(false)
     , repair(repair)
 {
     this->storePath = storePath;
@@ -2980,37 +2971,31 @@ void SubstitutionGoal::init()
     if (settings.readOnlyMode)
         throw Error(format("cannot substitute path `%1%' - no write access to the store") % storePath);
 
-    subs = settings.substituters;
-
     tryNext();
 }
 
 
 void SubstitutionGoal::tryNext()
 {
-    trace("trying next substituter");
+    trace("trying substituter");
 
-    if (subs.size() == 0) {
+    SubstitutablePathInfos infos;
+    PathSet dummy(singleton<PathSet>(storePath));
+    worker.store.querySubstitutablePathInfos(dummy, infos);
+    SubstitutablePathInfos::iterator k = infos.find(storePath);
+    if (k == infos.end()) {
         /* None left.  Terminate this goal and let someone else deal
            with it. */
         debug(format("path `%1%' is required, but there is no substituter that can build it") % storePath);
         /* Hack: don't indicate failure if there were no substituters.
            In that case the calling derivation should just do a
            build. */
-        amDone(hasSubstitute ? ecFailed : ecNoSubstituters);
-        return;
+        amDone(ecNoSubstituters);
+	return;
     }
 
-    sub = subs.front();
-    subs.pop_front();
-
-    SubstitutablePathInfos infos;
-    PathSet dummy(singleton<PathSet>(storePath));
-    worker.store.querySubstitutablePathInfos(sub, dummy, infos);
-    SubstitutablePathInfos::iterator k = infos.find(storePath);
-    if (k == infos.end()) { tryNext(); return; }
+    /* Found a substitute.  */
     info = k->second;
-    hasSubstitute = true;
 
     /* To maintain the closure invariant, we first have to realise the
        paths referenced by this one. */
@@ -3098,7 +3083,8 @@ void SubstitutionGoal::tryToRun()
 
     /* Fill in the arguments. */
     Strings args;
-    args.push_back(baseNameOf(sub));
+    args.push_back("guix");
+    args.push_back("substitute");
     args.push_back("--substitute");
     args.push_back(storePath);
     args.push_back(destPath);
@@ -3111,9 +3097,9 @@ void SubstitutionGoal::tryToRun()
         if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1)
             throw SysError("cannot dup output pipe into stdout");
 
-        execv(sub.c_str(), stringsToCharPtrs(args).data());
+        execv(settings.guixProgram.c_str(), stringsToCharPtrs(args).data());
 
-        throw SysError(format("executing `%1%'") % sub);
+        throw SysError(format("executing `%1% substitute'") % settings.guixProgram);
     });
 
     pid.setSeparatePG(true);
@@ -3126,7 +3112,9 @@ void SubstitutionGoal::tryToRun()
     state = &SubstitutionGoal::finished;
 
     if (settings.printBuildTrace)
-        printMsg(lvlError, format("@ substituter-started %1% %2%") % storePath % sub);
+	/* The second element in the message used to be the name of the
+	   substituter but we're left with only one.  */
+        printMsg(lvlError, format("@ substituter-started %1% substitute") % storePath);
 }
 
 
@@ -3192,9 +3180,7 @@ void SubstitutionGoal::finished()
                 % storePath % status % e.msg());
         }
 
-        /* Try the next substitute. */
-        state = &SubstitutionGoal::tryNext;
-        worker.wakeUp(shared_from_this());
+	amDone(ecFailed);
         return;
     }
 
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index 0d9315a41a..0069c85956 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -115,11 +115,6 @@ struct Settings {
        means infinity.  */
     time_t buildTimeout;
 
-    /* The substituters.  There are programs that can somehow realise
-       a store path without building, e.g., by downloading it or
-       copying it from a CD. */
-    Paths substituters;
-
     /* Whether to use build hooks (for distributed builds).  Sometimes
        users want to disable this from the command-line. */
     bool useBuildHook;
diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc
index 951c35faf3..3b08492c64 100644
--- a/nix/libstore/local-store.cc
+++ b/nix/libstore/local-store.cc
@@ -184,13 +184,15 @@ LocalStore::LocalStore(bool reserveSpace)
 LocalStore::~LocalStore()
 {
     try {
-        foreach (RunningSubstituters::iterator, i, runningSubstituters) {
-            if (i->second.disabled) continue;
-            i->second.to.close();
-            i->second.from.close();
-            i->second.error.close();
-            if (i->second.pid != -1)
-                i->second.pid.wait(true);
+	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();
@@ -808,11 +810,12 @@ void LocalStore::setSubstituterEnv()
 }
 
 
-void LocalStore::startSubstituter(const Path & substituter, RunningSubstituter & run)
+void LocalStore::startSubstituter(RunningSubstituter & run)
 {
     if (run.disabled || run.pid != -1) return;
 
-    debug(format("starting substituter program `%1%'") % substituter);
+    debug(format("starting substituter program `%1% substitute'")
+	  % settings.guixProgram);
 
     Pipe toPipe, fromPipe, errorPipe;
 
@@ -829,11 +832,10 @@ void LocalStore::startSubstituter(const Path & substituter, RunningSubstituter &
             throw SysError("dupping stdout");
         if (dup2(errorPipe.writeSide, STDERR_FILENO) == -1)
             throw SysError("dupping stderr");
-        execl(substituter.c_str(), substituter.c_str(), "--query", NULL);
-        throw SysError(format("executing `%1%'") % substituter);
+        execl(settings.guixProgram.c_str(), "guix", "substitute", "--query", NULL);
+        throw SysError(format("executing `%1%'") % settings.guixProgram);
     });
 
-    run.program = baseNameOf(substituter);
     run.to = toPipe.writeSide.borrow();
     run.from = run.fromBuf.fd = fromPipe.readSide.borrow();
     run.error = errorPipe.readSide.borrow();
@@ -889,13 +891,14 @@ string LocalStore::getLineFromSubstituter(RunningSubstituter & run)
                 if (errno == EINTR) continue;
                 throw SysError("reading from substituter's stderr");
             }
-            if (n == 0) throw EndOfFile(format("substituter `%1%' died unexpectedly") % run.program);
+            if (n == 0) throw EndOfFile(format("`%1% substitute' died unexpectedly")
+					% settings.guixProgram);
             err.append(buf, n);
             string::size_type p;
             while (((p = err.find('\n')) != string::npos)
 		   || ((p = err.find('\r')) != string::npos)) {
 	        string thing(err, 0, p + 1);
-	        writeToStderr(run.program + ": " + thing);
+	        writeToStderr("substitute: " + thing);
                 err = string(err, p + 1);
             }
         }
@@ -907,7 +910,7 @@ string LocalStore::getLineFromSubstituter(RunningSubstituter & run)
                 unsigned char c;
                 run.fromBuf(&c, 1);
                 if (c == '\n') {
-                    if (!err.empty()) printMsg(lvlError, run.program + ": " + err);
+                    if (!err.empty()) printMsg(lvlError, "substitute: " + err);
                     return res;
                 }
                 res += c;
@@ -930,38 +933,47 @@ PathSet LocalStore::querySubstitutablePaths(const PathSet & paths)
 {
     PathSet res;
 
-    if (!settings.useSubstitutes) return res;
-
-    foreach (Paths::iterator, i, settings.substituters) {
-        if (res.size() == paths.size()) break;
-        RunningSubstituter & run(runningSubstituters[*i]);
-        startSubstituter(*i, run);
-        if (run.disabled) continue;
-        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);
-        }
+    if (!settings.useSubstitutes || paths.empty()) return res;
+
+    if (!runningSubstituter) {
+	std::unique_ptr<RunningSubstituter>fresh(new RunningSubstituter);
+	runningSubstituter.swap(fresh);
+    }
+
+    RunningSubstituter & run = *runningSubstituter;
+    startSubstituter(run);
+
+    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);
+	}
     }
+
     return res;
 }
 
 
-void LocalStore::querySubstitutablePathInfos(const Path & substituter,
-    PathSet & paths, SubstitutablePathInfos & infos)
+void LocalStore::querySubstitutablePathInfos(PathSet & paths, SubstitutablePathInfos & infos)
 {
     if (!settings.useSubstitutes) return;
 
-    RunningSubstituter & run(runningSubstituters[substituter]);
-    startSubstituter(substituter, run);
+    if (!runningSubstituter) {
+	std::unique_ptr<RunningSubstituter>fresh(new RunningSubstituter);
+	runningSubstituter.swap(fresh);
+    }
+
+    RunningSubstituter & run = *runningSubstituter;
+    startSubstituter(run);
     if (run.disabled) return;
 
     string s = "info ";
@@ -993,10 +1005,9 @@ void LocalStore::querySubstitutablePathInfos(const Path & substituter,
 void LocalStore::querySubstitutablePathInfos(const PathSet & paths,
     SubstitutablePathInfos & infos)
 {
-    PathSet todo = paths;
-    foreach (Paths::iterator, i, settings.substituters) {
-        if (todo.empty()) break;
-        querySubstitutablePathInfos(*i, todo, infos);
+    if (!paths.empty()) {
+	PathSet todo = paths;
+	querySubstitutablePathInfos(todo, infos);
     }
 }
 
diff --git a/nix/libstore/local-store.hh b/nix/libstore/local-store.hh
index 4e6b4cfc1d..4113fafcb5 100644
--- a/nix/libstore/local-store.hh
+++ b/nix/libstore/local-store.hh
@@ -40,7 +40,6 @@ struct OptimiseStats
 
 struct RunningSubstituter
 {
-    Path program;
     Pid pid;
     AutoCloseFD to, from, error;
     FdSource fromBuf;
@@ -52,8 +51,8 @@ struct RunningSubstituter
 class LocalStore : public StoreAPI
 {
 private:
-    typedef std::map<Path, RunningSubstituter> RunningSubstituters;
-    RunningSubstituters runningSubstituters;
+    /* The currently running substituter or empty.  */
+    std::unique_ptr<RunningSubstituter> runningSubstituter;
 
     Path linksDir;
 
@@ -93,8 +92,8 @@ public:
 
     PathSet querySubstitutablePaths(const PathSet & paths);
 
-    void querySubstitutablePathInfos(const Path & substituter,
-        PathSet & paths, SubstitutablePathInfos & infos);
+    void querySubstitutablePathInfos(PathSet & paths,
+        SubstitutablePathInfos & infos);
 
     void querySubstitutablePathInfos(const PathSet & paths,
         SubstitutablePathInfos & infos);
@@ -261,8 +260,7 @@ private:
 
     void removeUnusedLinks(const GCState & state);
 
-    void startSubstituter(const Path & substituter,
-        RunningSubstituter & runningSubstituter);
+    void startSubstituter(RunningSubstituter & runningSubstituter);
 
     string getLineFromSubstituter(RunningSubstituter & run);
 
diff --git a/nix/local.mk b/nix/local.mk
index 8e52c77bd9..18e9ba7604 100644
--- a/nix/local.mk
+++ b/nix/local.mk
@@ -154,9 +154,6 @@ noinst_HEADERS =						\
 	         (lambda (in)					\
 	           (write (get-string-all in) out)))))"
 
-nodist_pkglibexec_SCRIPTS =			\
-  %D%/scripts/substitute
-
 # The '.service' files for systemd.
 systemdservicedir = $(libdir)/systemd/system
 nodist_systemdservice_DATA = etc/guix-daemon.service etc/guix-publish.service
diff --git a/nix/nix-daemon/guix-daemon.cc b/nix/nix-daemon/guix-daemon.cc
index 73962af584..6f9c404c8d 100644
--- a/nix/nix-daemon/guix-daemon.cc
+++ b/nix/nix-daemon/guix-daemon.cc
@@ -466,8 +466,7 @@ main (int argc, char *argv[])
     {
       settings.processEnvironment ();
 
-      /* Use our substituter by default.  */
-      settings.substituters.clear ();
+      /* Enable substitutes by default.  */
       settings.set ("build-use-substitutes", "true");
 
       /* Use our substitute server by default.  */
@@ -490,14 +489,6 @@ main (int argc, char *argv[])
       printMsg(lvlDebug,
 	       format ("build log compression: %1%") % settings.logCompression);
 
-      if (settings.useSubstitutes)
-	settings.substituters.push_back (settings.nixLibexecDir
-					 + "/substitute");
-      else
-	/* Clear the substituter list to make sure nothing ever gets
-	   substituted, regardless of the client's settings.  */
-	settings.substituters.clear ();
-
       if (geteuid () == 0 && settings.buildUsersGroup.empty ())
 	fprintf (stderr, _("warning: daemon is running as root, so \
 using `--build-users-group' is highly recommended\n"));
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index f29bcd2eab..ffac6cde34 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -596,8 +596,12 @@ static void performOp(bool trusted, unsigned int clientVersion,
         if (GET_PROTOCOL_MINOR(clientVersion) >= 6
             && GET_PROTOCOL_MINOR(clientVersion) < 0x61)
             settings.set("build-cores", std::to_string(readInt(from)));
-        if (GET_PROTOCOL_MINOR(clientVersion) >= 10)
-            settings.set("build-use-substitutes", readInt(from) ? "true" : "false");
+        if (GET_PROTOCOL_MINOR(clientVersion) >= 10) {
+	    if (settings.useSubstitutes)
+		settings.set("build-use-substitutes", readInt(from) ? "true" : "false");
+	    else
+		readInt(from);			// substitutes remain disabled
+	}
         if (GET_PROTOCOL_MINOR(clientVersion) >= 12) {
             unsigned int n = readInt(from);
             for (unsigned int i = 0; i < n; i++) {
diff --git a/nix/scripts/substitute.in b/nix/scripts/substitute.in
deleted file mode 100644
index 5a2eeb7259..0000000000
--- a/nix/scripts/substitute.in
+++ /dev/null
@@ -1,11 +0,0 @@
-#!@SHELL@
-# A shorthand for "guix substitute", for use by the daemon.
-
-if test "x$GUIX_UNINSTALLED" = "x"
-then
-    prefix="@prefix@"
-    exec_prefix="@exec_prefix@"
-    exec "@bindir@/guix" substitute "$@"
-else
-    exec guix substitute "$@"
-fi
-- 
2.23.0