diff options
| -rwxr-xr-x | guix/scripts/substitute.scm | 34 | ||||
| -rw-r--r-- | nix/libstore/build.cc | 129 | ||||
| -rw-r--r-- | tests/substitute.scm | 95 |
3 files changed, 145 insertions, 113 deletions
diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index 5e392eaa8b..73abd3f029 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))))) ;;; @@ -1127,6 +1131,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") @@ -1140,9 +1149,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)) @@ -1184,15 +1193,22 @@ default value." #:cache-urls (substitute-urls) #:acl acl) (loop (read-line))))))) - (("--substitute" store-path destination) + (("--substitute") ;; 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?))) + (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<Agent> hook; + std::shared_ptr<Agent> 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<PathLocks> outputLock; @@ -2795,6 +2787,17 @@ private: typedef void (SubstitutionGoal::*GoalState)(); GoalState state; + /* The substituter. */ + std::shared_ptr<Agent> 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); - - /* 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) - throw SysError("cannot dup output pipe into stdout"); + if (!worker.substituter) { + const Strings args = { "substitute", "--substitute" }; + const std::map<string, string> env = { { "_NIX_OPTIONS", settings.pack() } }; + worker.substituter = std::make_shared<Agent>(settings.guixProgram, args, env); + } - execv(settings.guixProgram.c_str(), stringsToCharPtrs(args).data()); + /* Borrow the worker's substituter. */ + if (!substituter) substituter.swap(worker.substituter); - throw SysError(format("executing `%1% substitute'") % settings.guixProgram); - }); + /* Send the request to the substituter. */ + writeLine(substituter->toAgent.writeSide, + (format("substitute %1% %2%") % storePath % destPath).str()); - pid.setSeparatePG(true); - pid.setKillSignal(SIGTERM); - outPipe.writeSide.close(); - logPipe.writeSide.close(); - worker.childStarted(shared_from_this(), - pid, singleton<set<int> >(logPipe.readSide), true, true); + set<int> 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); - - /* So the child is gone now. */ - worker.childTerminated(savedPid); - - /* Close the read side of the logger pipe. */ - logPipe.readSide.close(); + /* Remove the 'guix substitute' process from the list of children. */ + worker.childTerminated(substituter->pid); - /* 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"))))))) |