Merge branch 'juerg/object-file-mode' into 'master'

cascache.py: Fix file modes in checkout()

See merge request BuildStream/buildstream!1959

4 files changed, 45 insertions, 36 deletions

diff --git a/src/buildstream/_cas/cascache.py b/src/buildstream/_cas/cascache.py
index 9394ac5b2..6b2f0d1a4 100644
--- a/src/buildstream/_cas/cascache.py
+++ b/src/buildstream/_cas/cascache.py
@@ -241,21 +241,20 @@ class CASCache:
             if can_link and mtime is None:
                 utils.safe_link(self.objpath(filenode.digest), fullpath)
             else:
-                utils.safe_copy(self.objpath(filenode.digest), fullpath)
+                utils.safe_copy(self.objpath(filenode.digest), fullpath, copystat=False)
                 if mtime is not None:
                     utils._set_file_mtime(fullpath, mtime)
 
             if filenode.is_executable:
-                os.chmod(
-                    fullpath,
-                    stat.S_IRUSR
-                    | stat.S_IWUSR
-                    | stat.S_IXUSR
-                    | stat.S_IRGRP
-                    | stat.S_IXGRP
-                    | stat.S_IROTH
-                    | stat.S_IXOTH,
-                )
+                st = os.stat(fullpath)
+                mode = st.st_mode
+                if mode & stat.S_IRUSR:
+                    mode |= stat.S_IXUSR
+                if mode & stat.S_IRGRP:
+                    mode |= stat.S_IXGRP
+                if mode & stat.S_IROTH:
+                    mode |= stat.S_IXOTH
+                os.chmod(fullpath, mode)
 
         for dirnode in directory.directories:
             fullpath = os.path.join(dest, dirnode.name)
diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py
index 99c22d169..9c6761ccc 100644
--- a/src/buildstream/utils.py
+++ b/src/buildstream/utils.py
@@ -363,19 +363,20 @@ def sha256sum(filename: str) -> str:
     return h.hexdigest()
 
 
-def safe_copy(src: str, dest: str, *, result: Optional[FileListResult] = None) -> None:
-    """Copy a file while preserving attributes
+def safe_copy(src: str, dest: str, *, copystat: bool = True, result: Optional[FileListResult] = None) -> None:
+    """Copy a file while optionally preserving attributes
 
     Args:
        src: The source filename
        dest: The destination filename
+       copystat: Whether to preserve attributes
        result: An optional collective result
 
     Raises:
        UtilError: In the case of unexpected system call failures
 
-    This is almost the same as shutil.copy2(), except that
-    we unlink *dest* before overwriting it if it exists, just
+    This is almost the same as shutil.copy2() when copystat is True,
+    except that we unlink *dest* before overwriting it if it exists, just
     incase *dest* is a hardlink to a different file.
     """
     # First unlink the target if it exists
@@ -385,22 +386,24 @@ def safe_copy(src: str, dest: str, *, result: Optional[FileListResult] = None) -
         if e.errno != errno.ENOENT:
             raise UtilError("Failed to remove destination file '{}': {}".format(dest, e)) from e
 
-    shutil.copyfile(src, dest)
     try:
-        shutil.copystat(src, dest)
-    except PermissionError:
-        # If we failed to copy over some file stats, dont treat
-        # it as an unrecoverable error, but provide some feedback
-        # we can use for a warning.
-        #
-        # This has a tendency of happening when attempting to copy
-        # over extended file attributes.
-        if result:
-            result.failed_attributes.append(dest)
-
-    except shutil.Error as e:
+        shutil.copyfile(src, dest)
+    except (OSError, shutil.Error) as e:
         raise UtilError("Failed to copy '{} -> {}': {}".format(src, dest, e)) from e
 
+    if copystat:
+        try:
+            shutil.copystat(src, dest)
+        except PermissionError:
+            # If we failed to copy over some file stats, dont treat
+            # it as an unrecoverable error, but provide some feedback
+            # we can use for a warning.
+            #
+            # This has a tendency of happening when attempting to copy
+            # over extended file attributes.
+            if result:
+                result.failed_attributes.append(dest)
+
 
 def safe_link(src: str, dest: str, *, result: Optional[FileListResult] = None, _unlink=False) -> None:
     """Try to create a hardlink, but resort to copying in the case of cross device links.
diff --git a/tests/frontend/pull.py b/tests/frontend/pull.py
index 6a5a41977..c1fa76ab0 100644
--- a/tests/frontend/pull.py
+++ b/tests/frontend/pull.py
@@ -442,6 +442,8 @@ def test_pull_access_rights(cli, tmpdir, datafiles):
     project = str(datafiles)
     checkout = os.path.join(str(tmpdir), "checkout")
 
+    umask = utils.get_umask()
+
     # Work-around datafiles not preserving mode
     os.chmod(os.path.join(project, "files/bin-files/usr/bin/hello"), 0o0755)
 
@@ -467,15 +469,15 @@ def test_pull_access_rights(cli, tmpdir, datafiles):
 
         st = os.lstat(os.path.join(checkout, "usr/include/pony.h"))
         assert stat.S_ISREG(st.st_mode)
-        assert stat.S_IMODE(st.st_mode) == 0o0644
+        assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask
 
         st = os.lstat(os.path.join(checkout, "usr/bin/hello"))
         assert stat.S_ISREG(st.st_mode)
-        assert stat.S_IMODE(st.st_mode) == 0o0755
+        assert stat.S_IMODE(st.st_mode) == 0o0777 & ~umask
 
         st = os.lstat(os.path.join(checkout, "usr/share/big-file"))
         assert stat.S_ISREG(st.st_mode)
-        assert stat.S_IMODE(st.st_mode) == 0o0644
+        assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask
 
         shutil.rmtree(checkout)
 
@@ -493,15 +495,15 @@ def test_pull_access_rights(cli, tmpdir, datafiles):
 
         st = os.lstat(os.path.join(checkout, "usr/include/pony.h"))
         assert stat.S_ISREG(st.st_mode)
-        assert stat.S_IMODE(st.st_mode) == 0o0644
+        assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask
 
         st = os.lstat(os.path.join(checkout, "usr/bin/hello"))
         assert stat.S_ISREG(st.st_mode)
-        assert stat.S_IMODE(st.st_mode) == 0o0755
+        assert stat.S_IMODE(st.st_mode) == 0o0777 & ~umask
 
         st = os.lstat(os.path.join(checkout, "usr/share/big-file"))
         assert stat.S_ISREG(st.st_mode)
-        assert stat.S_IMODE(st.st_mode) == 0o0644
+        assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask
 
 
 # Tests `bst artifact pull $artifact_ref`
diff --git a/tests/sources/remote.py b/tests/sources/remote.py
index d546c6131..416d2d53c 100644
--- a/tests/sources/remote.py
+++ b/tests/sources/remote.py
@@ -5,6 +5,7 @@ import os
 import stat
 import pytest
 
+from buildstream import utils
 from buildstream.testing import ErrorDomain
 from buildstream.testing import generate_project
 from buildstream.testing import cli  # pylint: disable=unused-import
@@ -71,8 +72,12 @@ def test_simple_file_build(cli, tmpdir, datafiles):
     mode = os.stat(checkout_file).st_mode
     # Assert not executable by anyone
     assert not mode & (stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH)
-    # Assert not writeable by anyone other than me
-    assert not mode & (stat.S_IWGRP | stat.S_IWOTH)
+
+    # Assert not writeable by anyone other than me (unless umask allows it)
+    if utils.get_umask() & stat.S_IWGRP:
+        assert not mode & stat.S_IWGRP
+    if utils.get_umask() & stat.S_IWOTH:
+        assert not mode & stat.S_IWOTH
 
 
 @pytest.mark.datafiles(os.path.join(DATA_DIR, "single-file-custom-name"))