Ensure that artifact file permissions are set in the right order

4 files changed, 39 insertions, 9 deletions

diff --git a/buildstream/_artifactcache/ostreecache.py b/buildstream/_artifactcache/ostreecache.py
index 0321e8e47..48788f4b9 100644
--- a/buildstream/_artifactcache/ostreecache.py
+++ b/buildstream/_artifactcache/ostreecache.py
@@ -209,7 +209,10 @@ class OSTreeCache(ArtifactCache):
         # also store under weak cache key
         weak_ref = buildref(element, element._get_cache_key(strength=_KeyStrength.WEAK))
 
-        _ostree.commit(self.repo, content, ref, weak_ref)
+        try:
+            _ostree.commit(self.repo, content, ref, weak_ref)
+        except OSTreeError as e:
+            raise _ArtifactError("Failed to commit artifact: {}".format(e)) from e
 
     # pull():
     #
diff --git a/buildstream/_ostree.py b/buildstream/_ostree.py
index 40bfda644..d6e8c9ef5 100644
--- a/buildstream/_ostree.py
+++ b/buildstream/_ostree.py
@@ -153,6 +153,12 @@ def commit(repo, dir, ref, branch=None):
 
         # complete repo transaction
         repo.commit_transaction(None)
+    except GLib.IOError as e:
+
+        # Reraise any error as a buildstream error
+        repo.abort_transaction()
+        raise OSTreeError(e.message) from e
+
     except:
         repo.abort_transaction()
         raise
diff --git a/buildstream/element.py b/buildstream/element.py
index e3e9a4b34..a956246d9 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -1040,7 +1040,7 @@ class Element(Plugin):
 
             # Cleanup the build directory on explicit SIGTERM
             def cleanup_rootdir():
-                shutil.rmtree(rootdir)
+                utils._force_rmtree(rootdir)
 
             with _signals.terminator(cleanup_rootdir), \
                 self.__sandbox(rootdir, output_file, output_file) as sandbox:  # nopep8
@@ -1116,7 +1116,7 @@ class Element(Plugin):
                         self.__artifacts.commit(self, assembledir)
 
             # Finally cleanup the build dir
-            shutil.rmtree(rootdir)
+            cleanup_rootdir()
 
     # _pull():
     #
@@ -1415,7 +1415,7 @@ class Element(Plugin):
                 yield sandbox
 
             # Cleanup the build dir
-            shutil.rmtree(rootdir)
+            utils._force_rmtree(rootdir)
 
     def __compose_default_splits(self, defaults):
         project = self.get_project()
diff --git a/buildstream/utils.py b/buildstream/utils.py
index 273d5dd0b..2a873bf57 100644
--- a/buildstream/utils.py
+++ b/buildstream/utils.py
@@ -465,14 +465,26 @@ def get_bst_version():
     return (int(versions[0]), int(versions[1]))
 
 
-# Recursively make directories in target area and copy permissions
+# Recursively remove directories, ignoring file permissions as much as
+# possible.
+def _force_rmtree(path, **kwargs):
+    for root, dirs, _ in os.walk(path):
+        for d in dirs:
+            path = os.path.join(root, d.lstrip('/'))
+            if os.path.exists(path) and not os.path.islink(path):
+                os.chmod(path, 0o755)
+
+    shutil.rmtree(path, **kwargs)
+
+
+# Recursively make directories in target area
 def _copy_directories(srcdir, destdir, target):
     this_dir = os.path.dirname(target)
     new_dir = os.path.join(destdir, this_dir)
 
     if not os.path.lexists(new_dir):
         if this_dir:
-            _copy_directories(srcdir, destdir, this_dir)
+            yield from _copy_directories(srcdir, destdir, this_dir)
 
         old_dir = os.path.join(srcdir, this_dir)
         if os.path.lexists(old_dir):
@@ -481,7 +493,7 @@ def _copy_directories(srcdir, destdir, target):
 
             if stat.S_ISDIR(mode) or stat.S_ISLNK(mode):
                 os.makedirs(new_dir)
-                shutil.copystat(old_dir, new_dir)
+                yield (new_dir, mode)
             else:
                 raise OSError('Source directory tree has file where '
                               'directory expected: %s' % dir)
@@ -533,6 +545,10 @@ def _ensure_real_directory(root, destpath):
 #
 def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=False):
 
+    # Keep track of directory permissions, since these need to be set
+    # *after* files have been written.
+    permissions = []
+
     # Note we consume the filelist (which is a generator and not a list)
     # by sorting it, this is necessary to ensure that we processes symbolic
     # links which lead to directories before processing files inside those
@@ -547,7 +563,7 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
             result.overwritten.append(path)
 
         # The destination directory may not have been created separately
-        _copy_directories(srcdir, destdir, path)
+        permissions.extend(_copy_directories(srcdir, destdir, path))
 
         # Ensure that broken symlinks to directories have their targets
         # created before attempting to stage files across broken
@@ -557,6 +573,7 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
         try:
             file_stat = os.lstat(srcpath)
             mode = file_stat.st_mode
+
         except FileNotFoundError:
             # Skip this missing file
             if ignore_missing:
@@ -573,7 +590,7 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
             if not stat.S_ISDIR(dest_stat.st_mode):
                 raise OSError('Destination not a directory. source has %s'
                               ' destination has %s' % (srcpath, destpath))
-            shutil.copystat(srcpath, destpath)
+            permissions.append((destpath, os.stat(srcpath).st_mode))
 
         elif stat.S_ISLNK(mode):
             if not safe_remove(destpath):
@@ -607,6 +624,10 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
             # Unsupported type.
             raise OSError('Cannot extract %s into staging-area. Unsupported type.' % srcpath)
 
+    # Write directory permissions now that all files have been written
+    for d, perms in permissions:
+        os.chmod(d, perms)
+
 
 # _relative_symlink_target()
 #