Shorten RemoveDirectory and rename to rmtree. Remove rmtree from fake_repos.

Keep an alias to RemoveDirectory, will be removed in a later change.

TEST=new unit test to test this function
BUG=none

Review URL: http://codereview.chromium.org/6628032

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@77463 0039d316-1c4b-4281-b951-d872f2087c98

gclient_utils.py

@@ -182,10 +182,10 @@ def FileWrite(filename, content, mode='w'):
     f.close()
 
 
-def RemoveDirectory(*path):
+def rmtree(path):
-  """Recursively removes a directory, even if it's marked read-only.
+  """shutil.rmtree() on steroids.
 
-  Remove the directory located at *path, if it exists.
+  Recursively removes a directory, even if it's marked read-only.
 
   shutil.rmtree() doesn't work on Windows if any of the files or directories
   are read-only, which svn repositories and some .svn files are.  We need to
@@ -208,63 +208,55 @@ def RemoveDirectory(*path):
   In the ordinary case, this is not a problem: for our purposes, the user
   will never lack write permission on *path's parent.
   """
-  logging.debug(path)
+  if not os.path.exists(path):
-  file_path = os.path.join(*path)
-  if not os.path.exists(file_path):
     return
 
-  if os.path.islink(file_path) or not os.path.isdir(file_path):
+  if os.path.islink(path) or not os.path.isdir(path):
-    raise Error('RemoveDirectory asked to remove non-directory %s' % file_path)
+    raise Error('Called rmtree(%s) in non-directory' % path)
 
-  has_win32api = False
   if sys.platform == 'win32':
-    has_win32api = True
     # Some people don't have the APIs installed. In that case we'll do without.
     try:
       win32api = __import__('win32api')
       win32con = __import__('win32con')
     except ImportError:
-      has_win32api = False
+      pass
   else:
     # On POSIX systems, we need the x-bit set on the directory to access it,
     # the r-bit to see its contents, and the w-bit to remove files from it.
     # The actual modes of the files within the directory is irrelevant.
-    os.chmod(file_path, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
+    os.chmod(path, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
-  for fn in os.listdir(file_path):
-    fullpath = os.path.join(file_path, fn)
 
+  def remove(func, subpath):
+    if sys.platform == 'win32':
+      os.chmod(subpath, stat.S_IWRITE)
+      if win32api and win32con:
+        win32api.SetFileAttributes(subpath, win32con.FILE_ATTRIBUTE_NORMAL)
+    try:
+      func(subpath)
+    except OSError, e:
+      if e.errno != errno.EACCES or sys.platform != 'win32':
+        raise
+      # Failed to delete, try again after a 100ms sleep.
+      time.sleep(0.1)
+      func(subpath)
+
+  for fn in os.listdir(path):
     # If fullpath is a symbolic link that points to a directory, isdir will
     # be True, but we don't want to descend into that as a directory, we just
     # want to remove the link.  Check islink and treat links as ordinary files
     # would be treated regardless of what they reference.
+    fullpath = os.path.join(path, fn)
     if os.path.islink(fullpath) or not os.path.isdir(fullpath):
-      if sys.platform == 'win32':
+      remove(os.remove, fullpath)
-        os.chmod(fullpath, stat.S_IWRITE)
-        if has_win32api:
-          win32api.SetFileAttributes(fullpath, win32con.FILE_ATTRIBUTE_NORMAL)
-      try:
-        os.remove(fullpath)
-      except OSError, e:
-        if e.errno != errno.EACCES or sys.platform != 'win32':
-          raise
-        print 'Failed to delete %s: trying again' % fullpath
-        time.sleep(0.1)
-        os.remove(fullpath)
     else:
-      RemoveDirectory(fullpath)
+      # Recurse.
+      rmtree(fullpath)
 
-  if sys.platform == 'win32':
+  remove(os.rmdir, path)
-    os.chmod(file_path, stat.S_IWRITE)
+
-    if has_win32api:
+# TODO(maruel): Rename the references.
-      win32api.SetFileAttributes(file_path, win32con.FILE_ATTRIBUTE_NORMAL)
+RemoveDirectory = rmtree
-  try:
-    os.rmdir(file_path)
-  except OSError, e:
-    if e.errno != errno.EACCES or sys.platform != 'win32':
-      raise
-    print 'Failed to remove %s: trying again' % file_path
-    time.sleep(0.1)
-    os.rmdir(file_path)
 
 
 def CheckCallAndFilterAndHeader(args, always=False, **kwargs):

tests/fake_repos.py

@@ -7,20 +7,18 @@
 
 import atexit
 import datetime
-import errno
 import logging
 import os
 import pprint
 import re
 import socket
-import stat
 import subprocess
 import sys
 import tempfile
-import time
 
 # trial_dir must be first for non-system libraries.
 from tests import trial_dir
+import gclient_utils
 import scm
 
 ## Utility functions
@@ -66,71 +64,6 @@ def add_kill():
     subprocess.Popen.kill = kill_nix
 
 
-def rmtree(*path):
-  """Recursively removes a directory, even if it's marked read-only.
-
-  Remove the directory located at *path, if it exists.
-
-  shutil.rmtree() doesn't work on Windows if any of the files or directories
-  are read-only, which svn repositories and some .svn files are.  We need to
-  be able to force the files to be writable (i.e., deletable) as we traverse
-  the tree.
-
-  Even with all this, Windows still sometimes fails to delete a file, citing
-  a permission error (maybe something to do with antivirus scans or disk
-  indexing).  The best suggestion any of the user forums had was to wait a
-  bit and try again, so we do that too.  It's hand-waving, but sometimes it
-  works. :/
-  """
-  file_path = os.path.join(*path)
-  if not os.path.exists(file_path):
-    return
-
-  def RemoveWithRetry_win(rmfunc, path):
-    os.chmod(path, stat.S_IWRITE)
-    if win32_api_avail:
-      win32api.SetFileAttributes(path, win32con.FILE_ATTRIBUTE_NORMAL)
-    try:
-      return rmfunc(path)
-    except EnvironmentError, e:
-      if e.errno != errno.EACCES:
-        raise
-      print 'Failed to delete %s: trying again' % repr(path)
-      time.sleep(0.1)
-      return rmfunc(path)
-
-  def RemoveWithRetry_non_win(rmfunc, path):
-    if os.path.islink(path):
-      return os.remove(path)
-    else:
-      return rmfunc(path)
-
-  win32_api_avail = False
-  remove_with_retry = None
-  if sys.platform.startswith('win'):
-    # Some people don't have the APIs installed. In that case we'll do without.
-    try:
-      win32api = __import__('win32api')
-      win32con = __import__('win32con')
-      win32_api_avail = True
-    except ImportError:
-      pass
-    remove_with_retry = RemoveWithRetry_win
-  else:
-    remove_with_retry = RemoveWithRetry_non_win
-
-  for root, dirs, files in os.walk(file_path, topdown=False):
-    # For POSIX:  making the directory writable guarantees removability.
-    # Windows will ignore the non-read-only bits in the chmod value.
-    os.chmod(root, 0770)
-    for name in files:
-      remove_with_retry(os.remove, os.path.join(root, name))
-    for name in dirs:
-      remove_with_retry(os.rmdir, os.path.join(root, name))
-
-  remove_with_retry(os.rmdir, file_path)
-
-
 def write(path, content):
   f = open(path, 'wb')
   f.write(content)
@@ -309,9 +242,9 @@ class FakeReposBase(object):
       self.svnserve = None
       if not self.trial.SHOULD_LEAK:
         logging.debug('Removing %s' % self.svn_repo)
-        rmtree(self.svn_repo)
+        gclient_utils.rmtree(self.svn_repo)
         logging.debug('Removing %s' % self.svn_checkout)
-        rmtree(self.svn_checkout)
+        gclient_utils.rmtree(self.svn_checkout)
       else:
         return False
     return True
@@ -330,7 +263,7 @@ class FakeReposBase(object):
       self.wait_for_port_to_free(self.git_port)
       if not self.trial.SHOULD_LEAK:
         logging.debug('Removing %s' % self.git_root)
-        rmtree(self.git_root)
+        gclient_utils.rmtree(self.git_root)
       else:
         return False
     return True

tests/gclient_utils_test.py

@@ -9,7 +9,8 @@ import StringIO
 
 # Fixes include path.
 from super_mox import SuperMoxTestBase
-
+import trial_dir
+import os
 import gclient_utils
 
 
@@ -32,8 +33,8 @@ class GclientUtilsUnittest(GclientUtilBase):
         'ParseXML', 'Popen',
         'PrintableObject', 'RemoveDirectory', 'SoftClone', 'SplitUrlRevision',
         'SyntaxErrorToError', 'WorkItem',
-        'errno', 'hack_subprocess', 'logging', 'os', 'Queue', 're', 'stat',
+        'errno', 'hack_subprocess', 'logging', 'os', 'Queue', 're', 'rmtree',
-        'subprocess', 'sys','threading', 'time', 'xml',
+        'stat', 'subprocess', 'sys','threading', 'time', 'xml',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(gclient_utils, members)
@@ -181,6 +182,24 @@ class SplitUrlRevisionTestCase(GclientUtilBase):
     self.assertEquals(out_url, url)
 
 
+class GClientUtilsTest(trial_dir.TestCase):
+  def testHardToDelete(self):
+    # Use the fact that tearDown will delete the directory to make it hard to do
+    # so.
+    l1 = os.path.join(self.root_dir, 'l1')
+    l2 = os.path.join(l1, 'l2')
+    l3 = os.path.join(l2, 'l3')
+    f3 = os.path.join(l3, 'f3')
+    os.mkdir(l1)
+    os.mkdir(l2)
+    os.mkdir(l3)
+    gclient_utils.FileWrite(f3, 'foo')
+    os.chmod(f3, 0)
+    os.chmod(l3, 0)
+    os.chmod(l2, 0)
+    os.chmod(l1, 0)
+
+
 if __name__ == '__main__':
   import unittest
   unittest.main()

tests/scm_unittest.py

@@ -276,7 +276,7 @@ class RealSvnTest(fake_repos.FakeReposTestBase):
     self._capture(
         ['propset', 'foo', 'bar',
          scm.os.path.join(self.svn_root, 'prout', 'origin')])
-    fake_repos.rmtree(scm.os.path.join(self.svn_root, 'prout'))
+    fake_repos.gclient_utils.rmtree(scm.os.path.join(self.svn_root, 'prout'))
     with open(scm.os.path.join(self.svn_root, 'faa'), 'w') as f:
       f.write('eh')
     with open(scm.os.path.join(self.svn_root, 'faala'), 'w') as f: