diff --git a/mk/run-test.sh b/mk/run-test.sh index 305396c11..1a1d65930 100755 --- a/mk/run-test.sh +++ b/mk/run-test.sh @@ -27,18 +27,6 @@ run_test () { run_test -# Hack: Retry the test if it fails with “unexpected EOF reading a line” as these -# appear randomly without anyone knowing why. -# See https://github.com/NixOS/nix/issues/3605 for more info -if [[ $status -ne 0 && $status -ne 99 && \ - "$(uname)" == "Darwin" && \ - "$log" =~ "unexpected EOF reading a line" \ -]]; then - echo "$post_run_msg [${yellow}FAIL$normal] (possibly flaky, so will be retried)" - echo "$log" | sed 's/^/ /' - run_test -fi - if [ $status -eq 0 ]; then echo "$post_run_msg [${green}PASS$normal]" elif [ $status -eq 99 ]; then diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc index cb58a1f02..ea2ae210e 100644 --- a/src/libstore/build/hook-instance.cc +++ b/src/libstore/build/hook-instance.cc @@ -35,7 +35,7 @@ HookInstance::HookInstance() /* Fork the hook. */ pid = startProcess([&]() { - commonChildInit(fromHook); + commonChildInit(fromHook.writeSide.get()); if (chdir("/") == -1) throw SysError("changing into /"); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 13a977bd1..521117c68 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -292,7 +292,7 @@ void LocalDerivationGoal::closeReadPipes() if (hook) { DerivationGoal::closeReadPipes(); } else - builderOut.readSide = -1; + builderOut.close(); } @@ -802,15 +802,13 @@ void LocalDerivationGoal::startBuilder() /* Create the log file. */ Path logFile = openLogFile(); - /* Create a pipe to get the output of the builder. */ - //builderOut.create(); - - builderOut.readSide = posix_openpt(O_RDWR | O_NOCTTY); - if (!builderOut.readSide) + /* Create a pseudoterminal to get the output of the builder. */ + builderOut = posix_openpt(O_RDWR | O_NOCTTY); + if (!builderOut) throw SysError("opening pseudoterminal master"); // FIXME: not thread-safe, use ptsname_r - std::string slaveName(ptsname(builderOut.readSide.get())); + std::string slaveName = ptsname(builderOut.get()); if (buildUser) { if (chmod(slaveName.c_str(), 0600)) @@ -821,35 +819,14 @@ void LocalDerivationGoal::startBuilder() } #if __APPLE__ else { - if (grantpt(builderOut.readSide.get())) + if (grantpt(builderOut.get())) throw SysError("granting access to pseudoterminal slave"); } #endif - #if 0 - // Mount the pt in the sandbox so that the "tty" command works. - // FIXME: this doesn't work with the new devpts in the sandbox. - if (useChroot) - dirsInChroot[slaveName] = {slaveName, false}; - #endif - - if (unlockpt(builderOut.readSide.get())) + if (unlockpt(builderOut.get())) throw SysError("unlocking pseudoterminal"); - builderOut.writeSide = open(slaveName.c_str(), O_RDWR | O_NOCTTY); - if (!builderOut.writeSide) - throw SysError("opening pseudoterminal slave"); - - // Put the pt into raw mode to prevent \n -> \r\n translation. - struct termios term; - if (tcgetattr(builderOut.writeSide.get(), &term)) - throw SysError("getting pseudoterminal attributes"); - - cfmakeraw(&term); - - if (tcsetattr(builderOut.writeSide.get(), TCSANOW, &term)) - throw SysError("putting pseudoterminal into raw mode"); - buildResult.startTime = time(0); /* Fork a child to build the package. */ @@ -897,7 +874,11 @@ void LocalDerivationGoal::startBuilder() usingUserNamespace = userNamespacesSupported(); + Pipe sendPid; + sendPid.create(); + Pid helper = startProcess([&]() { + sendPid.readSide.close(); /* Drop additional groups here because we can't do it after we've created the new user namespace. FIXME: @@ -917,13 +898,14 @@ void LocalDerivationGoal::startBuilder() if (usingUserNamespace) options.cloneFlags |= CLONE_NEWUSER; - pid_t child = startProcess([&]() { runChild(); }, options); + pid_t child = startProcess([&]() { runChild(slaveName); }, options); - writeFull(builderOut.writeSide.get(), - fmt("%d %d\n", usingUserNamespace, child)); + writeFull(sendPid.writeSide.get(), fmt("%d\n", child)); _exit(0); }); + sendPid.writeSide.close(); + if (helper.wait() != 0) throw Error("unable to start build process"); @@ -935,10 +917,9 @@ void LocalDerivationGoal::startBuilder() userNamespaceSync.writeSide = -1; }); - auto ss = tokenizeString>(readLine(builderOut.readSide.get())); - assert(ss.size() == 2); - usingUserNamespace = ss[0] == "1"; - pid = string2Int(ss[1]).value(); + auto ss = tokenizeString>(readLine(sendPid.readSide.get())); + assert(ss.size() == 1); + pid = string2Int(ss[0]).value(); if (usingUserNamespace) { /* Set the UID/GID mapping of the builder's user namespace @@ -993,21 +974,20 @@ void LocalDerivationGoal::startBuilder() #endif { pid = startProcess([&]() { - runChild(); + runChild(slaveName); }); } /* parent */ pid.setSeparatePG(true); - builderOut.writeSide = -1; - worker.childStarted(shared_from_this(), {builderOut.readSide.get()}, true, true); + worker.childStarted(shared_from_this(), {builderOut.get()}, true, true); /* Check if setting up the build environment failed. */ std::vector msgs; while (true) { std::string msg = [&]() { try { - return readLine(builderOut.readSide.get()); + return readLine(builderOut.get()); } catch (Error & e) { auto status = pid.wait(); e.addTrace({}, "while waiting for the build environment for '%s' to initialize (%s, previous messages: %s)", @@ -1019,7 +999,7 @@ void LocalDerivationGoal::startBuilder() }(); if (msg.substr(0, 1) == "\2") break; if (msg.substr(0, 1) == "\1") { - FdSource source(builderOut.readSide.get()); + FdSource source(builderOut.get()); auto ex = readError(source); ex.addTrace({}, "while setting up the build environment"); throw ex; @@ -1640,7 +1620,7 @@ void setupSeccomp() } -void LocalDerivationGoal::runChild() +void LocalDerivationGoal::runChild(const Path & slaveName) { /* Warning: in the child we should absolutely not make any SQLite calls! */ @@ -1649,7 +1629,22 @@ void LocalDerivationGoal::runChild() try { /* child */ - commonChildInit(builderOut); + /* Open the slave side of the pseudoterminal. */ + AutoCloseFD builderOut = open(slaveName.c_str(), O_RDWR | O_NOCTTY); + if (!builderOut) + throw SysError("opening pseudoterminal slave"); + + // Put the pt into raw mode to prevent \n -> \r\n translation. + struct termios term; + if (tcgetattr(builderOut.get(), &term)) + throw SysError("getting pseudoterminal attributes"); + + cfmakeraw(&term); + + if (tcsetattr(builderOut.get(), TCSANOW, &term)) + throw SysError("putting pseudoterminal into raw mode"); + + commonChildInit(builderOut.get()); try { setupSeccomp(); @@ -2891,7 +2886,7 @@ void LocalDerivationGoal::deleteTmpDir(bool force) bool LocalDerivationGoal::isReadDesc(int fd) { return (hook && DerivationGoal::isReadDesc(fd)) || - (!hook && fd == builderOut.readSide.get()); + (!hook && fd == builderOut.get()); } diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 34c4e9187..4d2f1ac28 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -24,8 +24,9 @@ struct LocalDerivationGoal : public DerivationGoal /* The path of the temporary directory in the sandbox. */ Path tmpDirInSandbox; - /* Pipe for the builder's standard output/error. */ - Pipe builderOut; + /* Master side of the pseudoterminal used for the builder's + standard output/error. */ + AutoCloseFD builderOut; /* Pipe for synchronising updates to the builder namespaces. */ Pipe userNamespaceSync; @@ -168,7 +169,7 @@ struct LocalDerivationGoal : public DerivationGoal int getChildStatus() override; /* Run the builder's process. */ - void runChild(); + void runChild(const std::string & slaveName); /* Check that the derivation outputs all exist and register them as valid. */ diff --git a/src/libutil/util.cc b/src/libutil/util.cc index c1de4fb33..c605a33e6 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1968,7 +1968,7 @@ std::string showBytes(uint64_t bytes) // FIXME: move to libstore/build -void commonChildInit(Pipe & logPipe) +void commonChildInit(int stderrFd) { logger = makeSimpleLogger(); @@ -1983,7 +1983,7 @@ void commonChildInit(Pipe & logPipe) throw SysError("creating a new session"); /* Dup the write side of the logger pipe into stderr. */ - if (dup2(logPipe.writeSide.get(), STDERR_FILENO) == -1) + if (dup2(stderrFd, STDERR_FILENO) == -1) throw SysError("cannot pipe standard error into log file"); /* Dup stderr to stdout. */ diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 326c6b143..52ca36fd1 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -704,7 +704,7 @@ typedef std::function PathFilter; extern PathFilter defaultPathFilter; /* Common initialisation performed in child processes. */ -void commonChildInit(Pipe & logPipe); +void commonChildInit(int stderrFd); /* Create a Unix domain socket. */ AutoCloseFD createUnixDomainSocket();