Toggle diff (444 lines)
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<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);
+ 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);
+ }
- /* 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<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);
+ /* 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"
-
