From bc69ea2d605810cc32e13ed03d5848b8dc358b61 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Tue, 3 Sep 2019 22:11:47 +0200 Subject: daemon: Run 'guix offload' directly. * nix/scripts/offload.in: Remove. * nix/local.mk (nodist_pkglibexec_SCRIPTS) [BUILD_DAEMON_OFFLOAD]: Remove 'scripts/offload'. * config-daemon.ac: Don't output 'nix/scripts/offload'. * build-aux/pre-inst-env.in: Don't set 'NIX_BUILD_HOOK'. * nix/libstore/build.cc (HookInstance::HookInstance): Run 'guix offload'. (DerivationGoal::tryBuildHook): Remove reference to 'NIX_BUILD_HOOK'. * nix/nix-daemon/guix-daemon.cc (main) [HAVE_DAEMON_OFFLOAD_HOOK]: Don't set 'NIX_BUILD_HOOK'. * nix/nix-daemon/nix-daemon.cc (performOp) [!HAVE_DAEMON_OFFLOAD_HOOK]: Leave 'settings.useBuildHook' unchanged. --- nix/libstore/build.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'nix/libstore/build.cc') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index fe7bf79069..9f1f88933a 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -614,9 +614,7 @@ HookInstance::HookInstance() { debug("starting build hook"); - Path buildHook = getEnv("NIX_BUILD_HOOK"); - if (string(buildHook, 0, 1) != "/") buildHook = settings.nixLibexecDir + "/nix/" + buildHook; - buildHook = canonPath(buildHook); + const Path &buildHook = settings.guixProgram; /* Create a pipe to get the output of the child. */ fromHook.create(); @@ -642,13 +640,14 @@ HookInstance::HookInstance() if (dup2(builderOut.writeSide, 4) == -1) throw SysError("dupping builder's stdout/stderr"); - execl(buildHook.c_str(), buildHook.c_str(), settings.thisSystem.c_str(), + execl(buildHook.c_str(), buildHook.c_str(), "offload", + settings.thisSystem.c_str(), (format("%1%") % settings.maxSilentTime).str().c_str(), (format("%1%") % settings.printBuildTrace).str().c_str(), (format("%1%") % settings.buildTimeout).str().c_str(), NULL); - throw SysError(format("executing `%1%'") % buildHook); + throw SysError(format("executing `%1% offload'") % buildHook); }); pid.setSeparatePG(true); @@ -1581,7 +1580,7 @@ void DerivationGoal::buildDone() HookReply DerivationGoal::tryBuildHook() { - if (!settings.useBuildHook || getEnv("NIX_BUILD_HOOK") == "") return rpDecline; + if (!settings.useBuildHook) return rpDecline; if (!worker.hook) worker.hook = std::shared_ptr(new HookInstance); -- cgit v1.2.3 From f6919ebdc6b0ce0286814cc6ab0564b1a4c67f5f Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Wed, 4 Sep 2019 11:04:44 +0200 Subject: 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 | 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 (limited to 'nix/libstore/build.cc') 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; - - 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_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 -- cgit v1.2.3 From ada9a19a2dca74feafcf24df1152abd685d4142f Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Sat, 28 Sep 2019 18:06:15 +0200 Subject: daemon: Strictly respect timeouts for 'guix offload'. Until now it was up to 'guix offload' to honor timeouts. Unfortunately it would sometimes fail to do that, for example due to the libssh bug at . With this change, 'guix offload' is automatically killed by the daemon when one of the timeouts expires. Thus, data transfers performed by 'guix offload' now count as part of the timeouts, rather than just actual build time. * nix/libstore/build.cc (DerivationGoal::tryBuildHook): Pass true as the 'respectTimeouts' argument to 'childStarted'. --- nix/libstore/build.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'nix/libstore/build.cc') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index ad53b81413..74cd05417f 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -1648,7 +1648,7 @@ HookReply DerivationGoal::tryBuildHook() set fds; fds.insert(hook->fromHook.readSide); fds.insert(hook->builderOut.readSide); - worker.childStarted(shared_from_this(), hook->pid, fds, false, false); + worker.childStarted(shared_from_this(), hook->pid, fds, false, true); if (settings.printBuildTrace) printMsg(lvlError, format("@ build-started %1% - %2% %3% %4%") -- cgit v1.2.3