* Acquire the locks on the output paths before trying to run the build

hook.  This fixes a problem with log files being partially or
  completely filled with 0's because another nix-store process
  truncates the log file.  It should also be more efficient.

src/libstore/build.cc

@@ -108,7 +108,6 @@ protected:
     {
         nrFailed = 0;
         exitCode = ecBusy;
-	forceInputs = false;
     }
 
     virtual ~Goal()
@@ -116,8 +115,6 @@ protected:
         trace("goal destroyed");
     }
 
-    bool forceInputs;
-
 public:
     virtual void work() = 0;
 
@@ -152,11 +149,6 @@ public:
        (important!), etc. */
     virtual void cancel() = 0;
 
-    void setForceInputs(bool x)
-    {
-       forceInputs = x;
-    }
-
 protected:
     void amDone(ExitCode result);
 };
@@ -264,7 +256,7 @@ public:
 
 
 MakeError(SubstError, Error)
-MakeError(BuildError, Error)
+MakeError(BuildError, Error) /* denoted a permanent build failure */
 
 
 //////////////////////////////////////////////////////////////////////
@@ -496,7 +488,7 @@ void UserLock::acquire()
         }
     }
 
-    throw BuildError(format("all build users are currently in use; "
+    throw Error(format("all build users are currently in use; "
         "consider creating additional users and adding them to the `%1%' group")
         % buildUsersGroup);
 }
@@ -689,7 +681,7 @@ private:
     void buildDone();
 
     /* Is the build hook willing to perform the build? */
-    typedef enum {rpAccept, rpDecline, rpPostpone, rpDone, rpRestart} HookReply;
+    typedef enum {rpAccept, rpDecline, rpPostpone} HookReply;
     HookReply tryBuildHook();
 
     /* Synchronously wait for a build hook to finish. */
@@ -838,10 +830,8 @@ void DerivationGoal::haveDerivation()
 
     /* If they are all valid, then we're done. */
     if (invalidOutputs.size() == 0) {
-        if(!forceInputs) {
+        amDone(ecSuccess);
-	    amDone(ecSuccess);
+        return;
-	    return;
-	}
     }
 
     /* If this is a fixed-output derivation, it is possible that some
@@ -883,55 +873,21 @@ void DerivationGoal::outputsSubstituted()
     nrFailed = 0;
 
     if (checkPathValidity(false).size() == 0) {
-        if (! forceInputs){
+        amDone(ecSuccess);
-		amDone(ecSuccess);
+        return;
-		return;
-	}
     }
 
     /* Otherwise, at least one of the output paths could not be
        produced using a substitute.  So we have to build instead. */
 
     /* The inputs must be built before we can build this goal. */
-    /* !!! but if possible, only install the paths that we need */
+    foreach (DerivationInputs::iterator, i, drv.inputDrvs)
-    for (DerivationInputs::iterator i = drv.inputDrvs.begin();
+        addWaitee(worker.makeDerivationGoal(i->first));
-         i != drv.inputDrvs.end(); ++i){
-	GoalPtr newGoal = worker.makeDerivationGoal(i->first);
-	newGoal->setForceInputs(forceInputs);
-        addWaitee(newGoal);
-    }
 
     for (PathSet::iterator i = drv.inputSrcs.begin();
          i != drv.inputSrcs.end(); ++i)
         addWaitee(worker.makeSubstitutionGoal(*i));
 
-    /* Actually, I do some work twice just to be on the safe side */
-    string s = drv.env["exportBuildReferencesGraph"]; 
-    Strings ss = tokenizeString(s);
-    if (ss.size() % 2 !=0)
-	throw BuildError(format("odd number of tokens in `exportBuildReferencesGraph': `%1%'") % s); 
-    for (Strings::iterator i = ss.begin(); i != ss.end(); ) {
-	string fileName = *i++;
-	Path storePath=*i++;
-	
-        if (!isInStore(storePath))
-            throw BuildError(format("`exportBuildReferencesGraph' contains a non-store path `%1%'")
-                % storePath);
-        storePath = toStorePath(storePath);
-        if (!worker.store.isValidPath(storePath))
-            throw BuildError(format("`exportBuildReferencesGraph' contains an invalid path `%1%'")
-                % storePath);
-        
-        /* Build-time closure should be in dependencies 
-	 * We really want just derivation, its closure
-	 * and outputs. Looks like we should build it.
-	 * */
-
-	GoalPtr newGoal = worker.makeDerivationGoal(storePath);
-	newGoal->setForceInputs(true);
-        addWaitee(newGoal);
-    }
-
     state = &DerivationGoal::inputsRealised;
 }
 
@@ -943,18 +899,49 @@ void DerivationGoal::inputsRealised()
     if (nrFailed != 0) {
         printMsg(lvlError,
             format("cannot build derivation `%1%': "
-                "%2% inputs could not be realised")
+                "%2% dependencies couldn't be built")
             % drvPath % nrFailed);
         amDone(ecFailed);
         return;
     }
 
-    /* Maybe we just wanted to force build of inputs */
+    /* Gather information necessary for computing the closure and/or
-    if (checkPathValidity(false).size() == 0) {
+       running the build hook. */
-        amDone(ecSuccess);
+    
-        return;
+    /* The outputs are referenceable paths. */
+    foreach (DerivationOutputs::iterator, i, drv.outputs) {
+        debug(format("building path `%1%'") % i->second.path);
+        allPaths.insert(i->second.path);
     }
 
+    /* Determine the full set of input paths. */
+
+    /* First, the input derivations. */
+    foreach (DerivationInputs::iterator, i, drv.inputDrvs) {
+        /* Add the relevant output closures of the input derivation
+           `*i' as input paths.  Only add the closures of output paths
+           that are specified as inputs. */
+        assert(worker.store.isValidPath(i->first));
+        Derivation inDrv = derivationFromPath(i->first);
+        foreach (StringSet::iterator, j, i->second)
+            if (inDrv.outputs.find(*j) != inDrv.outputs.end())
+                computeFSClosure(inDrv.outputs[*j].path, inputPaths);
+            else
+                throw Error(
+                    format("derivation `%1%' requires non-existent output `%2%' from input derivation `%3%'")
+                    % drvPath % *j % i->first);
+    }
+
+    /* Second, the input sources. */
+    foreach (PathSet::iterator, i, drv.inputSrcs)
+        computeFSClosure(*i, inputPaths);
+        /* !!! if `*i' is a derivation, then add the closure of every
+           output in the dependency graph to `inputPaths'. */
+
+    debug(format("added input paths %1%") % showPaths(inputPaths));
+
+    allPaths.insert(inputPaths.begin(), inputPaths.end());
+
     /* Okay, try to build.  Note that here we don't wait for a build
        slot to become available, since we don't need one if there is a
        build hook. */
@@ -967,6 +954,20 @@ void DerivationGoal::tryToBuild()
 {
     trace("trying to build");
 
+    /* Acquire locks on the output paths.  After acquiring the locks,
+       it might turn out that somebody else has performed the build
+       already, and we're done.  If not, we can proceed. */
+    switch (prepareBuild()) {
+        case prDone:
+            amDone(ecSuccess);
+            return;
+        case prRestart:
+            worker.waitForAnyGoal(shared_from_this());
+            return;
+        case prProceed:
+            break;
+    }
+        
     try {
 
         /* Is the build hook willing to accept this job? */
@@ -980,38 +981,19 @@ void DerivationGoal::tryToBuild()
             case rpPostpone:
                 /* Not now; wait until at least one child finishes. */
                 worker.waitForChildTermination(shared_from_this());
+                outputLocks.unlock();
                 return;
             case rpDecline:
                 /* We should do it ourselves. */
                 break;
-            case rpDone:
-                /* Somebody else did it. */
-                amDone(ecSuccess);
-                return;
-            case rpRestart:
-                /* Somebody else is building this output path.
-                   Restart from haveDerivation(). */
-                state = &DerivationGoal::haveDerivation;
-                worker.waitForAnyGoal(shared_from_this());
-                return;
         }
 
+        usingBuildHook = false;
+
         /* Make sure that we are allowed to start a build. */
         if (!worker.canBuildMore()) {
             worker.waitForBuildSlot(shared_from_this());
-            return;
+            outputLocks.unlock();
-        }
-
-        /* Acquire locks and such.  If we then see that the build has
-           been done by somebody else, we're done. */
-        usingBuildHook = false;
-        PrepareBuildReply preply = prepareBuild();
-        if (preply == prDone) {
-            amDone(ecSuccess);
-            return;
-        } else if (preply == prRestart) {
-            state = &DerivationGoal::haveDerivation;
-            worker.waitForAnyGoal(shared_from_this());
             return;
         }
 
@@ -1022,14 +1004,13 @@ void DerivationGoal::tryToBuild()
         printMsg(lvlError, e.msg());
         outputLocks.unlock();
         buildUser.release();
-        if (printBuildTrace) {
+        if (printBuildTrace)
             if (usingBuildHook)
                 printMsg(lvlError, format("@ hook-failed %1% %2% %3% %4%")
                     % drvPath % drv.outputs["out"].path % 0 % e.msg());
             else
                 printMsg(lvlError, format("@ build-failed %1% %2% %3% %4%")
                     % drvPath % drv.outputs["out"].path % 0 % e.msg());
-        }
         amDone(ecFailed);
         return;
     }
@@ -1069,8 +1050,7 @@ void DerivationGoal::buildDone()
        malicious user from leaving behind a process that keeps files
        open and modifies them after they have been chown'ed to
        root. */
-    if (buildUser.enabled())
+    if (buildUser.enabled()) buildUser.kill();
-        buildUser.kill();
 
     try {
 
@@ -1140,7 +1120,7 @@ void DerivationGoal::buildDone()
                     % drvPath % drv.outputs["out"].path % status % e.msg());
             else
                 printMsg(lvlError, format("@ build-failed %1% %2% %3% %4%")
-                    % drvPath % drv.outputs["out"].path % status % e.msg());
+                    % drvPath % drv.outputs["out"].path % 1 % e.msg());
         }
         amDone(ecFailed);
         return;
@@ -1296,16 +1276,6 @@ DerivationGoal::HookReply DerivationGoal::tryBuildHook()
 
     else if (reply == "accept") {
 
-        /* Acquire locks and such.  If we then see that the output
-           paths are now valid, we're done. */
-        PrepareBuildReply preply = prepareBuild();
-        if (preply == prDone || preply == prRestart) {
-            /* Tell the hook to exit. */
-            writeLine(toHook.writeSide, "cancel");
-            terminateBuildHook();
-            return preply == prDone ? rpDone : rpRestart;
-        }
-
         printMsg(lvlInfo, format("running hook to build path(s) %1%")
             % showPaths(outputPaths(drv.outputs)));
         
@@ -1390,8 +1360,7 @@ DerivationGoal::PrepareBuildReply DerivationGoal::prepareBuild()
        has locked the output since we checked in haveDerivation().
        (It can't happen between here and the lockPaths() call below
        because we're not allowing multi-threading.) */
-    for (DerivationOutputs::iterator i = drv.outputs.begin(); 
+    foreach (DerivationOutputs::iterator, i, drv.outputs)
-         i != drv.outputs.end(); ++i)
         if (pathIsLockedByMe(i->second.path)) {
             debug(format("restarting derivation `%1%' because `%2%' is locked by another goal")
                 % drvPath % i->second.path);
@@ -1425,66 +1394,23 @@ DerivationGoal::PrepareBuildReply DerivationGoal::prepareBuild()
 
     if (validPaths.size() > 0) {
         /* !!! fix this; try to delete valid paths */
-        throw BuildError(
+        throw Error(
             format("derivation `%1%' is blocked by its output paths")
             % drvPath);
     }
 
     /* If any of the outputs already exist but are not registered,
        delete them. */
-    for (DerivationOutputs::iterator i = drv.outputs.begin(); 
+    foreach (DerivationOutputs::iterator, i, drv.outputs) {
-         i != drv.outputs.end(); ++i)
-    {
         Path path = i->second.path;
         if (worker.store.isValidPath(path))
-            throw BuildError(format("obstructed build: path `%1%' exists") % path);
+            throw Error(format("obstructed build: path `%1%' exists") % path);
         if (pathExists(path)) {
             debug(format("removing unregistered path `%1%'") % path);
             deletePathWrapped(path);
         }
     }
 
-    /* Gather information necessary for computing the closure and/or
-       running the build hook. */
-    
-    /* The outputs are referenceable paths. */
-    for (DerivationOutputs::iterator i = drv.outputs.begin();
-         i != drv.outputs.end(); ++i)
-    {
-        debug(format("building path `%1%'") % i->second.path);
-        allPaths.insert(i->second.path);
-    }
-
-    /* Determine the full set of input paths. */
-
-    /* First, the input derivations. */
-    for (DerivationInputs::iterator i = drv.inputDrvs.begin();
-         i != drv.inputDrvs.end(); ++i)
-    {
-        /* Add the relevant output closures of the input derivation
-           `*i' as input paths.  Only add the closures of output paths
-           that are specified as inputs. */
-        assert(worker.store.isValidPath(i->first));
-        Derivation inDrv = derivationFromPath(i->first);
-        for (StringSet::iterator j = i->second.begin();
-             j != i->second.end(); ++j)
-            if (inDrv.outputs.find(*j) != inDrv.outputs.end())
-                computeFSClosure(inDrv.outputs[*j].path, inputPaths);
-            else
-                throw BuildError(
-                    format("derivation `%1%' requires non-existent output `%2%' from input derivation `%3%'")
-                    % drvPath % *j % i->first);
-    }
-
-    /* Second, the input sources. */
-    for (PathSet::iterator i = drv.inputSrcs.begin();
-         i != drv.inputSrcs.end(); ++i)
-        computeFSClosure(*i, inputPaths);
-
-    debug(format("added input paths %1%") % showPaths(inputPaths));
-
-    allPaths.insert(inputPaths.begin(), inputPaths.end());
-
     return prProceed;
 }
 
@@ -1508,7 +1434,7 @@ void DerivationGoal::startBuilder()
         && !(drv.platform == "i686-linux" && thisSystem == "x86_64-linux")
 #endif
         )
-        throw BuildError(
+        throw Error(
             format("a `%1%' is required to build `%3%', but I am a `%2%'")
             % drv.platform % thisSystem % drvPath);
 

tests/dependencies.sh

@@ -1,5 +1,7 @@
 source common.sh
 
+clearStore
+
 drvPath=$($nixinstantiate dependencies.nix)
 
 echo "derivation is $drvPath"
@@ -13,7 +15,7 @@ if test -n "$dot"; then
     $dot < $TEST_ROOT/graph
 fi
 
-outPath=$($nixstore -rvv "$drvPath")
+outPath=$($nixstore -rvv "$drvPath") || fail "build failed"
 
 # Test Graphviz graph generation.
 $nixstore -q --graph "$outPath" > $TEST_ROOT/graph

tests/gc-concurrent.sh

@@ -1,6 +1,6 @@
 source common.sh
 
-$NIX_BIN_DIR/nix-collect-garbage -vvvvv
+$NIX_BIN_DIR/nix-collect-garbage
 
 drvPath1=$($nixinstantiate gc-concurrent.nix -A test1)
 outPath1=$($nixstore -q $drvPath1)
@@ -28,7 +28,7 @@ pid2=$!
 
 # Run the garbage collector while the build is running.
 sleep 4
-$NIX_BIN_DIR/nix-collect-garbage -vvvvv
+$NIX_BIN_DIR/nix-collect-garbage
 
 # Wait for build #1/#2 to finish.
 echo waiting for pid $pid1 to finish...
@@ -53,6 +53,6 @@ rm -f "$NIX_STATE_DIR"/gcroots/foo*
 ! test -e $outPath3.lock
 
 # If we run the collector now, it should delete outPath1/2.
-$NIX_BIN_DIR/nix-collect-garbage -vvvvv
+$NIX_BIN_DIR/nix-collect-garbage
 ! test -e $outPath1
 ! test -e $outPath2

tests/parallel.sh

@@ -2,7 +2,7 @@ source common.sh
 
 clearStore
 
-outPath=$($nixbuild -vv -j10000 parallel.nix)
+outPath=$($nixbuild -j10000 parallel.nix)
 
 echo "output path is $outPath"