[annotated_builder] try harder to clean build directories

[inglorion]

Summary

llvm-objcopy has a test that creates a non-writable file. This file
is causing the clang-with-thin-lto-windows bot to fail because it
cannot clean its build directory. This change makes util.clean_dir()
try harder to remove directory trees, which should allow the bot
to succeed.

[inglorion]

Anyone able to take a look at this?

[amccarth]

This looks like an improvement, but it still might not work on Windows, because there you're racing the file system--deleting a file on Windows is not an atomic, instantaneous operation. Tune in to https://www.youtube.com/watch?v=uhRWMGBjlO8 at 7:30 for details.

[zturner]

Doesn't shutil.rmtree handle this? I believe it's implemented as SHFileOperation. In any case, it's better to not try and re-invent robust directory tree deletion and just use the tools available.

[inglorion]

Doesn't shutil.rmtree handle this?

No. That's the code that's currently running. See here for the error: http://lab.llvm.org:8011/builders/clang-with-thin-lto-windows/builds/8570/steps/stage%202%20clean/logs/stdio

I tested the code on the affected machine. shutil.rmtree fails with the permissions error. The rmtree implementation I wrote succeeds.

[zturner]

I was talking about Adrian’s comment. I know it won’t clear the read only
flag, but Adrian’s comment was about something different.

I don’t think it’s worth the trouble doing anything better though, let the
OS handle it to the best of its ability (which your patch does)

[inglorion]

Ah, my bad. Thanks for clarifying, @zturner.

@amccarth, point about possible incorrect directory tree removal semantics well taken, but I'd still like to ship this change as-is if that's ok with you. The reason is that it fixes a problem that is currently causing essentially every build to fail. Hopefully zturner is right and shutil actually does the right thing on Windows, and if not, I'll be happy to cook up a new patch if we find out it's causing problems.

[amccarth]

Yes, by all means. My intent was not to block this but to point out that it might still suffer from an already existent problem.

If Zach's right that it uses SHFileOperation under the hood, that goes a long way toward solving the problem.

zorg/trunk/zorg/buildbot/builders/annotated/util.py

 from __future__ import print_function
 
 import errno
 import os
 import re
 import shutil
 import subprocess
 import sys
 
 
 def clean_dir(path):
     """
-    Remove directory path if it exists and create a new, empty directory
-    in its place.
+    Removes directory at path (and all its subdirectories) if it exists,
+    and creates an empty directory in its place.
     """
     try:
-        shutil.rmtree(path)
+        rmtree(path)
     except OSError as e:
         if e.errno != errno.ENOENT:
             raise
     mkdirp(path)
 
 
 def cmake_pjoin(*args):
     """
def mkdirp(path):
     """Create directory path if it does not already exist."""
     try:
         os.makedirs(path)
     except OSError as e:
         if e.errno != errno.EEXIST:
             raise
 
 
+def rmtree(path):
+    """
+    Remove directory path and all its subdirectories. This differs from
+    shutil.rmtree() in that it tries to adjust permissions so that deletion
+    will succeed.
+    """
+    # Some files will not be deletable, so we set permissions that allow
+    # deletion before we try deleting files.
+    for root, dirs, files in os.walk(path):
+        os.chmod(root, 0o755)
+        for f in files:
+            p = os.path.join(root, f)
+            os.chmod(p, 0o644)
+            os.unlink(p)
+    # At this point, we should have a tree of deletable directories.
+    shutil.rmtree(path)
+
+
 def safe_pjoin(dirname, *args):
     """
     Join path components with os.path.join, skipping the first component
     if it is None.
     """
     if dirname is None:
         return os.path.join(*args)
     else: