daemon: Don't delete files from the store with guix build -K --rounds=3 --check.

Specifying all three of -K, --check, --rounds=3 caused valid packages to be
deleted from the store.

When `buildMode == bmCheck`, the built package is not renamed from
`actualPath` to `i.second.path.  Then later, when `settings.keepFailed` and
`curRound < nrRounds`, `i.second.path` was renamed to `i.second.path + "-check"`
anyway.

* nix/libstore/build.cc (DerivationGoal::registerOutputs): Add check for bmCheck
before renaming store path.
* tests/store.scm ("build-things, check mode"): Ensure file is a valid path.
("build-things, check mode + keep-failed"): New test.

Fixes: #9631.
Change-Id: I31d24b2349f563867b8101a1b5ce3c10896ce165
Co-authored-by: Ludovic Courtès <ludo@gnu.org>
Signed-off-by: Ludovic Courtès <ludo@gnu.org>
Merges: #9632

2 files changed, 34 insertions, 2 deletions

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index 85be95d557f..b94bca5359a 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -3370,7 +3370,10 @@ void DerivationGoal::registerOutputs()
         assert(false); // shouldn't happen
     }
 
-    if (settings.keepFailed) {
+    /* Rename the built package when doing multiple rounds, but
+       don't move the package when running in check mode, since
+       that would remove the valid package from the store. */
+    if (settings.keepFailed && buildMode != bmCheck) {
         for (auto & i : drv.outputs) {
             Path prev = i.second.path + checkSuffix;
             if (pathExists(prev)) deletePath(prev);
diff --git a/tests/store.scm b/tests/store.scm
index a3e67ac7ea4..87773bf0c36 100644
--- a/tests/store.scm
+++ b/tests/store.scm
@@ -1702,7 +1702,8 @@ System: x86_64-linux~%"
            (begin
              (guard (c ((store-protocol-error? c)
                         (pk 'determinism-exception c)
-                        (and (not (zero? (store-protocol-error-status c)))
+                        (and (valid-path? store file) ;must still be valid
+                             (not (zero? (store-protocol-error-status c)))
                              (string-contains (store-protocol-error-message c)
                                               "deterministic"))))
                ;; This one will produce a different result.  Since we're in
@@ -1711,6 +1712,34 @@ System: x86_64-linux~%"
                              (build-mode check))
                #f))))))
 
+(test-equal "build-things, check mode + keep-failed"
+  '(#t #t)
+  (with-store store
+    (let* ((drv  (build-expression->derivation
+                  store "deterministic-thing-to-check"
+                  `(let ((out (assoc-ref %outputs "out")))
+                     (call-with-output-file out
+                       (lambda (port)
+                         (display ,(let ((now (gettimeofday)))
+                                     (+ (car now) (cdr now)))
+                                  port))))
+                  #:guile-for-build
+                  (package-derivation
+                   store %bootstrap-guile (%current-system))))
+           (file (derivation->output-path drv)))
+      (and (build-things store (list (derivation-file-name drv)))
+           (valid-path? store file)
+           (begin
+             ;; This would trigger accidental deletion of FILE.
+             (set-build-options store
+                                #:keep-failed? #t
+                                #:keep-going? #t
+                                #:rounds 3)
+             (build-things store (list (derivation-file-name drv))
+                           (build-mode check))
+             (list (valid-path? store file)
+                   (file-exists? file)))))))
+
 (test-assert "build-succeeded trace in check mode"
   (string-contains
    (call-with-output-string