01/04: daemon: Allow builds to be repeated.

[Ludovic Courtès]

civodul pushed a commit to branch master
in repository guix.

commit b23b4d394a39b60188ed74ecdf1027bc7dd5b9b3
Author: Eelco Dolstra <address@hidden>
Date:   Tue Dec 8 22:50:18 2015 +0100

    daemon: Allow builds to be repeated.
    
    This makes it easy to detect non-deterministic builds.
    
    * nix/libstore/build.cc (DerivationGoal): Remove 'InodesSeen'; add
    'curRound', 'nrRound', and 'prevInfos'.
    (DerivationGoal::inputsRealised): Initialize 'nrRound'.
    (NotDeterministic): New error type.
    (DerivationGoal::buildDone): Check whether we need to repeat.
    (DerivationGoal::startBuilder): Adjust message.
    (DerivationGoal::registerOutputs): Check whether we get the same result.
    * nix/libstore/globals.cc (Settings::get(const string & name, int def)):
    New method.
    * nix/libstore/globals.hh (Settings): Add it.
    * nix/libstore/store-api.hh (ValidPathInfo): Add operator ==.
    * nix/nix-daemon/nix-daemon.cc (performOp): Allow "build-repeat" for
    "untrusted" users.
    
    Co-authored-by: Ludovic Courtès <address@hidden>
---
 nix/libstore/build.cc        |   61 +++++++++++++++++++++++++++++++++++-------
 nix/libstore/globals.cc      |    7 +++++
 nix/libstore/globals.hh      |    2 +
 nix/libstore/store-api.hh    |   13 +++++++--
 nix/nix-daemon/nix-daemon.cc |    2 +-
 5 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 64678a5..e19c32b 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -785,10 +785,16 @@ private:
        temporary paths. */
     PathSet redirectedBadOutputs;
 
-    /* Set of inodes seen during calls to canonicalisePathMetaData()
-       for this build's outputs.  This needs to be shared between
-       outputs to allow hard links between outputs. */
-    InodesSeen inodesSeen;
+    /* The current round, if we're building multiple times. */
+    unsigned int curRound = 1;
+
+    unsigned int nrRounds;
+
+    /* Path registration info from the previous round, if we're
+       building multiple times. Since this contains the hash, it
+       allows us to compare whether two rounds produced the same
+       result. */
+    ValidPathInfos prevInfos;
 
 public:
     DerivationGoal(const Path & drvPath, const StringSet & wantedOutputs, 
Worker & worker, BuildMode buildMode = bmNormal);
@@ -1194,8 +1200,12 @@ void DerivationGoal::inputsRealised()
 
     /* Is this a fixed-output derivation? */
     fixedOutput = true;
-    foreach (DerivationOutputs::iterator, i, drv.outputs)
-        if (i->second.hash == "") fixedOutput = false;
+    for (auto & i : drv.outputs)
+       if (i.second.hash == "") fixedOutput = false;
+
+    /* Don't repeat fixed-output derivations since they're already
+       verified by their output hash.*/
+    nrRounds = fixedOutput ? 1 : settings.get("build-repeat", 0) + 1;
 
     /* 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
@@ -1371,6 +1381,9 @@ void replaceValidPath(const Path & storePath, const Path 
tmpPath)
 }
 
 
+MakeError(NotDeterministic, BuildError)
+
+
 void DerivationGoal::buildDone()
 {
     trace("build done");
@@ -1470,6 +1483,15 @@ void DerivationGoal::buildDone()
 
         deleteTmpDir(true);
 
+        /* Repeat the build if necessary. */
+        if (curRound++ < nrRounds) {
+            outputLocks.unlock();
+            buildUser.release();
+            state = &DerivationGoal::tryToBuild;
+            worker.wakeUp(shared_from_this());
+            return;
+        }
+
         /* It is now safe to delete the lock files, since all future
            lockers will see that the output paths are valid; they will
            not create new lock files with the same names as the old
@@ -1623,10 +1645,13 @@ int childEntry(void * arg)
 
 void DerivationGoal::startBuilder()
 {
-    startNest(nest, lvlInfo, format(
-            buildMode == bmRepair ? "repairing path(s) %1%" :
-            buildMode == bmCheck ? "checking path(s) %1%" :
-            "building path(s) %1%") % showPaths(missingPaths));
+    auto f = format(
+        buildMode == bmRepair ? "repairing path(s) %1%" :
+        buildMode == bmCheck ? "checking path(s) %1%" :
+        nrRounds > 1 ? "building path(s) %1% (round %2%/%3%)" :
+        "building path(s) %1%");
+    f.exceptions(boost::io::all_error_bits ^ boost::io::too_many_args_bit);
+    startNest(nest, lvlInfo, f % showPaths(missingPaths) % curRound % 
nrRounds);
 
     /* Right platform? */
     if (!canBuildLocally(drv.platform)) {
@@ -1638,6 +1663,7 @@ void DerivationGoal::startBuilder()
     }
 
     /* Construct the environment passed to the builder. */
+    env.clear();
 
     /* Most shells initialise PATH to some default (/bin:/usr/bin:...) when
        PATH is not set.  We don't want this, so we fill it in with some dummy
@@ -2267,6 +2293,11 @@ void DerivationGoal::registerOutputs()
 
     ValidPathInfos infos;
 
+    /* Set of inodes seen during calls to canonicalisePathMetaData()
+       for this build's outputs.  This needs to be shared between
+       outputs to allow hard links between outputs. */
+    InodesSeen inodesSeen;
+
     /* Check whether the output paths were created, and grep each
        output path to determine what other paths it references.  Also make all
        output paths read-only. */
@@ -2438,6 +2469,16 @@ void DerivationGoal::registerOutputs()
 
     if (buildMode == bmCheck) return;
 
+    if (curRound > 1 && prevInfos != infos)
+        throw NotDeterministic(
+            format("result of ‘%1%’ differs from previous round; rejecting as 
non-deterministic")
+            % drvPath);
+
+    if (curRound < nrRounds) {
+        prevInfos = infos;
+        return;
+    }
+
     /* Register each output path as valid, and register the sets of
        paths referenced by each of them.  If there are cycles in the
        outputs, this will fail. */
diff --git a/nix/libstore/globals.cc b/nix/libstore/globals.cc
index 07f23d4..84fc885 100644
--- a/nix/libstore/globals.cc
+++ b/nix/libstore/globals.cc
@@ -137,6 +137,13 @@ bool Settings::get(const string & name, bool def)
     return res;
 }
 
+int Settings::get(const string & name, int def)
+{
+    int res = def;
+    _get(res, name);
+    return res;
+}
+
 
 void Settings::update()
 {
diff --git a/nix/libstore/globals.hh b/nix/libstore/globals.hh
index c17e10d..8c07e36 100644
--- a/nix/libstore/globals.hh
+++ b/nix/libstore/globals.hh
@@ -27,6 +27,8 @@ struct Settings {
 
     bool get(const string & name, bool def);
 
+    int get(const string & name, int def);
+
     void update();
 
     string pack();
diff --git a/nix/libstore/store-api.hh b/nix/libstore/store-api.hh
index 3764f3e..9403cbe 100644
--- a/nix/libstore/store-api.hh
+++ b/nix/libstore/store-api.hh
@@ -88,10 +88,17 @@ struct ValidPathInfo
     Path deriver;
     Hash hash;
     PathSet references;
-    time_t registrationTime;
-    unsigned long long narSize; // 0 = unknown
+    time_t registrationTime = 0;
+    unsigned long long narSize = 0; // 0 = unknown
     unsigned long long id; // internal use only
-    ValidPathInfo() : registrationTime(0), narSize(0) { }
+
+    bool operator == (const ValidPathInfo & i) const
+    {
+        return
+            path == i.path
+            && hash == i.hash
+            && references == i.references;
+    }
 };
 
 typedef list<ValidPathInfo> ValidPathInfos;
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index c1e697b..35c284f 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -565,7 +565,7 @@ static void performOp(bool trusted, unsigned int 
clientVersion,
             for (unsigned int i = 0; i < n; i++) {
                 string name = readString(from);
                 string value = readString(from);
-                if (name == "build-timeout" || name == "use-ssh-substituter")
+                if (name == "build-timeout" || name == "build-repeat" || name 
== "use-ssh-substituter")
                     settings.set(name, value);
                 else
                     settings.set(trusted ? name : "untrusted-" + name, value);