* Simplify deleting .lock files in /nix/store: just don't delete them

  if they belong a path that's currently being built.  This gets rid
  of some Cygwin-specific code.

4 files changed, 38 insertions, 34 deletions

diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc
index 3621b88d9049..32cfbd022c61 100644
--- a/src/libstore/gc.cc
+++ b/src/libstore/gc.cc
@@ -476,23 +476,6 @@ void LocalStore::gcPath(const GCOptions & options, GCResults & results,
 
     if (!pathExists(path)) return;
                 
-#ifndef __CYGWIN__
-    AutoCloseFD fdLock;
-        
-    /* Only delete a lock file if we can acquire a write lock on it.
-       That means that it's either stale, or the process that created
-       it hasn't locked it yet.  In the latter case the other process
-       will detect that we deleted the lock, and retry (see
-       pathlocks.cc). */
-    if (path.size() >= 5 && string(path, path.size() - 5) == ".lock") {
-        fdLock = openLockFile(path, false);
-        if (fdLock != -1 && !lockFile(fdLock, ltWrite, false)) {
-            debug(format("skipping active lock `%1%'") % path);
-            return;
-        }
-    }
-#endif
-
     /* Okay, it's safe to delete. */
     unsigned long long bytesFreed, blocksFreed;
     deleteFromStore(path, bytesFreed, blocksFreed);
@@ -513,12 +496,6 @@ void LocalStore::gcPath(const GCOptions & options, GCResults & results,
             throw GCLimitReached();
         }
     }
-
-#ifndef __CYGWIN__
-    if (fdLock != -1)
-        /* Write token to stale (deleted) lock file. */
-        writeFull(fdLock, (const unsigned char *) "d", 1);
-#endif
 }
 
 
@@ -569,7 +546,7 @@ struct CachingAtimeComparator : public std::binary_function<Path, Path, bool>
 };
 
 
-string showTime(const string & format, time_t t)
+static string showTime(const string & format, time_t t)
 {
     char s[128];
     strftime(s, sizeof s, format.c_str(), localtime(&t));
@@ -577,6 +554,21 @@ string showTime(const string & format, time_t t)
 }
 
 
+static bool isLive(const Path & path, const PathSet & livePaths,
+    const PathSet & tempRoots, const PathSet & tempRootsClosed)
+{
+    if (livePaths.find(path) != livePaths.end() ||
+        tempRootsClosed.find(path) != tempRootsClosed.end()) return true;
+
+    /* A lock file belonging to a path that we're building right
+       now isn't garbage. */
+    if (hasSuffix(path, ".lock") && tempRoots.find(string(path, 0, path.size() - 5)) != tempRoots.end())
+        return true;
+
+    return false;
+}
+
+
 void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
 {
     bool gcKeepOutputs =
@@ -691,9 +683,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
         Paths entries = readDirectory(nixStore);
         foreach (Paths::iterator, i, entries) {
             Path path = canonPath(nixStore + "/" + *i);
-            if (livePaths.find(path) == livePaths.end() &&
-                tempRootsClosed.find(path) == tempRootsClosed.end())
-                storePaths.insert(path);
+            if (!isLive(path, livePaths, tempRoots, tempRootsClosed)) storePaths.insert(path);
         }
     }
 
@@ -701,10 +691,8 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
         foreach (PathSet::iterator, i, options.pathsToDelete) {
             assertStorePath(*i);
             storePaths.insert(*i);
-            if (livePaths.find(*i) != livePaths.end())
+            if (isLive(*i, livePaths, tempRoots, tempRootsClosed))
                 throw Error(format("cannot delete path `%1%' since it is still alive") % *i);
-            if (tempRootsClosed.find(*i) != tempRootsClosed.end())
-                throw Error(format("cannot delete path `%1%' since it is temporarily in use") % *i);
         }
     }
 
diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc
index 065d7d4c014d..f6271d56aafd 100644
--- a/src/nix-store/nix-store.cc
+++ b/src/nix-store/nix-store.cc
@@ -96,7 +96,7 @@ static void opRealise(Strings opFlags, Strings opArgs)
         if (isDerivation(*i)) drvPaths.insert(*i);
     store->buildDerivations(drvPaths);
 
-    foreach (Strings::iterator, i,opArgs)
+    foreach (Strings::iterator, i, opArgs)
         cout << format("%1%\n") % realisePath(*i);
 }
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index c65f68584663..ab6ac426aa4d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -7,7 +7,7 @@ dependencies.sh gc.sh nix-push.sh nix-pull.in logging.sh nix-build.sh install-pa
 locking.sh: locking.nix
 parallel.sh: parallel.nix
 build-hook.sh: build-hook.nix
-gc-concurrent.sh: gc-concurrent.nix gc-concurrent2.nix
+gc-concurrent.sh: gc-concurrent.nix gc-concurrent2.nix simple.nix
 user-envs.sh: user-envs.nix
 fixed.sh: fixed.nix
 gc-runtime.sh: gc-runtime.nix
diff --git a/tests/gc-concurrent.sh b/tests/gc-concurrent.sh
index 6cf65207887a..fbc9497206c1 100644
--- a/tests/gc-concurrent.sh
+++ b/tests/gc-concurrent.sh
@@ -8,8 +8,15 @@ outPath1=$($nixstore -q $drvPath1)
 drvPath2=$($nixinstantiate gc-concurrent2.nix)
 outPath2=$($nixstore -q $drvPath2)
 
-rm -f "$NIX_STATE_DIR"/gcroots/foo
+drvPath3=$($nixinstantiate simple.nix)
+outPath3=$($nixstore -r $drvPath3)
+
+! test -e $outPath3.lock
+touch $outPath3.lock
+
+rm -f "$NIX_STATE_DIR"/gcroots/foo*
 ln -s $drvPath2 "$NIX_STATE_DIR"/gcroots/foo
+ln -s $outPath3 "$NIX_STATE_DIR"/gcroots/foo2
 
 # Start build #1 in the background.  It starts immediately.
 $nixstore -rvv "$drvPath1" &
@@ -39,4 +46,13 @@ cat $outPath1/input-2/bar
 # derivation is a GC root.
 cat $outPath2/foobar
 
-rm "$NIX_STATE_DIR"/gcroots/foo
+rm -f "$NIX_STATE_DIR"/gcroots/foo*
+
+# The collector should have deleted lock files for paths that have
+# been built previously.
+! test -e $outPath3.lock
+
+# If we run the collector now, it should delete outPath1/2.
+$NIX_BIN_DIR/nix-collect-garbage -vvvvv
+! test -e $outPath1
+! test -e $outPath2