From d4fac4be0d1bca80a8e78d217b33f887a2843b74 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Sun, 29 Nov 2020 23:21:51 +0100 Subject: daemon: Remove pre-Guix hack. * nix/libstore/build.cc (DerivationGoal::startBuilder): Remove "NIX_OUTPUT_CHECKED" hack. --- nix/libstore/build.cc | 6 ------ 1 file changed, 6 deletions(-) (limited to 'nix/libstore') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index c894d72bda..70ee354e50 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -1638,12 +1638,6 @@ void DerivationGoal::startBuilder() getdents returns the inode of the mount point). */ env["PWD"] = tmpDirInSandbox; - /* Compatibility hack with Nix <= 0.7: if this is a fixed-output - derivation, tell the builder, so that for instance `fetchurl' - can skip checking the output. On older Nixes, this environment - variable won't be set, so `fetchurl' will do the check. */ - if (fixedOutput) env["NIX_OUTPUT_CHECKED"] = "1"; - /* *Only* if this is a fixed-output derivation, propagate the values of the environment variables specified in the `impureEnvVars' attribute to the builder. This allows for -- cgit v1.2.3 From 56fc14577e6750928091b525a235fa428346d598 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Mon, 30 Nov 2020 09:57:22 +0100 Subject: daemon: Remove unneeded forward declaration. This is a followup to ee9dff34f9317509cb2b833d07a0d5e01a36a4ae. * nix/libstore/build.cc: Remove 'struct Agent' forward declaration. --- nix/libstore/build.cc | 1 - 1 file changed, 1 deletion(-) (limited to 'nix/libstore') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index 70ee354e50..8413819114 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -82,7 +82,6 @@ using std::map; /* Forward definition. */ class Worker; -struct Agent; /* A pointer to a goal. */ -- cgit v1.2.3 From 79c6614f58a57b985daf8940766319e440311db0 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Tue, 1 Dec 2020 15:01:40 +0100 Subject: daemon: Use 'Agent' to spawn 'guix substitute --query'. * 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 | 132 ++++++++++++++++++------------------ nix/libstore/build.cc | 6 +- nix/libstore/local-store.cc | 159 +++++++++++--------------------------------- nix/libstore/local-store.hh | 22 +----- tests/substitute.scm | 3 +- 5 files changed, 110 insertions(+), 212 deletions(-) (limited to 'nix/libstore') diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index feae2df9cb..5e392eaa8b 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -124,11 +124,7 @@ (define %allow-unauthenticated-substitutes? ;; 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 @@ (define* (process-query command (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. @@ -1139,68 +1138,67 @@ (define print-build-trace? ((= 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) - - ;; 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)) - - ;; 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)) - - ;; 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' - - (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)))))) + ;; 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)) + + (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) + + ;; 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)))) + + (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 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); - - 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); - } + Agent & run = *runningSubstituter; + + 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(RunningSubstituter & run); + string getLineFromSubstituter(Agent & 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 @@ (define-syntax-rule (test-quit name error-rx exp) (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 -- cgit v1.2.3 From a618a8c6203d4cf57f12873a86797b8685b11e14 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Tue, 1 Dec 2020 15:55:57 +0100 Subject: daemon: Factorize substituter agent spawning. * nix/libstore/local-store.hh (class LocalStore)[substituter]: New method. [runningSubstituter]: Turn into a shared_ptr. * nix/libstore/local-store.cc (LocalStore::querySubstitutablePaths): Call 'substituter' instead of using inline code. (LocalStore::querySubstitutablePathInfos): Likewise. (LocalStore::substituter): New method. --- nix/libstore/local-store.cc | 25 +++++++++++-------------- nix/libstore/local-store.hh | 5 ++++- 2 files changed, 15 insertions(+), 15 deletions(-) (limited to 'nix/libstore') diff --git a/nix/libstore/local-store.cc b/nix/libstore/local-store.cc index 4219573a56..c304e2ddd1 100644 --- a/nix/libstore/local-store.cc +++ b/nix/libstore/local-store.cc @@ -850,14 +850,7 @@ PathSet LocalStore::querySubstitutablePaths(const PathSet & paths) if (!settings.useSubstitutes || paths.empty()) return res; - if (!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); - } - - Agent & run = *runningSubstituter; + Agent & run = *substituter(); string s = "have "; foreach (PathSet::const_iterator, j, paths) @@ -877,18 +870,22 @@ PathSet LocalStore::querySubstitutablePaths(const PathSet & paths) } -void LocalStore::querySubstitutablePathInfos(PathSet & paths, SubstitutablePathInfos & infos) +std::shared_ptr LocalStore::substituter() { - if (!settings.useSubstitutes) return; - if (!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 = std::make_shared(settings.guixProgram, args, env); } - Agent & run = *runningSubstituter; + return runningSubstituter; +} + +void LocalStore::querySubstitutablePathInfos(PathSet & paths, SubstitutablePathInfos & infos) +{ + if (!settings.useSubstitutes) return; + + Agent & run = *substituter(); string s = "info "; foreach (PathSet::const_iterator, i, paths) diff --git a/nix/libstore/local-store.hh b/nix/libstore/local-store.hh index 57d15bac7e..9ba37219da 100644 --- a/nix/libstore/local-store.hh +++ b/nix/libstore/local-store.hh @@ -42,7 +42,10 @@ class LocalStore : public StoreAPI { private: /* The currently running substituter or empty. */ - std::unique_ptr runningSubstituter; + std::shared_ptr runningSubstituter; + + /* Ensure the substituter is running and return it. */ + std::shared_ptr substituter(); Path linksDir; -- cgit v1.2.3 From 711df9ef3c04a0e0d7e844bed4c6b260ea1f65c1 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Wed, 2 Dec 2020 16:27:34 +0100 Subject: daemon: Run 'guix substitute --substitute' as an agent. 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(-) (limited to 'nix/libstore') 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 @@ (define-module (guix scripts substitute) write-narinfo %allow-unauthenticated-substitutes? + %error-to-file-descriptor-4? substitute-urls guix-substitute)) @@ -1016,7 +1017,10 @@ (define narinfo ;; 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 @@ (define (validate-uri uri) (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 @@ (define print-build-trace? ;; 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 @@ (define print-build-trace? #: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 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); - - /* 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 env = { { "_NIX_OPTIONS", settings.pack() } }; + worker.substituter = std::make_shared(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 >(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); - - /* 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 @@ (define-syntax-rule (test-quit name error-rx exp) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) ;; 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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) %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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (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 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) #: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 @@ (define (compress input output compression) (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"))))))) -- cgit v1.2.3 From 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Wed, 2 Dec 2020 22:49:39 +0100 Subject: substitute: Cache and reuse connections while substituting. That way, when fetching a series of substitutes from the same server(s), the connection is reused instead of being closed/opened for each substitutes, which saves on network round trips and TLS handshakes. * guix/http-client.scm (http-fetch): Add #:keep-alive? and honor it. * guix/progress.scm (progress-report-port): Add #:close? parameter and honor it. * guix/scripts/substitute.scm (at-most): Return the tail as a second value. (fetch): Add #:port and #:keep-alive? and honor them. (%max-cached-connections): New variable. (open-connection-for-uri/cached, call-with-cached-connection): New procedures. (with-cached-connection): New macro. (process-substitution): Wrap 'fetch' call in 'with-cached-connection'. Pass #:close? to 'progress-report-port'. --- guix/http-client.scm | 12 +++--- guix/progress.scm | 8 ++-- guix/scripts/substitute.scm | 103 +++++++++++++++++++++++++++++++++++++------- nix/libstore/build.cc | 29 ++++++++----- 4 files changed, 117 insertions(+), 35 deletions(-) (limited to 'nix/libstore') diff --git a/guix/http-client.scm b/guix/http-client.scm index a767175d67..553640fe9e 100644 --- a/guix/http-client.scm +++ b/guix/http-client.scm @@ -1,5 +1,5 @@ ;;; GNU Guix --- Functional package management for GNU -;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018 Ludovic Courtès +;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2020 Ludovic Courtès ;;; Copyright © 2015 Mark H Weaver ;;; Copyright © 2012, 2015 Free Software Foundation, Inc. ;;; Copyright © 2017 Tobias Geerinckx-Rice @@ -70,6 +70,7 @@ (define-condition-type &http-get-error &error (define* (http-fetch uri #:key port (text? #f) (buffered? #t) + (keep-alive? #f) (verify-certificate? #t) (headers '((user-agent . "GNU Guile"))) timeout) @@ -79,6 +80,9 @@ (define* (http-fetch uri #:key port (text? #f) (buffered? #t) unbuffered port, suitable for use in `filtered-port'. HEADERS is an alist of extra HTTP headers. +When KEEP-ALIVE? is true, the connection is marked as 'keep-alive' and PORT is +not closed upon completion. + When VERIFY-CERTIFICATE? is true, verify HTTPS server certificates. TIMEOUT specifies the timeout in seconds for connection establishment; when @@ -104,11 +108,7 @@ (define* (http-fetch uri #:key port (text? #f) (buffered? #t) (setvbuf port 'none)) (let*-values (((resp data) (http-get uri #:streaming? #t #:port port - ;; XXX: When #:keep-alive? is true, if DATA is - ;; a chunked-encoding port, closing DATA won't - ;; close PORT, leading to a file descriptor - ;; leak. - #:keep-alive? #f + #:keep-alive? keep-alive? #:headers headers)) ((code) (response-code resp))) diff --git a/guix/progress.scm b/guix/progress.scm index fec65b424c..cd80ae620a 100644 --- a/guix/progress.scm +++ b/guix/progress.scm @@ -337,9 +337,10 @@ (define buffer (report total) (loop total (get-bytevector-n! in buffer 0 buffer-size)))))))) -(define (progress-report-port reporter port) +(define* (progress-report-port reporter port #:key (close? #t)) "Return a port that continuously reports the bytes read from PORT using -REPORTER, which should be a object." +REPORTER, which should be a object. When CLOSE? is true, +PORT is closed when the returned port is closed." (match reporter (($ start report stop) (let* ((total 0) @@ -364,5 +365,6 @@ (define (progress-report-port reporter port) ;; trace. (unless (zero? total) (stop)) - (close-port port))))))) + (when close? + (close-port port)))))))) diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index 73abd3f029..25075eedff 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -188,9 +188,14 @@ (define-syntax-rule (with-timeout duration handler body ...) (sigaction SIGALRM SIG_DFL) (apply values result))))) -(define* (fetch uri #:key (buffered? #t) (timeout? #t)) +(define* (fetch uri #:key (buffered? #t) (timeout? #t) + (keep-alive? #f) (port #f)) "Return a binary input port to URI and the number of bytes it's expected to -provide." +provide. + +When PORT is true, use it as the underlying I/O port for HTTP transfers; when +PORT is false, open a new connection for URI. When KEEP-ALIVE? is true, the +connection (typically PORT) is kept open once data has been fetched from URI." (case (uri-scheme uri) ((file) (let ((port (open-file (uri-path uri) @@ -206,7 +211,7 @@ (define* (fetch uri #:key (buffered? #t) (timeout? #t)) ;; sudo tc qdisc add dev eth0 root netem delay 1500ms ;; and then cancel with: ;; sudo tc qdisc del dev eth0 root - (let ((port #f)) + (let ((port port)) (with-timeout (if timeout? %fetch-timeout 0) @@ -217,10 +222,11 @@ (define* (fetch uri #:key (buffered? #t) (timeout? #t)) (begin (when (or (not port) (port-closed? port)) (set! port (guix:open-connection-for-uri - uri #:verify-certificate? #f)) - (unless (or buffered? (not (file-port? port))) - (setvbuf port 'none))) + uri #:verify-certificate? #f))) + (unless (or buffered? (not (file-port? port))) + (setvbuf port 'none)) (http-fetch uri #:text? #f #:port port + #:keep-alive? keep-alive? #:verify-certificate? #f)))))) (else (leave (G_ "unsupported substitute URI scheme: ~a~%") @@ -478,17 +484,17 @@ (define (narinfo-request cache-url path) (build-request (string->uri url) #:method 'GET #:headers headers))) (define (at-most max-length lst) - "If LST is shorter than MAX-LENGTH, return it; otherwise return its -MAX-LENGTH first elements." + "If LST is shorter than MAX-LENGTH, return it and the empty list; otherwise +return its MAX-LENGTH first elements and its tail." (let loop ((len 0) (lst lst) (result '())) (match lst (() - (reverse result)) + (values (reverse result) '())) ((head . tail) (if (>= len max-length) - (reverse result) + (values (reverse result) lst) (loop (+ 1 len) tail (cons head result))))))) (define* (http-multiple-get base-uri proc seed requests @@ -962,6 +968,68 @@ (define (file-sizestring uri))) (let*-values (((raw download-size) - ;; Note that Hydra currently generates Nars on the fly - ;; and doesn't specify a Content-Length, so - ;; DOWNLOAD-SIZE is #f in practice. - (fetch uri #:buffered? #f #:timeout? #f)) + ;; 'guix publish' without '--cache' doesn't specify a + ;; Content-Length, so DOWNLOAD-SIZE is #f in this case. + (with-cached-connection uri port + (fetch uri #:buffered? #f #:timeout? #f + #:port port + #:keep-alive? #t))) ((progress) (let* ((dl-size (or download-size (and (equal? compression "none") @@ -1001,7 +1071,9 @@ (define narinfo (uri->string uri) dl-size (current-error-port) #:abbreviation nar-uri-abbreviation)))) - (progress-report-port reporter raw))) + ;; Keep RAW open upon completion so we can later reuse + ;; the underlying connection. + (progress-report-port reporter raw #:close? #f))) ((input pids) ;; NOTE: This 'progress' port of current process will be ;; closed here, while the child process doing the @@ -1218,6 +1290,7 @@ (define print-build-trace? ;;; Local Variables: ;;; eval: (put 'with-timeout 'scheme-indent-function 1) +;;; eval: (put 'with-cached-connection 'scheme-indent-function 2) ;;; End: ;;; substitute.scm ends here diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index 50d300253d..6cfe7aba7e 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -3114,17 +3114,24 @@ void SubstitutionGoal::handleChildOutput(int fd, const string & data) } 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); + /* DATA may consist of several lines. Process them one by one. */ + string input = data; + while (!input.empty()) { + /* Process up to the first newline. */ + size_t end = input.find_first_of("\n"); + string trimmed = (end != string::npos) ? input.substr(0, end) : input; + + /* Update the goal's state accordingly. */ + if (expectedHashStr == "") { + expectedHashStr = trimmed; + } else if (status == "") { + status = trimmed; + worker.wakeUp(shared_from_this()); + } else { + printMsg(lvlError, format("unexpected substituter message '%1%'") % input); + } + + input = (end != string::npos) ? input.substr(end + 1) : ""; } } } -- cgit v1.2.3 From bfe4cdf88ee3e88910d22291a4c745462f2d6417 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Thu, 3 Dec 2020 09:44:22 +0100 Subject: daemon: Raise an error if substituter doesn't send the expected hash. It was already impossible in practice for 'expectedHashStr' to be empty if 'status' == "success". * nix/libstore/build.cc (SubstitutionGoal::finished): Throw 'SubstError' when 'expectedHashStr' is empty. --- nix/libstore/build.cc | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) (limited to 'nix/libstore') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index 6cfe7aba7e..b5551b87ae 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -3040,27 +3040,28 @@ void SubstitutionGoal::finished() if (!pathExists(destPath)) throw SubstError(format("substitute did not produce path `%1%'") % destPath); + if (expectedHashStr == "") + throw SubstError(format("substituter did not communicate hash for `%1'") % storePath); + hash = hashPath(htSHA256, destPath); /* Verify the expected hash we got from the substituer. */ - if (expectedHashStr != "") { - size_t n = expectedHashStr.find(':'); - if (n == string::npos) - throw Error(format("bad hash from substituter: %1%") % expectedHashStr); - HashType hashType = parseHashType(string(expectedHashStr, 0, n)); - if (hashType == htUnknown) - throw Error(format("unknown hash algorithm in `%1%'") % expectedHashStr); - Hash expectedHash = parseHash16or32(hashType, string(expectedHashStr, n + 1)); - Hash actualHash = hashType == htSHA256 ? hash.first : hashPath(hashType, destPath).first; - if (expectedHash != actualHash) { - if (settings.printBuildTrace) - printMsg(lvlError, format("@ hash-mismatch %1% %2% %3% %4%") - % storePath % "sha256" - % printHash16or32(expectedHash) - % printHash16or32(actualHash)); - throw SubstError(format("hash mismatch for substituted item `%1%'") % storePath); - } - } + size_t n = expectedHashStr.find(':'); + if (n == string::npos) + throw Error(format("bad hash from substituter: %1%") % expectedHashStr); + HashType hashType = parseHashType(string(expectedHashStr, 0, n)); + if (hashType == htUnknown) + throw Error(format("unknown hash algorithm in `%1%'") % expectedHashStr); + Hash expectedHash = parseHash16or32(hashType, string(expectedHashStr, n + 1)); + Hash actualHash = hashType == htSHA256 ? hash.first : hashPath(hashType, destPath).first; + if (expectedHash != actualHash) { + if (settings.printBuildTrace) + printMsg(lvlError, format("@ hash-mismatch %1% %2% %3% %4%") + % storePath % "sha256" + % printHash16or32(expectedHash) + % printHash16or32(actualHash)); + throw SubstError(format("hash mismatch for substituted item `%1%'") % storePath); + } } catch (SubstError & e) { -- cgit v1.2.3 From 9dfa20a22ae0be3d3b01a7b3d422af97428c627e Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Sun, 13 Dec 2020 22:46:03 +0100 Subject: daemon: Let 'guix substitute' perform hash checks. This way, the hash of the store item can be computed as it is restored, thereby avoiding an additional file tree traversal ('hashPath' call) later on in the daemon. Consequently, it should reduce latency between subsequent substitute downloads. This is a followup to 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203. * guix/scripts/substitute.scm (narinfo-hash-algorithm+value): New procedure. (process-substitution): Wrap INPUT into a hash input port, 'hashed', and read from it. Compare the actual and expected hashes, and print a "hash-mismatch" status line when they differ. When they match, print not just "success" but also the nar hash and size. * nix/libstore/build.cc (class SubstitutionGoal)[expectedHashStr]: Remove. (SubstitutionGoal::finished): Tokenize 'status'. Parse it and handle "success" and "hash-mismatch" accordingly. Call 'hashPath' only when the returned hash is not SHA256. (SubstitutionGoal::handleChildOutput): Remove 'expectedHashStr' handling. * tests/substitute.scm ("substitute, invalid hash"): Rename to... ("substitute, invalid narinfo hash"): ... this. ("substitute, invalid hash"): New test. --- guix/scripts/substitute.scm | 45 +++++++++++++++++++++++----- nix/libstore/build.cc | 73 ++++++++++++++++++++++++--------------------- tests/substitute.scm | 50 +++++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 44 deletions(-) (limited to 'nix/libstore') diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index 25075eedff..17d0002b9f 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -26,6 +26,8 @@ (define-module (guix scripts substitute) #:use-module (guix combinators) #:use-module (guix config) #:use-module (guix records) + #:use-module (guix diagnostics) + #:use-module (guix i18n) #:use-module ((guix serialization) #:select (restore-file)) #:autoload (guix scripts discover) (read-substitute-urls) #:use-module (gcrypt hash) @@ -256,6 +258,18 @@ (define-record-type ;; for more information. (contents narinfo-contents)) +(define (narinfo-hash-algorithm+value narinfo) + "Return two values: the hash algorithm used by NARINFO and its value as a +bytevector." + (match (string-tokenize (narinfo-hash narinfo) + (char-set-complement (char-set #\:))) + ((algorithm base32) + (values (lookup-hash-algorithm (string->symbol algorithm)) + (nix-base32-string->bytevector base32))) + (_ + (raise (formatted-message + (G_ "invalid narinfo hash: ~s") (narinfo-hash narinfo)))))) + (define (narinfo-hash->sha256 hash) "If the string HASH denotes a sha256 hash, return it as a bytevector. Otherwise return #f." @@ -1033,7 +1047,9 @@ (define-syntax-rule (with-cached-connection uri port exp ...) (define* (process-substitution store-item destination #:key cache-urls acl print-build-trace?) "Substitute STORE-ITEM (a store file name) from CACHE-URLS, and write it to -DESTINATION as a nar file. Verify the substitute against ACL." +DESTINATION as a nar file. Verify the substitute against ACL, and verify its +hash against what appears in the narinfo. Print a status line on the current +output port." (define narinfo (lookup-narinfo cache-urls store-item (cut valid-narinfo? <> acl))) @@ -1044,9 +1060,6 @@ (define narinfo (let-values (((uri compression file-size) (narinfo-best-uri narinfo))) - ;; Tell the daemon what the expected hash of the Nar itself is. - (format #t "~a~%" (narinfo-hash narinfo)) - (unless print-build-trace? (format (current-error-port) (G_ "Downloading ~a...~%") (uri->string uri))) @@ -1079,9 +1092,16 @@ (define narinfo ;; closed here, while the child process doing the ;; reporting will close it upon exit. (decompressed-port (string->symbol compression) - progress))) + progress)) + + ;; Compute the actual nar hash as we read it. + ((algorithm expected) + (narinfo-hash-algorithm+value narinfo)) + ((hashed get-hash) + (open-hash-input-port algorithm input))) ;; Unpack the Nar at INPUT into DESTINATION. - (restore-file input destination) + (restore-file hashed destination) + (close-port hashed) (close-port input) ;; Wait for the reporter to finish. @@ -1091,8 +1111,17 @@ (define narinfo ;; one to visually separate substitutions. (display "\n\n" (current-error-port)) - ;; Tell the daemon that we're done. - (display "success\n" (current-output-port))))) + ;; Check whether we got the data announced in NARINFO. + (let ((actual (get-hash))) + (if (bytevector=? actual expected) + ;; Tell the daemon that we're done. + (format (current-output-port) "success ~a ~a~%" + (narinfo-hash narinfo) (narinfo-size narinfo)) + ;; The actual data has a different hash than that in NARINFO. + (format (current-output-port) "hash-mismatch ~a ~a ~a~%" + (hash-algorithm-name algorithm) + (bytevector->nix-base32-string expected) + (bytevector->nix-base32-string actual))))))) ;;; diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index b5551b87ae..b19471a68f 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -2790,10 +2790,6 @@ private: /* 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; @@ -3032,36 +3028,47 @@ void SubstitutionGoal::finished() /* Check the exit status and the build result. */ HashResult hash; try { - - 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); - - if (expectedHashStr == "") - throw SubstError(format("substituter did not communicate hash for `%1'") % storePath); - - hash = hashPath(htSHA256, destPath); - - /* Verify the expected hash we got from the substituer. */ - size_t n = expectedHashStr.find(':'); - if (n == string::npos) - throw Error(format("bad hash from substituter: %1%") % expectedHashStr); - HashType hashType = parseHashType(string(expectedHashStr, 0, n)); - if (hashType == htUnknown) - throw Error(format("unknown hash algorithm in `%1%'") % expectedHashStr); - Hash expectedHash = parseHash16or32(hashType, string(expectedHashStr, n + 1)); - Hash actualHash = hashType == htSHA256 ? hash.first : hashPath(hashType, destPath).first; - if (expectedHash != actualHash) { - if (settings.printBuildTrace) + auto statusList = tokenizeString >(status); + + if (statusList.empty()) { + throw SubstError(format("fetching path `%1%' (empty status: '%2%')") + % storePath % status); + } else if (statusList[0] == "hash-mismatch") { + if (settings.printBuildTrace) { + auto hashType = statusList[1]; + auto expectedHash = statusList[2]; + auto actualHash = statusList[3]; printMsg(lvlError, format("@ hash-mismatch %1% %2% %3% %4%") - % storePath % "sha256" - % printHash16or32(expectedHash) - % printHash16or32(actualHash)); + % storePath + % hashType % expectedHash % actualHash); + } throw SubstError(format("hash mismatch for substituted item `%1%'") % storePath); + } else if (statusList[0] == "success") { + if (!pathExists(destPath)) + throw SubstError(format("substitute did not produce path `%1%'") % destPath); + + std::string hashStr = statusList[1]; + size_t n = hashStr.find(':'); + if (n == string::npos) + throw Error(format("bad hash from substituter: %1%") % hashStr); + + HashType hashType = parseHashType(string(hashStr, 0, n)); + switch (hashType) { + case htUnknown: + throw Error(format("unknown hash algorithm in `%1%'") % hashStr); + case htSHA256: + hash.first = parseHash16or32(hashType, string(hashStr, n + 1)); + hash.second = std::atoi(statusList[2].c_str()); + break; + default: + /* The database only stores SHA256 hashes, so compute it. */ + hash = hashPath(htSHA256, destPath); + break; + } } + else + throw SubstError(format("fetching path `%1%' (status: '%2%')") + % storePath % status); } catch (SubstError & e) { @@ -3123,9 +3130,7 @@ void SubstitutionGoal::handleChildOutput(int fd, const string & data) string trimmed = (end != string::npos) ? input.substr(0, end) : input; /* Update the goal's state accordingly. */ - if (expectedHashStr == "") { - expectedHashStr = trimmed; - } else if (status == "") { + if (status == "") { status = trimmed; worker.wakeUp(shared_from_this()); } else { diff --git a/tests/substitute.scm b/tests/substitute.scm index b86ce09425..5b42632552 100644 --- a/tests/substitute.scm +++ b/tests/substitute.scm @@ -28,7 +28,9 @@ (define-module (test-substitute) #:use-module (guix base32) #:use-module ((guix store) #:select (%store-prefix)) #:use-module ((guix ui) #:select (guix-warning-port)) - #:use-module ((guix utils) #:select (call-with-compressed-output-port)) + #:use-module ((guix utils) + #:select (call-with-temporary-directory + call-with-compressed-output-port)) #:use-module ((guix build utils) #:select (mkdir-p delete-file-recursively dump-port)) #:use-module (guix tests http) @@ -36,6 +38,7 @@ (define-module (test-substitute) #:use-module (rnrs io ports) #:use-module (web uri) #:use-module (ice-9 regex) + #:use-module (srfi srfi-11) #:use-module (srfi srfi-26) #:use-module (srfi srfi-34) #:use-module (srfi srfi-35) @@ -304,7 +307,7 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (lambda () (guix-substitute "--substitute"))))) -(test-quit "substitute, invalid hash" +(test-quit "substitute, invalid narinfo hash" "no valid substitute" ;; The hash in the signature differs from the hash of %NARINFO. (with-narinfo (string-append %narinfo "Signature: " @@ -317,6 +320,49 @@ (define-syntax-rule (with-narinfo* narinfo directory body ...) (lambda () (guix-substitute "--substitute"))))) +(test-equal "substitute, invalid hash" + (string-append "hash-mismatch sha256 " + (bytevector->nix-base32-string (sha256 #vu8())) " " + (let-values (((port get-hash) + (open-hash-port (hash-algorithm sha256))) + ((content) + "Substitutable data.")) + (write-file-tree "foo" port + #:file-type+size + (lambda _ + (values 'regular + (string-length content))) + #:file-port + (lambda _ + (open-input-string content))) + (close-port port) + (bytevector->nix-base32-string (get-hash))) + "\n") + + ;; Arrange so the actual data hash does not match the 'NarHash' field in the + ;; narinfo. + (with-output-to-string + (lambda () + (let ((narinfo (string-append "StorePath: " (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-wrong-hash +URL: example.nar +Compression: none +NarHash: sha256:" (bytevector->nix-base32-string (sha256 #vu8())) " +NarSize: 42 +References: +Deriver: " (%store-prefix) "/foo.drv +System: mips64el-linux\n"))) + (with-narinfo (string-append narinfo "Signature: " + (signature-field narinfo) "\n") + (call-with-temporary-directory + (lambda (directory) + (with-input-from-string (string-append + "substitute " (%store-prefix) + "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-wrong-hash " + directory "/wrong-hash\n") + (lambda () + (guix-substitute "--substitute")))))))))) + (test-quit "substitute, unauthorized key" "no valid substitute" (with-narinfo (string-append %narinfo "Signature: " -- cgit v1.2.3 From 15cf28fbb4672b05a0a0f430dc496c7eada18794 Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Mon, 14 Dec 2020 15:11:51 +0100 Subject: daemon: Do not reset timestamps and permissions on substituted items. 'guix substitute' now takes care of it via 'restore-file'. * nix/libstore/build.cc (SubstitutionGoal::finished): Remove call to 'canonicalisePathMetaData'. --- nix/libstore/build.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'nix/libstore') diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index b19471a68f..ea809c6971 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -3085,7 +3085,8 @@ void SubstitutionGoal::finished() if (repair) replaceValidPath(storePath, destPath); - canonicalisePathMetaData(storePath, -1); + /* Note: 'guix substitute' takes care of resetting timestamps and + permissions on 'destPath', so no need to do it here. */ worker.store.optimisePath(storePath); // FIXME: combine with hashPath() -- cgit v1.2.3 From c7c7f068c15e419aaf5ef616516aa5ad4e55c2fa Mon Sep 17 00:00:00 2001 From: Ludovic Courtès Date: Mon, 14 Dec 2020 17:59:32 +0100 Subject: daemon: Delegate deduplication to 'guix substitute'. This removes the main source of latency between subsequent downloads. * nix/libstore/build.cc (SubstitutionGoal::tryToRun): Add a "deduplicate" key to ENV. (SubstitutionGoal::finished): Remove call to 'optimisePath'. * guix/scripts/substitute.scm (process-substitution)[destination-in-store?] [dump-file/deduplicate*]: New variables. Pass #:dump-file to 'restore-file'. * guix/scripts/substitute.scm (guix-substitute)[deduplicate?]: New variable. Pass #:deduplicate? to 'process-substitution'. * guix/serialization.scm (dump-file): Export and augment 'dump-file'. --- guix/scripts/substitute.scm | 31 ++++++++++++++++++++++++++----- guix/serialization.scm | 8 ++++++-- nix/libstore/build.cc | 13 ++++++++----- 3 files changed, 40 insertions(+), 12 deletions(-) (limited to 'nix/libstore') diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index 17d0002b9f..38702d0c4b 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -28,7 +28,8 @@ (define-module (guix scripts substitute) #:use-module (guix records) #:use-module (guix diagnostics) #:use-module (guix i18n) - #:use-module ((guix serialization) #:select (restore-file)) + #:use-module ((guix serialization) #:select (restore-file dump-file)) + #:autoload (guix store deduplication) (dump-file/deduplicate) #:autoload (guix scripts discover) (read-substitute-urls) #:use-module (gcrypt hash) #:use-module (guix base32) @@ -1045,15 +1046,27 @@ (define-syntax-rule (with-cached-connection uri port exp ...) (call-with-cached-connection uri (lambda (port) exp ...))) (define* (process-substitution store-item destination - #:key cache-urls acl print-build-trace?) + #:key cache-urls acl + deduplicate? print-build-trace?) "Substitute STORE-ITEM (a store file name) from CACHE-URLS, and write it to DESTINATION as a nar file. Verify the substitute against ACL, and verify its -hash against what appears in the narinfo. Print a status line on the current -output port." +hash against what appears in the narinfo. When DEDUPLICATE? is true, and if +DESTINATION is in the store, deduplicate its files. Print a status line on +the current output port." (define narinfo (lookup-narinfo cache-urls store-item (cut valid-narinfo? <> acl))) + (define destination-in-store? + (string-prefix? (string-append (%store-prefix) "/") + destination)) + + (define (dump-file/deduplicate* . args) + ;; Make sure deduplication looks at the right store (necessary in test + ;; environments). + (apply dump-file/deduplicate + (append args (list #:store (%store-prefix))))) + (unless narinfo (leave (G_ "no valid substitute for '~a'~%") store-item)) @@ -1100,7 +1113,11 @@ (define narinfo ((hashed get-hash) (open-hash-input-port algorithm input))) ;; Unpack the Nar at INPUT into DESTINATION. - (restore-file hashed destination) + (restore-file hashed destination + #:dump-file (if (and destination-in-store? + deduplicate?) + dump-file/deduplicate* + dump-file)) (close-port hashed) (close-port input) @@ -1248,6 +1265,9 @@ (define print-build-trace? ((= string->number number) (> number 0)) (_ #f))) + (define deduplicate? + (find-daemon-option "deduplicate")) + ;; The daemon's agent code opens file descriptor 4 for us and this is where ;; stderr should go. (parameterize ((current-error-port (if (%error-to-file-descriptor-4?) @@ -1307,6 +1327,7 @@ (define print-build-trace? (process-substitution store-path destination #:cache-urls (substitute-urls) #:acl (current-acl) + #:deduplicate? deduplicate? #:print-build-trace? print-build-trace?) (loop)))))) diff --git a/guix/serialization.scm b/guix/serialization.scm index 9e2dce8bb0..59cd93fb18 100644 --- a/guix/serialization.scm +++ b/guix/serialization.scm @@ -51,7 +51,8 @@ (define-module (guix serialization) write-file write-file-tree fold-archive - restore-file)) + restore-file + dump-file)) ;;; Comment: ;;; @@ -458,7 +459,10 @@ (define (read-eof-marker) (&nar-read-error (port port) (file file) (token x))))))))) (define (dump-file file input size type) - "Dump SIZE bytes from INPUT to FILE." + "Dump SIZE bytes from INPUT to FILE. + +This procedure is suitable for use as the #:dump-file argument to +'restore-file'." (call-with-output-file file (lambda (output) (dump input output size)))) diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc index ea809c6971..20d83fea4a 100644 --- a/nix/libstore/build.cc +++ b/nix/libstore/build.cc @@ -2984,7 +2984,12 @@ void SubstitutionGoal::tryToRun() if (!worker.substituter) { const Strings args = { "substitute", "--substitute" }; - const std::map env = { { "_NIX_OPTIONS", settings.pack() } }; + const std::map env = { + { "_NIX_OPTIONS", + settings.pack() + "deduplicate=" + + (settings.autoOptimiseStore ? "yes" : "no") + } + }; worker.substituter = std::make_shared(settings.guixProgram, args, env); } @@ -3085,10 +3090,8 @@ void SubstitutionGoal::finished() if (repair) replaceValidPath(storePath, destPath); - /* Note: 'guix substitute' takes care of resetting timestamps and - permissions on 'destPath', so no need to do it here. */ - - worker.store.optimisePath(storePath); // FIXME: combine with hashPath() + /* Note: 'guix substitute' takes care of resetting timestamps and of + deduplicating 'destPath', so no need to do it here. */ ValidPathInfo info2; info2.path = storePath; -- cgit v1.2.3