Finish refactoring and add tests. Move _CleanOutClientRoot and _CleanOutOthers

to the ClientRootKeeper package in basics.  Replace _RemoveDirectoryTree with
shutil function.  Add tests to see that directories are created and deleted.
Add a couple of title headers to basics.py.  Remove a couple of now irrelevant
comments.

Tests: make pump-maintainer-check
       make include-server-maintainer-check

Reviewer: csilvers@google.com

3 files changed, 86 insertions, 82 deletions

diff --git a/include_server/basics.py b/include_server/basics.py
index a0d7228..8f5d856 100755
--- a/include_server/basics.py
+++ b/include_server/basics.py
@@ -25,11 +25,12 @@ import glob
 import os.path
 import resource
 import signal
+import shutil
 import sys
 import tempfile
 
 
-# TEMPORARY LOCATIONS FOR GENERATIONS OF COMPRESSED FILES
+# MANAGEMENT OF TEMPORARY LOCATIONS FOR GENERATIONS OF COMPRESSED FILES
 
 
 class ClientRootKeeper(object):
@@ -43,6 +44,7 @@ class ClientRootKeeper(object):
   Instance vars:
     client_tmp: a path, the place for creation of temporary directories.
     client_root: a path, the current such temporary directory
+    _client_root_before_padding: a path kept for testing purposes
 
   A typical client root looks like:
 
@@ -92,12 +94,12 @@ class ClientRootKeeper(object):
     try:
       # Create a unique identifier that will never repeat. Use pid as suffix for
       # cleanout mechanism that wipes files not associated with a running pid.
