From f6919ebdc6b0ce0286814cc6ab0564b1a4c67f5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= Date: Wed, 4 Sep 2019 11:04:44 +0200 Subject: [PATCH] daemon: Run 'guix substitute' directly and assume a single substituter. 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 | 95 +++++++++++++++++++---------------- 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, 84 insertions(+), 115 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(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(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; + if (!settings.useSubstitutes || paths.empty()) 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 (!runningSubstituter) { + std::unique_ptrfresh(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_ptrfresh(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 RunningSubstituters; - RunningSubstituters runningSubstituters; + /* The currently running substituter or empty. */ + std::unique_ptr 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