webkitpy: Remove contents of directory.

third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py b/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
index c06c190f8e66399a55140e0fbcdfeceba612bab1..e6fc1b556d859d4940a7525d674c03880190bb16 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
@@ -28,14 +28,15 @@
 
 """Wrapper object for the file system / source tree."""
 
-import stat
 import codecs
 import errno
 import exceptions
 import glob
 import hashlib
+import logging

[qyearsley, 2017/05/25 23:32:12]

Nit: The general convention in webkitpy is to use

    _log = logging.getLogger(__name__)

And then when logging, use _log.method instead of logging.method.

Example:
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

The main practical benefit of this is that when tools use these common modules
they have verbose logging modes which print the place from where the message is
being logged. For example:

./webkit-patch auto-rebaseline -v 
webkitpy.common.system.log_utils: [DEBUG] Debug logging enabled.
webkitpy.common.system.executive: [DEBUG] "git rev-parse --is-inside-work-tree"
took 0.02s
...

If, instead, we directly using "logging.debug" in executive.py, this would
instead print:
webkitpy.common.system.log_utils: [DEBUG] Debug logging enabled.
root: [DEBUG] "git rev-parse --is-inside-work-tree" took 0.02s

[mithro, 2017/05/26 04:04:23]

Done.

 import os
 import shutil
+import stat
 import sys
 import tempfile
 import time
@@ -136,8 +137,8 @@ class FileSystem(object):
     def listdir(self, path):
         return os.listdir(path)
 
-    def walk(self, top):
-        return os.walk(top)
+    def walk(self, top, topdown=True, onerror=None, followlinks=False):
+        return os.walk(top, topdown=topdown, onerror=onerror, followlinks=followlinks)
 
     def mkdtemp(self, **kwargs):
         """Create and return a uniquely named directory.
@@ -269,9 +270,72 @@ class FileSystem(object):
                 if retry_timeout_sec < 0:
                     raise
 
-    def rmtree(self, path):
+    def rmtree(self, path, ignore_errors=True, onerror=None):
         """Delete the directory rooted at path, whether empty or not."""
-        shutil.rmtree(path, ignore_errors=True)
+        shutil.rmtree(path, ignore_errors=ignore_errors, onerror=onerror)
+
+    def remove_contents(self, dirname):
+        """Attempt to remove the contents of a directory tree.
+        Args:
+            dirname (string): Directory to remove the contents of.
+
+        Returns:
+            bool: True when the directory is now empty.

[mcgreevy_g, 2017/05/26 06:38:01]

s/when/if/

[mithro, 2017/05/29 03:55:58]

Done.

+        """
+        # We try multiple times as on Windows a process which is currently closing

[mcgreevy_g, 2017/05/26 06:38:01]

I'd put a comma after "times" and use "because" instead of "as".

[mithro, 2017/05/29 03:55:58]

Done.

+        # could still have a file open in the directory.
+        logging.info('Removing contents of %s', dirname)
+        errors = []
+
+        def onerror(func, path, exc_info):
+            errors.append(path)
+            logging.exception('Failed at %s %s: %r', func, path, exc_info)
+
+        attempts = 0
+        while attempts < 5:
+            del errors[:]
+
+            for name in self.listdir(dirname):
+                fullname = self.join(dirname, name)
+
+                isdir = True

[mcgreevy_g, 2017/05/26 06:38:01]

I think it's odd to intialize this to True when you are unconditionally setting
it on line 303, and you continue in the case of an exception.  I would
initialize it to None here. But I defer to your judgment when it comes to python
idioms.

[mithro, 2017/05/29 03:55:57]

Acknowledged.

+                try:
+                    isdir = self.isdir(fullname)
+                except os.error:
+                    onerror(self.isdir, fullname, sys.exc_info())
+                    continue
+
+                if isdir:
+                    try:
+                        self.rmtree(fullname, ignore_errors=False, onerror=onerror)
+                    except os.error:
+                        onerror(self.rmtree, fullname, sys.exc_info())
+                        continue
+                else:
+                    try:
+                        self.remove(fullname)

[mcgreevy_g, 2017/05/26 06:38:01]

The implementation of remove has a series of retries, but you're calling it from
inside another retry loop here.  So you'll wait up to 5 attempts * 3s = 15s in
the case where you're waiting on a single file.  If you have N files that you're
failing to remove (which never succeed) you'll end up waiting 15*N seconds.

Instead, I would decide on a deadline before line 295. Get rid of the while
attempts < 5 loop, and work through each entry in dirname one by one,
reattempting each one an indefinite number of times until the deadline expires. 
At that point, either give up, or just try to delete the rest of the entries
with no retries.

You will end up allowing equal time for each entry to be deleted (because while
you're waiting on one, you're also waiting on the rest), but the timeout will be
predicable rather than depending on the number of files.

[mithro, 2017/05/29 03:55:57]

I added a flag to prevent retry behaviour in the remove function. The desired
behaviour is to attempt to delete the whole directory three times -- I'm not
really after a timeout behaviour.

If a file is failed to be removed, it will cause a kind of cascade where the
parent directories will fail to remove (because they are not empty). This is why
we want to try the whole process again.

[mcgreevy_g, 2017/05/29 04:17:56]

Why three?
But this code operates from the top of the tree, and relies on shutil.rmtree to
traverse down the tree.  So as far as this code is concerned, a failure (of
either "remove" or "rmtree") does not require any special handling of parent
directories.  What am I missing?

[mithro, 2017/05/29 06:00:30]

Arbitrary number.
You are correct. 

The cascade will only happen in the rmtree path. The errors in that case get
handled by the onerror function.

[mcgreevy_g, 2017/05/29 07:00:54]

I still think setting a deadline would be nicer, but at least with retries
disabled in remove, the running time is now predictable.

+                    except os.error:
+                        onerror(self.remove, fullname, sys.exc_info())
+                        continue
+
+            if not errors:
+                break
+
+            attempts += 1
+            time.sleep(1)
+
+        # Check the path is gone.
+        if not self.listdir(dirname):
+            return True
+
+        logging.warning('Unable to remove %s', dirname)
+        for dirpath, dirnames, filenames in self.walk(dirname, onerror=onerror, topdown=False):
+            for fname in filenames:
+                logging.warning('File %s still in output dir.', self.join(dirpath, fname))
+            for dname in dirnames:
+                logging.warning('Dir %s still in output dir.', self.join(dirpath, dname))
+
+        return False
 
     def copytree(self, source, destination):
         shutil.copytree(source, destination)