-      client_root_before_padding = tempfile.mkdtemp(
+      self._client_root_before_padding = tempfile.mkdtemp(
           '.%s-%s-%d' %
           (self.INCLUDE_SERVER_NAME,
            os.getpid(), generation),
           dir=self.client_tmp)
-      self.client_root = (client_root_before_padding
+      self.client_root = (self._client_root_before_padding
                           + '/padding' * self.number_missing_levels)
       if not os.path.isdir(self.client_root):
         os.makedirs(self.client_root)
@@ -105,6 +107,49 @@ class ClientRootKeeper(object):
       sys.exit('Could not create client root directory %s: %s' %
                (self.client_root, why))
 
+  def CleanOutClientRoots(self, pid=None):
+    """Delete client root directories pertaining to this process.
+    Args:
+      pid: None (which means 'pid of current process') or an integer
+    """
+    if not pid:
+      pid = os.getpid()
+    for client_root in self.Glob(str(pid)):
+      shutil.rmtree(client_root, ignore_errors=True)
+
+  def CleanOutOthers(self):
+    """Search for left-overs from include servers that have passed away."""
+    # Find all client root subdirectories whether abandoned or not.
+    distcc_directories = self.Glob('*')
+    for directory in distcc_directories:
+      # Fish out pid from end of directory name.
+      hyphen_ultimate_position = directory.rfind('-')
+      assert hyphen_ultimate_position != -1
+      hyphen_penultimate_position = directory.rfind('-', 0,
+                                                    hyphen_ultimate_position)
+      assert hyphen_penultimate_position != -1
+      pid_str = directory[hyphen_penultimate_position + 1:
+                          hyphen_ultimate_position]
+      try:
+        pid = int(pid_str)
+      except ValueError:
+        continue  # Happens only if a spoofer is around.
+      try:
+        # Got a pid; does it still exist?
+        os.getpgid(pid)
+        continue
+      except OSError:
+        # Process pid does not exist. Nuke its associated files. This will
+        # of course only succeed if the files belong the current uid of
+        # this process.
+        if not os.access(directory, os.W_OK):
+          continue  # no access, not ours
+        Debug(DEBUG_TRACE,
+              "Cleaning out '%s' after defunct include server." % directory)
+        self.CleanOutClientRoots(pid)
+
+
+# EMAILS
 
 # For automated emails, see also src/emaillog.h.
 DCC_EMAILLOG_WHOM_TO_BLAME = os.getenv('DISTCC_EMAILLOG_WHOM_TO_BLAME',
@@ -114,6 +159,8 @@ CANT_SEND_MESSAGE = """Please notify %s that the distcc-pump include server
 tried to send them email but failed.""" % DCC_EMAILLOG_WHOM_TO_BLAME
 MAX_EMAILS_TO_SEND = 3
 
+# TIME QUOTAS (SOLVING THE HALTING PROBLEM)
+
 # The maximum user time the include server is allowed handling one request. This
 # is a critical parameter because all caches are reset if this time is
 # exceeded. And if all caches are reset, then the next request may take much
diff --git a/include_server/basics_test.py b/include_server/basics_test.py
index 31fff38..29af2fa 100755
--- a/include_server/basics_test.py
+++ b/include_server/basics_test.py
@@ -72,7 +72,6 @@ class BasicsTest(unittest.TestCase):
       os.environ['DISTCC_CLIENT_TMP'] = '/to'
       client_root_keeper = basics.ClientRootKeeper()
       client_root_keeper.ClientRootMakedir(2)
-      print 'xxxxxxxxxxxx', client_root_keeper.client_root
       self.assertEqual(os.path.dirname(
           os.path.dirname(client_root_keeper.client_root)), "/to")
       self.assertEqual(os.path.basename(client_root_keeper.client_root),
@@ -82,5 +81,37 @@ class BasicsTest(unittest.TestCase):
     finally:
       tempfile.mkdtemp = tempfile_mkdtemp
       os.makedirs = os_makedirs
-    
+
+
+  def test_ClientRootKeeper_Deletions(self):
+    """Test whether directories emerge and go away appropriately."""
+
+    # Test with a one-level value of DISTCC_CLIENT_TMP.
+    os.environ['DISTCC_CLIENT_TMP'] = '/tmp'
+    client_root_keeper = basics.ClientRootKeeper()
+    client_root_keeper.ClientRootMakedir(117)
+    self.assert_(os.path.isdir(client_root_keeper._client_root_before_padding))
+    self.assert_(os.path.isdir(client_root_keeper.client_root))
+    self.assert_(client_root_keeper.client_root.endswith('/padding'))
+    client_root_keeper.ClientRootMakedir(118)
+    client_root_keeper.CleanOutClientRoots()
+    # Directories must be gone now!
+    self.assert_(not os.path.isdir(
+        client_root_keeper._client_root_before_padding))
+    # Test with a two-level value of DISTCC_CLIENT_TMP.
+    try:
+      os.environ['DISTCC_CLIENT_TMP'] = tempfile.mkdtemp('basics_test',
+                                                         dir='/tmp')
+      client_root_keeper = basics.ClientRootKeeper()
+      client_root_keeper.ClientRootMakedir(117)
+      self.assert_(os.path.isdir(
+          client_root_keeper._client_root_before_padding))
+      self.assert_(os.path.isdir(client_root_keeper.client_root))
+      client_root_keeper.ClientRootMakedir(118)
+      client_root_keeper.CleanOutClientRoots()
+      self.assert_(os.path.isdir,
+                   client_root_keeper._client_root_before_padding)
+    finally:
+      os.rmdir(os.environ['DISTCC_CLIENT_TMP'])
+
 unittest.main()    
diff --git a/include_server/include_server.py b/include_server/include_server.py
index b0562f6..23befbe 100755
--- a/include_server/include_server.py
+++ b/include_server/include_server.py
@@ -173,79 +173,6 @@ class _EmailSender(object):
     fd.close()
 
 
-def _RemoveDirectoryTree(tree_top):
-  """Recursively remove everything.
-
-  Ignore filesystem errors, because this function may be called as a last resort
-  and it does its job on a best-effort basis.
-  """
-  # Copied, more or less, from Python 2.4 Library Reference.
-  if not os.access(tree_top, os.W_OK):
-    return
-  for root, dirs, files in os.walk(tree_top, topdown=False):
-    for name in files:
-      try:
-        os.remove(os.path.join(root, name))
-      except (IOError, OSError):  # should not happen
-        pass
-    for name in dirs:
-      try:
-        if os.path.islink(os.path.join(root, name)):
-          os.remove(os.path.join(root, name))
-        else:
-          os.rmdir(os.path.join(root, name))
-      except (IOError, OSError):  # should not happen
-          pass
-    try:
-      os.rmdir(root)
-    except (IOError, OSError):  # should not happen
-      pass
-
-
-def _CleanOutClientRoots(client_root_keeper, pid=None):
-  """Delete client root directories pertaining to this process.
-  Args:
-    client_root_keeper: an object of type ClientRootKeeper
-    pid: None (which means 'pid of current process') or an integer
-  """
-  if not pid:
-    pid = os.getpid()
-  for client_root_ in client_root_keeper.Glob(str(pid)):
-    _RemoveDirectoryTree(client_root_)
-
-
-def _CleanOutOthers(client_root_keeper):
-  """Search for left-overs from include servers that have passed away."""
-  # Find all client root subdirectories whether abandoned or not.
-  distcc_directories = client_root_keeper.Glob('*')
-  for directory in distcc_directories:
-    # Fish out pid from end of directory name.
-    hyphen_ultimate_position = directory.rfind('-')
-    assert hyphen_ultimate_position != -1
-    hyphen_penultimate_position = directory[:hyphen_ultimate_position].rfind(
-        '-')
-    assert hyphen_penultimate_position != -1
-    pid_str = directory[hyphen_penultimate_position + 1:
-                        hyphen_ultimate_position]
-    try:
-      pid = int(pid_str)
-    except ValueError:
-      continue  # Happens only if a spoofer is around.
-    try:
-      # Got a pid; does it still exist?
-      os.getpgid(pid)
-      continue
-    except OSError:
-      # Process pid does not exist. Nuke its associated files. This will
-      # of course only succeed if the files belong the current uid of
-      # this process.
-      if not os.access(directory, os.W_OK):
-        continue  # no access, not ours
-      Debug(DEBUG_TRACE,
-            "Cleaning out '%s' after defunct include server." % directory)
-      _CleanOutClientRoots(client_root_keeper, pid)
-
-
 NEWLINE_RE = re.compile(r"\n", re.MULTILINE)
 BACKSLASH_NEWLINE_RE = re.compile(r"\\\n", re.MULTILINE)
 
@@ -638,9 +565,8 @@ def _SetUp(include_server_port):
     sys.exit("Expected '/' as separator in filepaths.")
 
   client_root_keeper = basics.ClientRootKeeper()
-  # So that we can call this function --- to sweep out possible junk. Also, this
-  # will allow the include analyzer to call InitializeClientRoot.
-  _CleanOutOthers(client_root_keeper)
+  # Clean out any junk left over from prior runs.
+  client_root_keeper.CleanOutOthers()
 
   Debug(DEBUG_TRACE, "Starting socketserver %s" % include_server_port)
 
@@ -664,7 +590,7 @@ def _SetUp(include_server_port):
 def _CleanOut(include_analyzer, include_server_port):
   """Prepare shutdown by cleaning out files and unlinking port."""
   if include_analyzer and include_analyzer.client_root_keeper:
-    _CleanOutClientRoots(include_analyzer.client_root_keeper)
+    include_analyzer.client_root_keeper.CleanOutClientRoots()
   try:
     os.unlink(include_server_port)
   except OSError: