Merge "libstore: report all differing outputs rather than just the first" into main

doc/manual/change-authors.yml

@@ -85,6 +85,10 @@ kloenk:
   forgejo: kloenk
   github: kloenk
 
+lheckemann:
+  forgejo: lheckemann
+  github: lheckemann
+
 lovesegfault:
   github: lovesegfault
 

doc/manual/rl-next/report-differing-outputs.md

@@ -0,0 +1,22 @@
+---
+synopsis: "Reproducibility check builds now report all differing outputs"
+cls: [2069]
+category: Improvements
+credits: [lheckemann]
+---
+
+`nix-build --check` allows rerunning the build of an already-built derivation to check that it produces the same output again.
+
+If a multiple-output derivation with impure behaviour is built with `--check`, only the first output would be shown in the resulting error message (and kept for comparison):
+
+```
+error: derivation '/nix/store/4spy3nz1661zm15gkybsy1h5f36aliwx-python3.11-test-1.0.0.drv' may not be deterministic: output '/nix/store/ccqcp01zg18wp9iadzmzimqzdi3ll08d-python3.11-test
+-1.0.0-dist' differs from '/nix/store/ccqcp01zg18wp9iadzmzimqzdi3ll08d-python3.11-test-1.0.0-dist.check'
+```
+
+Now, all differing outputs are kept and reported:
+```
+error: derivation '4spy3nz1661zm15gkybsy1h5f36aliwx-python3.11-test-1.0.0.drv' may not be deterministic: outputs differ
+         output differs: output '/nix/store/ccqcp01zg18wp9iadzmzimqzdi3ll08d-python3.11-test-1.0.0-dist' differs from '/nix/store/ccqcp01zg18wp9iadzmzimqzdi3ll08d-python3.11-test-1.0.0-dist.check'
+         output differs: output '/nix/store/yl59v08356i841c560alb0zmk7q16klb-python3.11-test-1.0.0' differs from '/nix/store/yl59v08356i841c560alb0zmk7q16klb-python3.11-test-1.0.0.check'
+```

src/libstore/build/local-derivation-goal.cc

@@ -2021,6 +2021,8 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
 
     OutputPathMap finalOutputs;
 
+    std::vector<std::pair<Path, std::optional<Path>>> nondeterministic;
+
     for (auto & outputName : sortedOutputNames) {
         auto output = get(drv->outputs, outputName);
         auto scratchPath = get(scratchOutputs, outputName);
@@ -2307,20 +2309,20 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
                         buildUser ? buildUser->getGID() : getgid(),
                         finalDestPath, dst, worker.store.printStorePath(drvPath), tmpDir);
 
-                    throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs from '%s'",
+                    nondeterministic.push_back(std::make_pair(worker.store.toRealPath(finalDestPath), dst));
-                        worker.store.printStorePath(drvPath), worker.store.toRealPath(finalDestPath), dst);
                 } else
-                    throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs",
+                    nondeterministic.push_back(std::make_pair(worker.store.toRealPath(finalDestPath), std::nullopt));
-                        worker.store.printStorePath(drvPath), worker.store.toRealPath(finalDestPath));
             }
 
             /* Since we verified the build, it's now ultimately trusted. */
-            if (!oldInfo.ultimate) {
+            else if (!oldInfo.ultimate) {
                 oldInfo.ultimate = true;
                 localStore.signPathInfo(oldInfo);
                 localStore.registerValidPaths({{oldInfo.path, oldInfo}});
             }
 
+            /* Don't register anything, since we already have the
+               previous versions which we're comparing. */
             continue;
         }
 
@@ -2351,6 +2353,18 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
     }
 
     if (buildMode == bmCheck) {
+        if (!nondeterministic.empty()) {
+            std::ostringstream msg;
+            msg << HintFmt("derivation '%s' may not be deterministic: outputs differ", drvPath.to_string());
+            for (auto [oldPath, newPath]: nondeterministic) {
+                if (newPath) {
+                    msg << HintFmt("\n  output differs: output '%s' differs from '%s'", oldPath.c_str(), *newPath);
+                } else {
+                    msg << HintFmt("\n  output '%s' differs", oldPath.c_str());
+                }
+            }
+            throw NotDeterministic(msg.str());
+        }
         /* In case of fixed-output derivations, if there are
            mismatches on `--check` an error must be thrown as this is
            also a source for non-determinism. */

tests/functional/check.nix

@@ -6,10 +6,12 @@ with import ./config.nix;
   nondeterministic = mkDerivation {
     inherit checkBuildId;
     name = "nondeterministic";
+    outputs = ["a" "b"];
     buildCommand =
       ''
-        mkdir $out
+        mkdir $a $b
-        date +%s.%N > $out/date
+        date +%s.%N > $a/date
+        date +%s.%N > $b/date
         echo "CHECK_TMPDIR=$TMPDIR"
         echo "checkBuildId=$checkBuildId"
         echo "$checkBuildId" > $TMPDIR/checkBuildId
@@ -19,10 +21,12 @@ with import ./config.nix;
   deterministic = mkDerivation {
     inherit checkBuildId;
     name = "deterministic";
+    outputs = ["a" "b"];
     buildCommand =
       ''
-        mkdir $out
+        mkdir $a $b
-        echo date > $out/date
+        echo date > $a/date
+        echo date > $b/date
         echo "CHECK_TMPDIR=$TMPDIR"
         echo "checkBuildId=$checkBuildId"
         echo "$checkBuildId" > $TMPDIR/checkBuildId
@@ -32,10 +36,12 @@ with import ./config.nix;
   failed = mkDerivation {
     inherit checkBuildId;
     name = "failed";
+    outputs = ["a" "b"];
     buildCommand =
       ''
-        mkdir $out
+        mkdir $a $b
-        echo date > $out/date
+        echo date > $a/date
+        echo date > $b/date
         echo "CHECK_TMPDIR=$TMPDIR"
         echo "checkBuildId=$checkBuildId"
         echo "$checkBuildId" > $TMPDIR/checkBuildId

tests/functional/check.sh

@@ -65,6 +65,8 @@ checkBuildTempDirRemoved $TEST_ROOT/log
 nix-build check.nix -A nondeterministic --argstr checkBuildId $checkBuildId \
     --no-out-link --check 2> $TEST_ROOT/log || status=$?
 grep 'may not be deterministic' $TEST_ROOT/log
+# the differences in both outputs should be reported
+[[ $(grep -c 'differs' $TEST_ROOT/log) = 2 ]]
 [ "$status" = "104" ]
 checkBuildTempDirRemoved $TEST_ROOT/log