From patchwork Sun Dec 6 22:04:11 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Ludovic_Court=C3=A8s?= X-Patchwork-Id: 25615 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 89BE027BC00; Sun, 6 Dec 2020 22:05:28 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 468CF27BC01 for ; Sun, 6 Dec 2020 22:05:26 +0000 (GMT) Received: from localhost ([::1]:33720 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1km29l-0001Kj-Eh for patchwork@mira.cbaines.net; Sun, 06 Dec 2020 17:05:25 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:46452) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1km29P-0001K6-9c for guix-patches@gnu.org; Sun, 06 Dec 2020 17:05:03 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:40040) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1km29P-0003Jn-1l for guix-patches@gnu.org; Sun, 06 Dec 2020 17:05:03 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1km29O-0006Rp-To for guix-patches@gnu.org; Sun, 06 Dec 2020 17:05:02 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#45018] [PATCH v2 2/6] daemon: Use 'Agent' to spawn 'guix substitute --query'. Resent-From: Ludovic =?utf-8?q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Sun, 06 Dec 2020 22:05:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 45018 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 45018@debbugs.gnu.org Cc: Ludovic =?utf-8?q?Court=C3=A8s?= Received: via spool by 45018-submit@debbugs.gnu.org id=B45018.160729229224720 (code B ref 45018); Sun, 06 Dec 2020 22:05:02 +0000 Received: (at 45018) by debbugs.gnu.org; 6 Dec 2020 22:04:52 +0000 Received: from localhost ([127.0.0.1]:51575 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1km29D-0006QZ-TN for submit@debbugs.gnu.org; Sun, 06 Dec 2020 17:04:52 -0500 Received: from eggs.gnu.org ([209.51.188.92]:56374) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1km29B-0006Px-Q9 for 45018@debbugs.gnu.org; Sun, 06 Dec 2020 17:04:50 -0500 Received: from fencepost.gnu.org ([2001:470:142:3::e]:44343) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1km293-0003Dq-DA; Sun, 06 Dec 2020 17:04:44 -0500 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=45440 helo=gnu.org) by fencepost.gnu.org with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1km292-00020D-75; Sun, 06 Dec 2020 17:04:40 -0500 From: Ludovic =?utf-8?q?Court=C3=A8s?= Date: Sun, 6 Dec 2020 23:04:11 +0100 Message-Id: <20201206220415.23279-3-ludo@gnu.org> X-Mailer: git-send-email 2.29.2 In-Reply-To: <20201206220415.23279-1-ludo@gnu.org> References: <87o8janf50.fsf@gnu.org> <20201206220415.23279-1-ludo@gnu.org> MIME-Version: 1.0 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: "Guix-patches" X-getmail-retrieved-from-mailbox: Patches * nix/libstore/local-store.hh (RunningSubstituter): Remove. (LocalStore)[runningSubstituter]: Change to unique_ptr. [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 T LocalStore::getIntLineFromSubstituter(RunningSubstituter & run) +template 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_ptrfresh(new RunningSubstituter); + const Strings args = { "substitute", "--query" }; + const std::map env = { { "_NIX_OPTIONS", settings.pack() } }; + std::unique_ptr 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_ptrfresh(new RunningSubstituter); + const Strings args = { "substitute", "--query" }; + const std::map env = { { "_NIX_OPTIONS", settings.pack() } }; + std::unique_ptr 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; + std::unique_ptr 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 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 T getIntLineFromSubstituter(RunningSubstituter & run); + template 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