From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id ENOVE/e7yF/xXAAA0tVLHw (envelope-from ) for ; Thu, 03 Dec 2020 10:20:39 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id uK9bD/e7yF89aQAA1q6Kng (envelope-from ) for ; Thu, 03 Dec 2020 10:20:39 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id ECC219403EB for ; Thu, 3 Dec 2020 10:20:38 +0000 (UTC) Received: from localhost ([::1]:52946 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kklj3-0004fc-Un for larch@yhetil.org; Thu, 03 Dec 2020 05:20:37 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:37840) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kkliV-0004Pu-OM for guix-patches@gnu.org; Thu, 03 Dec 2020 05:20:03 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:55172) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kkliV-0006RC-G7 for guix-patches@gnu.org; Thu, 03 Dec 2020 05:20:03 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kkliV-0000Fr-CS for guix-patches@gnu.org; Thu, 03 Dec 2020 05:20:03 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#45018] [PATCH 4/6] daemon: Run 'guix substitute --substitute' as an agent. Resent-From: Ludovic =?UTF-8?Q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Thu, 03 Dec 2020 10:20:03 +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.1606990797912 (code B ref 45018); Thu, 03 Dec 2020 10:20:03 +0000 Received: (at 45018) by debbugs.gnu.org; 3 Dec 2020 10:19:57 +0000 Received: from localhost ([127.0.0.1]:38475 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kkliP-0000Ed-1n for submit@debbugs.gnu.org; Thu, 03 Dec 2020 05:19:57 -0500 Received: from eggs.gnu.org ([209.51.188.92]:47652) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kkliN-0000Dp-IA for 45018@debbugs.gnu.org; Thu, 03 Dec 2020 05:19:56 -0500 Received: from fencepost.gnu.org ([2001:470:142:3::e]:40707) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kkliI-0006Jz-DM; Thu, 03 Dec 2020 05:19:50 -0500 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=49238 helo=gnu.org) by fencepost.gnu.org with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1kkliG-0007oh-Lr; Thu, 03 Dec 2020 05:19:49 -0500 From: Ludovic =?UTF-8?Q?Court=C3=A8s?= Date: Thu, 3 Dec 2020 11:19:28 +0100 Message-Id: <20201203101930.11210-4-ludo@gnu.org> X-Mailer: git-send-email 2.29.2 In-Reply-To: <20201203101930.11210-1-ludo@gnu.org> References: <20201203101930.11210-1-ludo@gnu.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN X-Migadu-Spam-Score: 2.22 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=pass (policy=none) header.from=gnu.org; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Queue-Id: ECC219403EB X-Spam-Score: 2.22 X-Migadu-Scanner: ns3122888.ip-94-23-21.eu X-TUID: L7m35xl+IlGf This avoids spawning one substitute process per substitution. * nix/libstore/build.cc (class Worker)[substituter]: New field. [outPipe, logPipe, pid]: Remove. (class SubstitutionGoal)[expectedHashStr, status, substituter]: New fields. (SubstitutionGoal::timedOut): Adjust to check 'substituter'. (SubstitutionGoal::tryToRun): Remove references to 'outPipe' and 'logPipe'. Run "guix substitute --substitute" as an 'Agent'. Send the request with 'writeLine'. (SubstitutionGoal::finished): Likewise. (SubstitutionGoal::handleChildOutput): Change to fill in 'expectedHashStr' and 'status'. (SubstitutionGoal::handleEOF): Call 'wakeUp' unconditionally. (SubstitutionGoal::~SubstitutionGoal): Adjust to check 'substituter'. * guix/scripts/substitute.scm (process-substitution): Write "success\n" to stdout upon success. (%error-to-file-descriptor-4?): New variable. (guix-substitute): Set 'current-error-port' to file descriptor 4 unless (%error-to-file-descriptor-4?) is false. Remove "--substitute" arguments. Loop reading line from stdin. * tests/substitute.scm : Call '%error-to-file-descriptor-4?'. (request-substitution): New procedure. ("substitute, no signature") ("substitute, invalid hash") ("substitute, unauthorized key") ("substitute, authorized key") ("substitute, unauthorized narinfo comes first") ("substitute, unsigned narinfo comes first") ("substitute, first narinfo is unsigned and has wrong hash") ("substitute, first narinfo is unsigned and has wrong refs") ("substitute, two invalid narinfos") ("substitute, narinfo with several URLs"): Adjust to new "guix substitute --substitute" calling convention. --- guix/scripts/substitute.scm | 34 +++++++--- nix/libstore/build.cc | 129 ++++++++++++++++++------------------ tests/substitute.scm | 95 +++++++++++++++----------- 3 files changed, 145 insertions(+), 113 deletions(-) diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index b2ae7bac40..334d3c97f8 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -88,6 +88,7 @@ write-narinfo %allow-unauthenticated-substitutes? + %error-to-file-descriptor-4? substitute-urls guix-substitute)) @@ -1016,7 +1017,10 @@ DESTINATION as a nar file. Verify the substitute against ACL." ;; Skip a line after what 'progress-reporter/file' printed, and another ;; one to visually separate substitutions. - (display "\n\n" (current-error-port))))) + (display "\n\n" (current-error-port)) + + ;; Tell the daemon that we're done. + (display "success\n" (current-output-port))))) ;;; @@ -1125,6 +1129,11 @@ default value." (unless (string->uri uri) (leave (G_ "~a: invalid URI~%") uri))) +(define %error-to-file-descriptor-4? + ;; Whether to direct 'current-error-port' to file descriptor 4 like + ;; 'guix-daemon' expects. + (make-parameter #t)) + (define-command (guix-substitute . args) (category internal) (synopsis "implement the build daemon's substituter protocol") @@ -1138,9 +1147,9 @@ default value." ;; 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))))) + (parameterize ((current-error-port (if (%error-to-file-descriptor-4?) + (fdopen 4 "wl") + (current-error-port)))) ;; Redirect diagnostics to file descriptor 4 as well. (guix-warning-port (current-error-port)) @@ -1182,15 +1191,22 @@ default value." #:cache-urls (substitute-urls) #:acl acl) (loop (read-line))))))) - (("--substitute" store-path destination) + (("--substitute") ;; 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?))) + (let loop () + (match (read-line) + ((? eof-object?) + #t) + ((= string-tokenize ("substitute" store-path destination)) + (process-substitution store-path destination + #:cache-urls (substitute-urls) + #:acl (current-acl) + #:print-build-trace? + print-build-trace?) + (loop)))))) ((or ("-V") ("--version")) (show-version-and-exit "guix substitute")) (("--help") diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index 7e9ab3f39c..50d300253d 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -262,6 +262,7 @@ public: LocalStore & store; std::shared_ptr hook; + std::shared_ptr substituter; Worker(LocalStore & store); ~Worker(); @@ -2773,15 +2774,6 @@ private: /* Path info returned by the substituter's query info operation. */ SubstitutablePathInfo info; - /* Pipe for the substituter's standard output. */ - Pipe outPipe; - - /* Pipe for the substituter's standard error. */ - Pipe logPipe; - - /* The process ID of the builder. */ - Pid pid; - /* Lock on the store path. */ std::shared_ptr outputLock; @@ -2795,6 +2787,17 @@ private: typedef void (SubstitutionGoal::*GoalState)(); GoalState state; + /* The substituter. */ + std::shared_ptr substituter; + + /* Either the empty string, or the expected hash as returned by the + substituter. */ + string expectedHashStr; + + /* Either the empty string, or the status phrase returned by the + substituter. */ + string status; + void tryNext(); public: @@ -2840,7 +2843,7 @@ SubstitutionGoal::SubstitutionGoal(const Path & storePath, Worker & worker, bool SubstitutionGoal::~SubstitutionGoal() { - if (pid != -1) worker.childTerminated(pid); + if (substituter) worker.childTerminated(substituter->pid); } @@ -2848,9 +2851,9 @@ void SubstitutionGoal::timedOut() { if (settings.printBuildTrace) printMsg(lvlError, format("@ substituter-failed %1% timeout") % storePath); - if (pid != -1) { - pid_t savedPid = pid; - pid.kill(); + if (substituter) { + pid_t savedPid = substituter->pid; + substituter.reset(); worker.childTerminated(savedPid); } amDone(ecFailed); @@ -2977,45 +2980,29 @@ void SubstitutionGoal::tryToRun() printMsg(lvlInfo, format("fetching path `%1%'...") % storePath); - outPipe.create(); - logPipe.create(); - destPath = repair ? storePath + ".tmp" : storePath; /* Remove the (stale) output path if it exists. */ if (pathExists(destPath)) deletePath(destPath); - /* Fill in the arguments. */ - Strings args; - args.push_back("guix"); - args.push_back("substitute"); - args.push_back("--substitute"); - args.push_back(storePath); - args.push_back(destPath); + if (!worker.substituter) { + const Strings args = { "substitute", "--substitute" }; + const std::map env = { { "_NIX_OPTIONS", settings.pack() } }; + worker.substituter = std::make_shared(settings.guixProgram, args, env); + } - /* Fork the substitute program. */ - pid = startProcess([&]() { + /* Borrow the worker's substituter. */ + if (!substituter) substituter.swap(worker.substituter); - /* Communicate substitute-urls & co. to 'guix substitute'. */ - setenv("_NIX_OPTIONS", settings.pack().c_str(), 1); + /* Send the request to the substituter. */ + writeLine(substituter->toAgent.writeSide, + (format("substitute %1% %2%") % storePath % destPath).str()); - commonChildInit(logPipe); - - if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1) - throw SysError("cannot dup output pipe into stdout"); - - execv(settings.guixProgram.c_str(), stringsToCharPtrs(args).data()); - - throw SysError(format("executing `%1% substitute'") % settings.guixProgram); - }); - - pid.setSeparatePG(true); - pid.setKillSignal(SIGTERM); - outPipe.writeSide.close(); - logPipe.writeSide.close(); - worker.childStarted(shared_from_this(), - pid, singleton >(logPipe.readSide), true, true); + set fds; + fds.insert(substituter->fromAgent.readSide); + fds.insert(substituter->builderOut.readSide); + worker.childStarted(shared_from_this(), substituter->pid, fds, true, true); state = &SubstitutionGoal::finished; @@ -3030,28 +3017,25 @@ void SubstitutionGoal::finished() { trace("substitute finished"); - /* Since we got an EOF on the logger pipe, the substitute is - presumed to have terminated. */ - pid_t savedPid = pid; - int status = pid.wait(true); + /* Remove the 'guix substitute' process from the list of children. */ + worker.childTerminated(substituter->pid); - /* So the child is gone now. */ - worker.childTerminated(savedPid); - - /* Close the read side of the logger pipe. */ - logPipe.readSide.close(); - - /* Get the hash info from stdout. */ - string expectedHashStr = statusOk(status) ? readLine(outPipe.readSide) : ""; - outPipe.readSide.close(); + /* If max-jobs > 1, the worker might have created a new 'substitute' + process in the meantime. If that is the case, terminate ours; + otherwise, give it back to the worker. */ + if (worker.substituter) { + substituter.reset (); + } else { + worker.substituter.swap(substituter); + } /* Check the exit status and the build result. */ HashResult hash; try { - if (!statusOk(status)) - throw SubstError(format("fetching path `%1%' %2%") - % storePath % statusToString(status)); + if (status != "success") + throw SubstError(format("fetching path `%1%' (status: '%2%')") + % storePath % status); if (!pathExists(destPath)) throw SubstError(format("substitute did not produce path `%1%'") % destPath); @@ -3122,16 +3106,33 @@ void SubstitutionGoal::finished() void SubstitutionGoal::handleChildOutput(int fd, const string & data) { - assert(fd == logPipe.readSide); - if (verbosity >= settings.buildVerbosity) writeToStderr(data); - /* Don't write substitution output to a log file for now. We - probably should, though. */ + if (verbosity >= settings.buildVerbosity + && fd == substituter->builderOut.readSide) { + writeToStderr(data); + /* Don't write substitution output to a log file for now. We + probably should, though. */ + } + + if (fd == substituter->fromAgent.readSide) { + /* Trim whitespace to the right. */ + size_t end = data.find_last_not_of(" \t\n"); + string trimmed = (end != string::npos) ? data.substr(0, end + 1) : data; + + if (expectedHashStr == "") { + expectedHashStr = trimmed; + } else if (status == "") { + status = trimmed; + worker.wakeUp(shared_from_this()); + } else { + printMsg(lvlError, format("unexpected substituter message '%1%'") % data); + } + } } void SubstitutionGoal::handleEOF(int fd) { - if (fd == logPipe.readSide) worker.wakeUp(shared_from_this()); + worker.wakeUp(shared_from_this()); } diff --git a/tests/substitute.scm b/tests/substitute.scm index bd5b6305b0..b86ce09425 100644 --- a/tests/substitute.scm +++ b/tests/substitute.scm @@ -58,6 +58,14 @@ it writes to GUIX-WARNING-PORT a messages that matches ERROR-RX." (let ((message (get-output-string error-output))) (->bool (string-match error-rx message)))))))))) +(define (request-substitution item destination) + "Run 'guix substitute --substitute' to fetch ITEM to DESTINATION." + (parameterize ((guix-warning-port (current-error-port))) + (with-input-from-string (string-append "substitute " item " " + destination "\n") + (lambda () + (guix-substitute "--substitute"))))) + (define %public-key ;; This key is known to be in the ACL by default. (call-with-input-file (string-append %config-directory "/signing-key.pub") @@ -184,6 +192,11 @@ a file for NARINFO." ;; Transmit these options to 'guix substitute'. (substitute-urls (list (getenv "GUIX_BINARY_SUBSTITUTE_URL"))) +;; Never use file descriptor 4, unlike what happens when invoked by the +;; daemon. +(%error-to-file-descriptor-4? #f) + + (test-equal "query narinfo without signature" "" ; not substitutable @@ -284,10 +297,12 @@ System: mips64el-linux\n") (test-quit "substitute, no signature" "no valid substitute" (with-narinfo %narinfo - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "foo"))) + (with-input-from-string (string-append "substitute " + (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo" + " foo\n") + (lambda () + (guix-substitute "--substitute"))))) (test-quit "substitute, invalid hash" "no valid substitute" @@ -295,10 +310,12 @@ System: mips64el-linux\n") (with-narinfo (string-append %narinfo "Signature: " (signature-field "different body") "\n") - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "foo"))) + (with-input-from-string (string-append "substitute " + (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo" + " foo\n") + (lambda () + (guix-substitute "--substitute"))))) (test-quit "substitute, unauthorized key" "no valid substitute" @@ -307,10 +324,12 @@ System: mips64el-linux\n") %narinfo #:public-key %wrong-public-key) "\n") - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "foo"))) + (with-input-from-string (string-append "substitute " + (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo" + " foo\n") + (lambda () + (guix-substitute "--substitute"))))) (test-equal "substitute, authorized key" "Substitutable data." @@ -319,10 +338,9 @@ System: mips64el-linux\n") (dynamic-wind (const #t) (lambda () - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved") + (request-substitution (string-append (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") + "substitute-retrieved") (call-with-input-file "substitute-retrieved" get-string-all)) (lambda () (false-if-exception (delete-file "substitute-retrieved")))))) @@ -352,10 +370,9 @@ System: mips64el-linux\n") (map (cut string-append "file://" <>) (list %alternate-substitute-directory %main-substitute-directory)))) - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved")) + (request-substitution (string-append (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") + "substitute-retrieved")) (call-with-input-file "substitute-retrieved" get-string-all)) (lambda () (false-if-exception (delete-file "substitute-retrieved"))))))) @@ -381,10 +398,9 @@ System: mips64el-linux\n") (map (cut string-append "file://" <>) (list %alternate-substitute-directory %main-substitute-directory)))) - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved")) + (request-substitution (string-append (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") + "substitute-retrieved")) (call-with-input-file "substitute-retrieved" get-string-all)) (lambda () (false-if-exception (delete-file "substitute-retrieved"))))))) @@ -417,10 +433,9 @@ System: mips64el-linux\n") (map (cut string-append "file://" <>) (list %alternate-substitute-directory %main-substitute-directory)))) - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved")) + (request-substitution (string-append (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") + "substitute-retrieved")) (call-with-input-file "substitute-retrieved" get-string-all)) (lambda () (false-if-exception (delete-file "substitute-retrieved"))))))) @@ -451,10 +466,9 @@ System: mips64el-linux\n") (map (cut string-append "file://" <>) (list %alternate-substitute-directory %main-substitute-directory)))) - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved")) + (request-substitution (string-append (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") + "substitute-retrieved")) (call-with-input-file "substitute-retrieved" get-string-all)) (lambda () (false-if-exception (delete-file "substitute-retrieved"))))))) @@ -470,10 +484,12 @@ System: mips64el-linux\n") #:public-key %wrong-public-key)) %main-substitute-directory - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved")))) + (with-input-from-string (string-append "substitute " + (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo" + " substitute-retrieved\n") + (lambda () + (guix-substitute "--substitute")))))) (test-equal "substitute, narinfo with several URLs" "Substitutable data." @@ -513,10 +529,9 @@ System: mips64el-linux\n"))) (parameterize ((substitute-urls (list (string-append "file://" %main-substitute-directory)))) - (guix-substitute "--substitute" - (string-append (%store-prefix) - "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") - "substitute-retrieved")) + (request-substitution (string-append (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-foo") + "substitute-retrieved")) (call-with-input-file "substitute-retrieved" get-string-all)) (lambda () (false-if-exception (delete-file "substitute-retrieved"))))))) -- 2.29.2