From 06c4929958c60b085cbe18a558df9ef58c8f8689 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 8 Dec 2006 17:26:21 +0000 Subject: [PATCH] * Some refactoring. * Throw more exceptions as BuildErrors instead of Errors. This matters when --keep-going is turned on. (A BuildError is caught and terminates the goal in question, an Error terminates the program.) --- src/libstore/build.cc | 213 +++++++++++++++++++++++------------------- 1 file changed, 118 insertions(+), 95 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index cff114a182..31b2876ab0 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -99,7 +99,7 @@ public: void addWaitee(GoalPtr waitee); - virtual void waiteeDone(GoalPtr waitee, bool success); + virtual void waiteeDone(GoalPtr waitee, ExitCode result); virtual void handleChildOutput(int fd, const string & data) { @@ -123,8 +123,13 @@ public: return exitCode; } + void cancel() + { + amDone(ecFailed); + } + protected: - void amDone(bool success = true); + void amDone(ExitCode result); }; @@ -189,13 +194,24 @@ public: /* Can we start another child process? */ bool canBuildMore(); - /* Registers / unregisters a running child process. */ + /* Registers a running child process. `inBuildSlot' means that + the process counts towards the jobs limit. */ void childStarted(GoalPtr goal, pid_t pid, const set & fds, bool inBuildSlot); + + /* Unregisters a running child process. `wakeSleepers' should be + false if there is no sense in waking up goals that are sleeping + because they can't run yet (e.g., there is no free build slot, + or the hook would still say `postpone'). */ void childTerminated(pid_t pid, bool wakeSleepers = true); - /* Add a goal to the set of goals waiting for a build slot. */ - void waitForBuildSlot(GoalPtr goal, bool reallyWait = false); + /* Put `goal' to sleep until a build slot becomes available (which + might be right away). */ + void waitForBuildSlot(GoalPtr goal); + + /* Put `goal' to sleep until a child process terminates, i.e., a + call is made to childTerminate(..., true). */ + void waitForChildTermination(GoalPtr goal); /* Loop until the specified top-level goals have finished. */ void run(const Goals & topGoals); @@ -205,19 +221,8 @@ public: }; -class SubstError : public Error -{ -public: - SubstError(const format & f) : Error(f) { }; -}; - - -class BuildError : public Error -{ -public: - BuildError(const format & f) : Error(f) { }; -}; - +MakeError(SubstError, Error) +MakeError(BuildError, Error) ////////////////////////////////////////////////////////////////////// @@ -230,7 +235,7 @@ void Goal::addWaitee(GoalPtr waitee) } -void Goal::waiteeDone(GoalPtr waitee, bool success) +void Goal::waiteeDone(GoalPtr waitee, ExitCode result) { assert(waitees.find(waitee) != waitees.end()); waitees.erase(waitee); @@ -238,9 +243,9 @@ void Goal::waiteeDone(GoalPtr waitee, bool success) trace(format("waitee `%1%' done; %2% left") % waitee->name % waitees.size()); - if (!success) ++nrFailed; + if (result == ecFailed) ++nrFailed; - if (waitees.empty() || (!success && !keepGoing)) { + if (waitees.empty() || (result == ecFailed && !keepGoing)) { /* If we failed and keepGoing is not set, we remove all remaining waitees. */ @@ -260,14 +265,15 @@ void Goal::waiteeDone(GoalPtr waitee, bool success) } -void Goal::amDone(bool success) +void Goal::amDone(ExitCode result) { trace("done"); assert(exitCode == ecBusy); - exitCode = success ? ecSuccess : ecFailed; + assert(result == ecSuccess || result == ecFailed); + exitCode = result; for (WeakGoals::iterator i = waiters.begin(); i != waiters.end(); ++i) { GoalPtr goal = i->lock(); - if (goal) goal->waiteeDone(shared_from_this(), success); + if (goal) goal->waiteeDone(shared_from_this(), result); } waiters.clear(); worker.removeGoal(shared_from_this()); @@ -439,7 +445,7 @@ void UserLock::acquire() } } - throw Error(format("all build users are currently in use; " + throw BuildError(format("all build users are currently in use; " "consider creating additional users and adding them to the `%1%' group") % buildUsersGroup); } @@ -715,7 +721,7 @@ void DerivationGoal::haveDerivation() printMsg(lvlError, format("cannot build missing derivation `%1%'") % drvPath); - amDone(false); + amDone(ecFailed); return; } @@ -733,7 +739,7 @@ void DerivationGoal::haveDerivation() /* If they are all valid, then we're done. */ if (invalidOutputs.size() == 0) { - amDone(true); + amDone(ecSuccess); return; } @@ -764,7 +770,7 @@ void DerivationGoal::outputsSubstituted() nrFailed = 0; if (checkPathValidity(false).size() == 0) { - amDone(true); + amDone(ecSuccess); return; } @@ -794,7 +800,7 @@ void DerivationGoal::inputsRealised() format("cannot build derivation `%1%': " "%2% inputs could not be realised") % drvPath % nrFailed); - amDone(false); + amDone(ecFailed); return; } @@ -821,14 +827,14 @@ void DerivationGoal::tryToBuild() return; case rpPostpone: /* Not now; wait until at least one child finishes. */ - worker.waitForBuildSlot(shared_from_this(), true); + worker.waitForChildTermination(shared_from_this()); return; case rpDecline: /* We should do it ourselves. */ break; case rpDone: /* Somebody else did it. */ - amDone(); + amDone(ecSuccess); return; } @@ -841,7 +847,7 @@ void DerivationGoal::tryToBuild() /* Acquire locks and such. If we then see that the build has been done by somebody else, we're done. */ if (!prepareBuild()) { - amDone(); + amDone(ecSuccess); return; } @@ -850,7 +856,7 @@ void DerivationGoal::tryToBuild() } catch (BuildError & e) { printMsg(lvlError, e.msg()); - amDone(false); + amDone(ecFailed); return; } @@ -892,61 +898,62 @@ void DerivationGoal::buildDone() if (buildUser.enabled()) buildUser.kill(); - /* Some cleanup per path. We do this here and not in - computeClosure() for convenience when the build has failed. */ - for (DerivationOutputs::iterator i = drv.outputs.begin(); - i != drv.outputs.end(); ++i) - { - Path path = i->second.path; - if (!pathExists(path)) continue; + try { - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError(format("getting attributes of path `%1%'") % path); + /* Some cleanup per path. We do this here and not in + computeClosure() for convenience when the build has + failed. */ + for (DerivationOutputs::iterator i = drv.outputs.begin(); + i != drv.outputs.end(); ++i) + { + Path path = i->second.path; + if (!pathExists(path)) continue; + + struct stat st; + if (lstat(path.c_str(), &st) == -1) + throw SysError(format("getting attributes of path `%1%'") % path); #ifndef __CYGWIN__ - /* Check that the output is not group or world writable, as - that means that someone else can have interfered with the - build. Also, the output should be owned by the build - user. */ - if ((st.st_mode & (S_IWGRP | S_IWOTH)) || - (buildUser.enabled() && st.st_uid != buildUser.getUID())) - throw Error(format("suspicious ownership or permission on `%1%'; rejecting this build output") % path); + /* Check that the output is not group or world writable, + as that means that someone else can have interfered + with the build. Also, the output should be owned by + the build user. */ + if ((st.st_mode & (S_IWGRP | S_IWOTH)) || + (buildUser.enabled() && st.st_uid != buildUser.getUID())) + throw BuildError(format("suspicious ownership or permission on `%1%'; rejecting this build output") % path); #endif - /* Gain ownership of the build result using the setuid wrapper - if we're not root. If we *are* root, then - canonicalisePathMetaData() will take care of this later - on. */ - if (buildUser.enabled() && !amPrivileged()) - getOwnership(path); - } + /* Gain ownership of the build result using the setuid + wrapper if we're not root. If we *are* root, then + canonicalisePathMetaData() will take care of this later + on. */ + if (buildUser.enabled() && !amPrivileged()) + getOwnership(path); + } - /* Check the exit status. */ - if (!statusOk(status)) { - deleteTmpDir(false); - printMsg(lvlError, format("builder for `%1%' %2%") - % drvPath % statusToString(status)); - amDone(false); - return; - } + /* Check the exit status. */ + if (!statusOk(status)) { + deleteTmpDir(false); + throw BuildError(format("builder for `%1%' %2%") + % drvPath % statusToString(status)); + } - deleteTmpDir(true); + deleteTmpDir(true); - /* Compute the FS closure of the outputs and register them as - being valid. */ - try { + /* Compute the FS closure of the outputs and register them as + being valid. */ computeClosure(); + } catch (BuildError & e) { printMsg(lvlError, e.msg()); - amDone(false); + amDone(ecFailed); return; } /* Release the build user, if applicable. */ buildUser.release(); - amDone(); + amDone(ecSuccess); } @@ -1184,6 +1191,8 @@ void DerivationGoal::terminateBuildHook() debug("terminating build hook"); pid_t savedPid = pid; pid.wait(true); + /* `false' means don't wake up waiting goals, since we want to + keep this build slot ourselves (at least if the hook reply XXX. */ worker.childTerminated(savedPid, false); fromHook.readSide.close(); toHook.writeSide.close(); @@ -1218,7 +1227,7 @@ bool DerivationGoal::prepareBuild() if (validPaths.size() > 0) { /* !!! fix this; try to delete valid paths */ - throw Error( + throw BuildError( format("derivation `%1%' is blocked by its output paths") % drvPath); } @@ -1250,7 +1259,7 @@ bool DerivationGoal::prepareBuild() if (inDrv.outputs.find(*j) != inDrv.outputs.end()) computeFSClosure(inDrv.outputs[*j].path, inputPaths); else - throw Error( + throw BuildError( format("derivation `%1%' requires non-existent output `%2%' from input derivation `%3%'") % drvPath % *j % i->first); } @@ -1286,7 +1295,7 @@ void DerivationGoal::startBuilder() { Path path = i->second.path; if (store->isValidPath(path)) - throw Error(format("obstructed build: path `%1%' exists") % path); + throw BuildError(format("obstructed build: path `%1%' exists") % path); if (pathExists(path)) { debug(format("removing unregistered path `%1%'") % path); deletePathWrapped(path); @@ -1368,12 +1377,12 @@ void DerivationGoal::startBuilder() string s = drv.env["exportReferencesGraph"]; Strings ss = tokenizeString(s); if (ss.size() % 2 != 0) - throw Error(format("odd number of tokens in `exportReferencesGraph': `%1%'") % s); + throw BuildError(format("odd number of tokens in `exportReferencesGraph': `%1%'") % s); for (Strings::iterator i = ss.begin(); i != ss.end(); ) { string fileName = *i++; Path storePath = *i++; if (!store->isValidPath(storePath)) - throw Error(format("`exportReferencesGraph' refers to an invalid path `%1%'") + throw BuildError(format("`exportReferencesGraph' refers to an invalid path `%1%'") % storePath); checkStoreName(fileName); /* !!! abuse of this function */ PathSet refs; @@ -1534,7 +1543,7 @@ PathSet parseReferenceSpecifiers(const Derivation & drv, string attr) result.insert(*i); else if (drv.outputs.find(*i) != drv.outputs.end()) result.insert(drv.outputs.find(*i)->second.path); - else throw Error( + else throw BuildError( format("derivation contains an illegal reference specifier `%1%'") % *i); } @@ -1561,7 +1570,7 @@ void DerivationGoal::computeClosure() } struct stat st; - if (lstat(path.c_str(), &st)) + if (lstat(path.c_str(), &st) == -1) throw SysError(format("getting attributes of path `%1%'") % path); startNest(nest, lvlTalkative, @@ -1584,7 +1593,7 @@ void DerivationGoal::computeClosure() /* The output path should be a regular file without execute permission. */ if (!S_ISREG(st.st_mode) || (st.st_mode & S_IXUSR) != 0) - throw Error( + throw BuildError( format("output path `%1% should be a non-executable regular file") % path); } @@ -1592,11 +1601,11 @@ void DerivationGoal::computeClosure() /* Check the hash. */ HashType ht = parseHashType(algo); if (ht == htUnknown) - throw Error(format("unknown hash algorithm `%1%'") % algo); + throw BuildError(format("unknown hash algorithm `%1%'") % algo); Hash h = parseHash(ht, i->second.hash); Hash h2 = recursive ? hashPath(ht, path) : hashFile(ht, path); if (h != h2) - throw Error( + throw BuildError( format("output path `%1%' should have %2% hash `%3%', instead has `%4%'") % path % algo % printHash(h) % printHash(h2)); } @@ -1630,7 +1639,7 @@ void DerivationGoal::computeClosure() PathSet allowed = parseReferenceSpecifiers(drv, drv.env["allowedReferences"]); for (PathSet::iterator i = references.begin(); i != references.end(); ++i) if (allowed.find(*i) == allowed.end()) - throw Error(format("output is not allowed to refer to path `%1%'") % *i); + throw BuildError(format("output is not allowed to refer to path `%1%'") % *i); } /* Hash the contents of the path. The hash is stored in the @@ -1849,7 +1858,7 @@ void SubstitutionGoal::init() /* If the path already exists we're done. */ if (store->isValidPath(storePath)) { - amDone(); + amDone(ecSuccess); return; } @@ -1880,8 +1889,12 @@ void SubstitutionGoal::referencesValid() { trace("all referenced realised"); - if (nrFailed > 0) - throw Error(format("some references of path `%1%' could not be realised") % storePath); + if (nrFailed > 0) { + printMsg(lvlError, + format("some references of path `%1%' could not be realised") % storePath); + amDone(ecFailed); + return; + } for (PathSet::iterator i = references.begin(); i != references.end(); ++i) @@ -1902,7 +1915,7 @@ void SubstitutionGoal::tryNext() printMsg(lvlError, format("path `%1%' is required, but it has no (remaining) substitutes") % storePath); - amDone(false); + amDone(ecFailed); return; } sub = subs.front(); @@ -1933,7 +1946,7 @@ void SubstitutionGoal::tryToRun() if (store->isValidPath(storePath)) { debug(format("store path `%1%' has become valid") % storePath); outputLock->setDeletion(true); - amDone(); + amDone(ecSuccess); return; } @@ -2046,7 +2059,7 @@ void SubstitutionGoal::finished() printMsg(lvlChatty, format("substitution of path `%1%' succeeded") % storePath); - amDone(); + amDone(ecSuccess); } @@ -2197,24 +2210,30 @@ void Worker::childTerminated(pid_t pid, bool wakeSleepers) } wantingToBuild.clear(); - } } -void Worker::waitForBuildSlot(GoalPtr goal, bool reallyWait) +void Worker::waitForBuildSlot(GoalPtr goal) { debug("wait for build slot"); - if (reallyWait && children.size() == 0) - throw Error("waiting for a build slot, yet there are no children - " - "maybe the build hook gave an inappropriate `postpone' reply?"); - if (!reallyWait && canBuildMore()) + if (canBuildMore()) wakeUp(goal); /* we can do it right away */ else wantingToBuild.insert(goal); } +void Worker::waitForChildTermination(GoalPtr goal) +{ + debug("wait for child termination"); + if (children.size() == 0) + throw Error("waiting for a build slot, yet there are no running children - " + "maybe the build hook gave an inappropriate `postpone' reply?"); + wantingToBuild.insert(goal); +} + + void Worker::run(const Goals & _topGoals) { for (Goals::iterator i = _topGoals.begin(); @@ -2342,8 +2361,12 @@ void Worker::waitForInput() if (maxSilentTime != 0 && now - i->second.lastOutput >= (time_t) maxSilentTime) - throw Error(format("%1% timed out after %2% seconds of silence") + { + printMsg(lvlError, + format("%1% timed out after %2% seconds of silence") % goal->getName() % maxSilentTime); + goal->cancel(); + } } }