Merge pull request #2688 from tollb/fix/build_check_keep_failed_sandbox_perms

Fix nix-build --check -K in sandbox w/o root

src/libstore/build.cc

@@ -3537,6 +3537,29 @@ StorePathSet parseReferenceSpecifiers(Store & store, const BasicDerivation & drv
 }
 
 
+static void moveCheckToStore(const Path & src, const Path & dst)
+{
+    /* For the rename of directory to succeed, we must be running as root or
+       the directory must be made temporarily writable (to update the
+       directory's parent link ".."). */
+    struct stat st;
+    if (lstat(src.c_str(), &st) == -1) {
+        throw SysError(format("getting attributes of path '%1%'") % src);
+    }
+
+    bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR));
+
+    if (changePerm)
+        chmod_(src, st.st_mode | S_IWUSR);
+
+    if (rename(src.c_str(), dst.c_str()))
+        throw SysError(format("renaming '%1%' to '%2%'") % src % dst);
+
+    if (changePerm)
+        chmod_(dst, st.st_mode);
+}
+
+
 void DerivationGoal::registerOutputs()
 {
     /* When using a build hook, the build hook can register the output
@@ -3715,8 +3738,7 @@ void DerivationGoal::registerOutputs()
                 if (settings.runDiffHook || settings.keepFailed) {
                     Path dst = worker.store.toRealPath(path + checkSuffix);
                     deletePath(dst);
-                    if (rename(actualPath.c_str(), dst.c_str()))
+                    moveCheckToStore(actualPath, dst);
-                        throw SysError(format("renaming '%1%' to '%2%'") % actualPath % dst);
 
                     handleDiffHook(
                         buildUser ? buildUser->getUID() : getuid(),
@@ -3724,10 +3746,10 @@ void DerivationGoal::registerOutputs()
                         path, dst, worker.store.printStorePath(drvPath), tmpDir);
 
                     throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs from '%s'",
-                        worker.store.printStorePath(drvPath), path, dst);
+                        worker.store.printStorePath(drvPath), worker.store.toRealPath(path), dst);
                 } else
                     throw NotDeterministic("derivation '%s' may not be deterministic: output '%s' differs",
-                        worker.store.printStorePath(drvPath), path);
+                        worker.store.printStorePath(drvPath), worker.store.toRealPath(path));
             }
 
             /* Since we verified the build, it's now ultimately trusted. */

tests/check.sh

@@ -49,13 +49,8 @@ checkBuildTempDirRemoved $TEST_ROOT/log
 
 nix-build check.nix -A nondeterministic --argstr checkBuildId $checkBuildId \
     --no-out-link --check --keep-failed 2> $TEST_ROOT/log || status=$?
-
-# The above nix-build fails with status=1 on darwin (not sure why)
-# ...but the primary purpose of the test case is to verify the temp directory is retained
-if [ "$(uname -s)" != "Darwin" ]; then
 grep 'may not be deterministic' $TEST_ROOT/log
 [ "$status" = "104" ]
-fi
 if checkBuildTempDirRemoved $TEST_ROOT/log; then false; fi
 
 clearStore

tests/linux-sandbox.sh

@@ -28,3 +28,10 @@ nix cat-store $outPath/foobar | grep FOOBAR
 
 # Test --check without hash rewriting.
 nix-build dependencies.nix --no-out-link --check --sandbox-paths /nix/store
+
+# Test that sandboxed builds with --check and -K can move .check directory to store
+nix-build check.nix -A nondeterministic --sandbox-paths /nix/store --no-out-link
+
+(! nix-build check.nix -A nondeterministic --sandbox-paths /nix/store --no-out-link --check -K 2> $TEST_ROOT/log)
+if grep -q 'error: renaming' $TEST_ROOT/log; then false; fi
+grep -q 'may not be deterministic' $TEST_ROOT/log