webkitpy: Remove contents of directory.

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

 # Copyright (C) 2010 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 10 matching lines @@
 # OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 # SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 # LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 # DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 """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
 
 
 class FileSystem(object):
     """FileSystem interface for webkitpy.
 
     Unless otherwise noted, all paths are allowed to be either absolute
     or relative.
@@ skipping 80 matching lines @@
 
     def isdir(self, path):
         return os.path.isdir(path)
 
     def join(self, *comps):
         return os.path.join(*comps)
 
     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.
 
         This is like tempfile.mkdtemp, but if used in a with statement
         the directory will self-delete at the end of the block (if the
         directory is empty; non-empty directories raise errors). The
         directory can be safely deleted inside the block as well, if so
         desired.
 
@@ skipping 111 matching lines @@
         while True:
             try:
                 osremove(path)
                 return True
             except exceptions.WindowsError:
                 time.sleep(sleep_interval)
                 retry_timeout_sec -= sleep_interval
                 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)
 
     def split(self, path):
         """Return (dirname, basename + '.' + ext)"""
         return os.path.split(path)
 
     def splitext(self, path):
         """Return (dirname + os.sep + basename, '.' + ext)"""
         return os.path.splitext(path)
 
     def make_executable(self, file_path):
         os.chmod(file_path, stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR | stat.S_IRGRP | stat.S_IXGRP)