* Make nix-env --dry-run print the paths to be substituted correctly

  again.  (After the previous substituter mechanism refactoring I
  didn't update the code that obtains the references of substitutable
  paths.)  This required some refactoring: the substituter programs
  are now kept running and receive/respond to info requests via
  stdin/stdout.

10 files changed, 211 insertions, 180 deletions

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 1d624723f9..22f1c64ae3 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -184,11 +184,6 @@ private:
     /* Goals waiting for a build slot. */
     WeakGoals wantingToBuild;
 
-    /* Goals waiting for info from substituters (using --query-info),
-       and the info they're (collectively) waiting for. */
-    WeakGoals waitingForInfo;
-    map<Path, PathSet> requestedInfo;
-
     /* Child processes currently running. */
     Children children;
 
@@ -243,11 +238,6 @@ public:
        call is made to childTerminate(..., true).  */
     void waitForChildTermination(GoalPtr goal);
 
-    /* Put `goal' to sleep until the top-level loop has run `sub' to
-       get info about `storePath' (with --query-info).  We combine
-       substituter invocations to reduce overhead. */
-    void waitForInfo(GoalPtr goal, Path sub, Path storePath);
-
     /* Wait for any goal to finish.  Pretty indiscriminate way to
        wait for some resource that some other goal is holding. */
     void waitForAnyGoal(GoalPtr goal);
@@ -257,12 +247,6 @@ public:
 
     /* Wait for input to become available. */
     void waitForInput();
-
-private:
-
-    /* Process the pending paths in requestedInfo and wake up the
-       goals in waitingForInfo. */
-    void getInfo();
     
 };
 
@@ -2042,12 +2026,12 @@ void DerivationGoal::initChild()
     }
 
     /* Close all other file descriptors. */
-    int maxFD = 0;
-    maxFD = sysconf(_SC_OPEN_MAX);
-    for (int fd = 0; fd < maxFD; ++fd)
-        if (fd != STDIN_FILENO && fd != STDOUT_FILENO && fd != STDERR_FILENO
-            && (!inHook || (fd != 3 && fd != 4)))
-            close(fd); /* ignore result */
+    set<int> exceptions;
+    if (inHook) {
+        exceptions.insert(3);
+        exceptions.insert(4);
+    }
+    closeMostFDs(exceptions);
 }
 
 
@@ -2118,10 +2102,8 @@ private:
     /* The current substituter. */
     Path sub;
 
-    /* Path info returned by the substituter's --query-info operation. */
-    bool infoOkay;
-    PathSet references;
-    Path deriver;
+    /* Path info returned by the substituter's query info operation. */
+    SubstitutablePathInfo info;
 
     /* Pipe for the substitute's standard output/error. */
     Pipe logPipe;
@@ -2205,48 +2187,17 @@ void SubstitutionGoal::init()
         return;
     }
 
-    subs = substituters;
-
-    tryNext();
-}
-
-
-void SubstitutionGoal::tryNext()
-{
-    trace("trying next substituter");
-
-    if (subs.size() == 0) {
-        /* None left.  Terminate this goal and let someone else deal
-           with it. */
+    if (!worker.store.querySubstitutablePathInfo(storePath, info)) {
         printMsg(lvlError,
-            format("path `%1%' is required, but there is no substituter that can build it")
+            format("path `%1%' is required, but there is no substituter that knows anything about it")
             % storePath);
         amDone(ecFailed);
         return;
     }
 
-    sub = subs.front();
-    subs.pop_front();
-
-    infoOkay = false;
-    state = &SubstitutionGoal::gotInfo;
-    worker.waitForInfo(shared_from_this(), sub, storePath);
-}
-
-
-void SubstitutionGoal::gotInfo()
-{
-    trace("got info");
-
-    if (!infoOkay) {
-        tryNext();
-        return;
-    }
-    
     /* To maintain the closure invariant, we first have to realise the
        paths referenced by this one. */
-    for (PathSet::iterator i = references.begin();
-         i != references.end(); ++i)
+    foreach (PathSet::iterator, i, info.references)
         if (*i != storePath) /* ignore self-references */
             addWaitee(worker.makeSubstitutionGoal(*i));
 
@@ -2268,11 +2219,33 @@ void SubstitutionGoal::referencesValid()
         return;
     }
 
-    for (PathSet::iterator i = references.begin();
-         i != references.end(); ++i)
+    foreach (PathSet::iterator, i, info.references)
         if (*i != storePath) /* ignore self-references */
             assert(worker.store.isValidPath(*i));
 
+    subs = substituters;
+
+    tryNext();
+}
+
+
+void SubstitutionGoal::tryNext()
+{
+    trace("trying next substituter");
+
+    if (subs.size() == 0) {
+        /* None left.  Terminate this goal and let someone else deal
+           with it. */
+        printMsg(lvlError,
+            format("path `%1%' is required, but there is no substituter that can build it")
+            % storePath);
+        amDone(ecFailed);
+        return;
+    }
+
+    sub = subs.front();
+    subs.pop_front();
+
     state = &SubstitutionGoal::tryToRun;
     worker.waitForBuildSlot(shared_from_this());
 }
@@ -2413,7 +2386,7 @@ void SubstitutionGoal::finished()
     Hash contentHash = hashPath(htSHA256, storePath);
 
     worker.store.registerValidPath(storePath, contentHash,
-        references, deriver);
+        info.references, info.deriver);
 
     outputLock->setDeletion(true);
     
@@ -2608,14 +2581,6 @@ void Worker::waitForChildTermination(GoalPtr goal)
 }
 
 
-void Worker::waitForInfo(GoalPtr goal, Path sub, Path storePath)
-{
-    debug("wait for info");
-    requestedInfo[sub].insert(storePath);
-    waitingForInfo.insert(goal);
-}
-
-
 void Worker::waitForAnyGoal(GoalPtr goal)
 {
     debug("wait for any goal");
@@ -2623,68 +2588,6 @@ void Worker::waitForAnyGoal(GoalPtr goal)
 }
 
 
-void Worker::getInfo()
-{
-    for (map<Path, PathSet>::iterator i = requestedInfo.begin();
-         i != requestedInfo.end(); ++i)
-    {
-        Path sub = i->first;
-        PathSet paths = i->second;
-
-        while (!paths.empty()) {
-
-            /* Run the substituter for at most 100 paths at a time to
-               prevent command line overflows. */
-            PathSet paths2;
-            while (!paths.empty() && paths2.size() < 100) {
-                paths2.insert(*paths.begin());
-                paths.erase(paths.begin());
-            }
-
-            /* Ask the substituter for the references and deriver of
-               the paths. */
-            debug(format("running `%1%' to get info about `%2%'") % sub % showPaths(paths2));
-            Strings args;
-            args.push_back("--query-info");
-            args.insert(args.end(), paths2.begin(), paths2.end());
-            string res = runProgram(sub, false, args);
-            std::istringstream str(res);
-
-            while (true) {
-                ValidPathInfo info = decodeValidPathInfo(str);
-                if (info.path == "") break;
-
-                /* !!! inefficient */
-                for (WeakGoals::iterator k = waitingForInfo.begin();
-                     k != waitingForInfo.end(); ++k)
-                {
-                    GoalPtr goal = k->lock();
-                    if (goal) {
-                        SubstitutionGoal * goal2 = dynamic_cast<SubstitutionGoal *>(goal.get());
-                        if (goal2->storePath == info.path) {
-                            goal2->references = info.references;
-                            goal2->deriver = info.deriver;
-                            goal2->infoOkay = true;
-                            wakeUp(goal);
-                        }
-                    }
-                }
-            }
-        }
-    }
-
-    for (WeakGoals::iterator k = waitingForInfo.begin();
-         k != waitingForInfo.end(); ++k)
-    {
-        GoalPtr goal = k->lock();
-        if (goal) wakeUp(goal);
-    }
-    
-    requestedInfo.clear();
-    waitingForInfo.clear(); // !!! have we done them all?
-}
-
-
 void Worker::run(const Goals & _topGoals)
 {
     for (Goals::iterator i = _topGoals.begin();
@@ -2711,8 +2614,6 @@ void Worker::run(const Goals & _topGoals)
 
         if (topGoals.empty()) break;
 
-        getInfo();
-
         /* Wait for input. */
         if (!children.empty())
             waitForInput();
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index d886ba5586..6926c49370 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -83,6 +83,13 @@ LocalStore::~LocalStore()
 {
     try {
         flushDelayedUpdates();
+
+        foreach (RunningSubstituters::iterator, i, runningSubstituters) {
+            i->second.toBuf.reset();
+            i->second.to.reset();
+            i->second.pid.wait(true);
+        }
+                
     } catch (...) {
         ignoreException();
     }
@@ -367,8 +374,6 @@ ValidPathInfo LocalStore::queryPathInfo(const Path & path)
     std::map<Path, ValidPathInfo>::iterator lookup = pathInfoCache.find(path);
     if (lookup != pathInfoCache.end()) return lookup->second;
     
-    //printMsg(lvlError, "queryPathInfo: " + path);
-    
     /* Read the info file. */
     Path infoFile = infoFileFor(path);
     if (!pathExists(infoFile))
@@ -467,33 +472,105 @@ Path LocalStore::queryDeriver(const Path & path)
 }
 
 
-PathSet LocalStore::querySubstitutablePaths()
+void LocalStore::startSubstituter(const Path & substituter, RunningSubstituter & run)
 {
-    if (!substitutablePathsLoaded) {
-        for (Paths::iterator i = substituters.begin(); i != substituters.end(); ++i) {
-            debug(format("running `%1%' to find out substitutable paths") % *i);
-            Strings args;
-            args.push_back("--query-paths");
-            Strings ss = tokenizeString(runProgram(*i, false, args), "\n");
-            for (Strings::iterator j = ss.begin(); j != ss.end(); ++j) {
-                if (!isStorePath(*j))
-                    throw Error(format("`%1%' returned a bad substitutable path `%2%'")
-                        % *i % *j);
-                substitutablePaths.insert(*j);
-            }
+    if (run.pid != -1) return;
+    
+    debug(format("starting substituter program `%1%'") % substituter);
+
+    Pipe toPipe, fromPipe;
+            
+    toPipe.create();
+    fromPipe.create();
+
+    run.pid = fork();
+            
+    switch (run.pid) {
+
+    case -1:
+        throw SysError("unable to fork");
+
+    case 0: /* child */
+        try {
+            fromPipe.readSide.close();
+            toPipe.writeSide.close();
+            if (dup2(toPipe.readSide, STDIN_FILENO) == -1)
+                throw SysError("dupping stdin");
+            if (dup2(fromPipe.writeSide, STDOUT_FILENO) == -1)
+                throw SysError("dupping stdout");
+            closeMostFDs(set<int>());
+            execl(substituter.c_str(), substituter.c_str(), "--query", NULL);
+            throw SysError(format("executing `%1%'") % substituter);
+        } catch (std::exception & e) {
+            std::cerr << "error: " << e.what() << std::endl;
         }
-        substitutablePathsLoaded = true;
+        quickExit(1);
     }
 
-    return substitutablePaths;
+    /* Parent. */
+    
+    toPipe.readSide.close();
+    fromPipe.writeSide.close();
+
+    run.toBuf = boost::shared_ptr<stdio_filebuf>(new stdio_filebuf(toPipe.writeSide.borrow(), std::ios_base::out));
+    run.to = boost::shared_ptr<std::ostream>(new std::ostream(&*run.toBuf));
+
+    run.fromBuf = boost::shared_ptr<stdio_filebuf>(new stdio_filebuf(fromPipe.readSide.borrow(), std::ios_base::in));
+    run.from = boost::shared_ptr<std::istream>(new std::istream(&*run.fromBuf));
 }
 
 
 bool LocalStore::hasSubstitutes(const Path & path)
 {
-    if (!substitutablePathsLoaded)
-        querySubstitutablePaths(); 
-    return substitutablePaths.find(path) != substitutablePaths.end();
+    foreach (Paths::iterator, i, substituters) {
+        RunningSubstituter & run(runningSubstituters[*i]);
+        startSubstituter(*i, run);
+
+        *run.to << "have\n" << path << "\n" << std::flush;
+
+        string s;
+
+        int res;
+        getline(*run.from, s);
+        if (!string2Int(s, res)) abort();
+
+        if (res) return true;
+    }
+
+    return false;
+}
+
+
+bool LocalStore::querySubstitutablePathInfo(const Path & path,
+    SubstitutablePathInfo & info)
+{
+    foreach (Paths::iterator, i, substituters) {
+        RunningSubstituter & run(runningSubstituters[*i]);
+        startSubstituter(*i, run);
+
+        *run.to << "info\n" << path << "\n" << std::flush;
+
+        string s;
+
+        int res;
+        getline(*run.from, s);
+        if (!string2Int(s, res)) abort();
+
+        if (res) {
+            getline(*run.from, info.deriver);
+            int nrRefs;
+            getline(*run.from, s);
+            if (!string2Int(s, nrRefs)) abort();
+            while (nrRefs--) {
+                Path p; getline(*run.from, p);
+                info.references.insert(p);
+            }
+            info.downloadSize = 0;
+            return true;
+        }
+    }
+    
+    return false;
 }
 
 
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 1957b9f579..f096bdd855 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -3,6 +3,8 @@
 
 #include <string>
 
+#include <ext/stdio_filebuf.h>
+
 #include "store-api.hh"
 #include "util.hh"
 
@@ -34,11 +36,26 @@ struct OptimiseStats
 };
 
 
+typedef __gnu_cxx::stdio_filebuf<char> stdio_filebuf;
+
+
+struct RunningSubstituter
+{
+    Pid pid;
+    boost::shared_ptr<stdio_filebuf> toBuf, fromBuf;
+    boost::shared_ptr<std::ostream> to;
+    boost::shared_ptr<std::istream> from;
+};
+
+
 class LocalStore : public StoreAPI
 {
 private:
     bool substitutablePathsLoaded;
     PathSet substitutablePaths;
+
+    typedef std::map<Path, RunningSubstituter> RunningSubstituters;
+    RunningSubstituters runningSubstituters;
     
 public:
 
@@ -65,6 +82,9 @@ public:
     PathSet querySubstitutablePaths();
     
     bool hasSubstitutes(const Path & path);
+
+    bool querySubstitutablePathInfo(const Path & path,
+        SubstitutablePathInfo & info);
     
     Path addToStore(const Path & srcPath, bool fixed = false,
         bool recursive = false, string hashAlgo = "",
@@ -147,6 +167,9 @@ private:
     void tryToDelete(const GCOptions & options, GCResults & results,
         const PathSet & livePaths, const PathSet & tempRootsClosed, PathSet & done, 
         const Path & path);
+
+    void startSubstituter(const Path & substituter,
+        RunningSubstituter & runningSubstituter);
 };
 
 
diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc
index 4b192ec9a0..acf9346d4b 100644
--- a/src/libstore/misc.cc
+++ b/src/libstore/misc.cc
@@ -1,5 +1,6 @@
 #include "misc.hh"
 #include "store-api.hh"
+#include "local-store.hh"
 
 #include <aterm2.h>
 
@@ -79,10 +80,13 @@ void queryMissing(const PathSet & targets,
 
         else {
             if (store->isValidPath(p)) continue;
-            if (store->hasSubstitutes(p))
+            SubstitutablePathInfo info;
+            if (dynamic_cast<LocalStore *>(store.get())->querySubstitutablePathInfo(p, info)) {
                 willSubstitute.insert(p);
-            // XXX call the substituters
-            // store->queryReferences(p, todo);
+                todo.insert(info.references.begin(), info.references.end());
+            }
+            /* Not substitutable and not buildable; should we flag
+               this? */
         }
     }
 }
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 274196a2b6..b7e60043d6 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -213,6 +213,13 @@ bool RemoteStore::hasSubstitutes(const Path & path)
 }
 
 
+bool RemoteStore::querySubstitutablePathInfo(const Path & path,
+    SubstitutablePathInfo & info)
+{
+    throw Error("not implemented");
+}
+
+
 Hash RemoteStore::queryPathHash(const Path & path)
 {
     writeInt(wopQueryPathHash, to);
@@ -256,12 +263,6 @@ Path RemoteStore::queryDeriver(const Path & path)
 }
 
 
-PathSet RemoteStore::querySubstitutablePaths()
-{
-    throw Error("not implemented");
-}
-
-
 Path RemoteStore::addToStore(const Path & _srcPath, bool fixed,
     bool recursive, string hashAlgo, PathFilter & filter)
 {
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index c40feeaa47..969c8f9b62 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -37,10 +37,11 @@ public:
 
     Path queryDeriver(const Path & path);
     
-    PathSet querySubstitutablePaths();
-    
     bool hasSubstitutes(const Path & path);
     
+    bool querySubstitutablePathInfo(const Path & path,
+        SubstitutablePathInfo & info);
+    
     Path addToStore(const Path & srcPath, bool fixed = false,
         bool recursive = false, string hashAlgo = "",
         PathFilter & filter = defaultPathFilter);
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index 1fb7326335..9b578eac7d 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -17,13 +17,6 @@ GCOptions::GCOptions()
 }
 
 
-bool StoreAPI::hasSubstitutes(const Path & path)
-{
-    PathSet paths = querySubstitutablePaths();
-    return paths.find(path) != paths.end();
-}
-
-
 bool isInStore(const Path & path)
 {
     return path[0] == '/'
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index b2a2dc7a53..9645237e04 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -88,6 +88,14 @@ struct GCResults
 };
 
 
+struct SubstitutablePathInfo
+{
+    Path deriver;
+    PathSet references;
+    unsigned long long downloadSize; /* 0 = unknown or inapplicable */
+};
+
+
 class StoreAPI 
 {
 public:
@@ -117,12 +125,13 @@ public:
        no deriver has been set. */
     virtual Path queryDeriver(const Path & path) = 0;
 
-    /* Query the set of substitutable paths. */
-    virtual PathSet querySubstitutablePaths() = 0;
+    /* Query whether a path has substitutes. */
+    virtual bool hasSubstitutes(const Path & path) = 0;
 
-    /* More efficient variant if we just want to know if a path has
-       substitutes. */
-    virtual bool hasSubstitutes(const Path & path);
+    /* Query the references, deriver and download size of a
+       substitutable path. */
+    virtual bool querySubstitutablePathInfo(const Path & path,
+        SubstitutablePathInfo & info) = 0;
     
     /* Copy the contents of a path to the store and register the
        validity the resulting path.  The resulting path is returned.
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 4f6c367da8..c28f4e1bb7 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -579,7 +579,14 @@ AutoCloseFD::AutoCloseFD(int fd)
 
 AutoCloseFD::AutoCloseFD(const AutoCloseFD & fd)
 {
-    abort();
+    /* Copying a AutoCloseFD isn't allowed (who should get to close
+       it?).  But as a edge case, allow copying of closed
+       AutoCloseFDs.  This is necessary due to tiresome reasons
+       involving copy constructor use on default object values in STL
+       containers (like when you do `map[value]' where value isn't in
+       the map yet). */
+    this->fd = fd.fd;
+    if (this->fd != -1) abort();
 }
 
 
@@ -832,7 +839,7 @@ string runProgram(Path program, bool searchPath, const Strings & args)
             pipe.readSide.close();
 
             if (dup2(pipe.writeSide, STDOUT_FILENO) == -1)
-                throw SysError("dupping from-hook write side");
+                throw SysError("dupping stdout");
 
             std::vector<const char *> cargs; /* careful with c_str()! */
             cargs.push_back(program.c_str());
@@ -868,6 +875,17 @@ string runProgram(Path program, bool searchPath, const Strings & args)
 }
 
 
+void closeMostFDs(const set<int> & exceptions)
+{
+    int maxFD = 0;
+    maxFD = sysconf(_SC_OPEN_MAX);
+    for (int fd = 0; fd < maxFD; ++fd)
+        if (fd != STDIN_FILENO && fd != STDOUT_FILENO && fd != STDERR_FILENO
+            && exceptions.find(fd) == exceptions.end())
+            close(fd); /* ignore result */
+}
+
+
 void quickExit(int status)
 {
 #ifdef __CYGWIN__
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 5d28a8308c..6c67e450b5 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -245,6 +245,10 @@ void killUser(uid_t uid);
 string runProgram(Path program, bool searchPath = false,
     const Strings & args = Strings());
 
+/* Close all file descriptors except stdin, stdout, stderr, and those
+   listed in the given set.  Good practice in child processes. */
+void closeMostFDs(const set<int> & exceptions);
+
 /* Wrapper around _exit() on Unix and ExitProcess() on Windows.  (On
    Cygwin, _exit() doesn't seem to do the right thing.) */
 void quickExit(int status);