buildstream/_artifactcache/cascache.py: Set 0644 rights to pulled filesvalentindavid/pull-chmod-bug-1.2

This was broken by 9252a18180ce79d70c193768293baa0f0eff9981.

2 files changed, 84 insertions, 5 deletions

diff --git a/buildstream/_artifactcache/cascache.py b/buildstream/_artifactcache/cascache.py
index 837363d9d..20fc9847d 100644
--- a/buildstream/_artifactcache/cascache.py
+++ b/buildstream/_artifactcache/cascache.py
@@ -395,9 +395,7 @@ class CASCache(ArtifactCache):
                     for chunk in iter(lambda: tmp.read(4096), b""):
                         h.update(chunk)
                 else:
-                    tmp = stack.enter_context(tempfile.NamedTemporaryFile(dir=self.tmpdir))
-                    # Set mode bits to 0644
-                    os.chmod(tmp.name, stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
+                    tmp = stack.enter_context(self._temporary_object())
 
                     if path:
                         with open(path, 'rb') as f:
@@ -834,6 +832,19 @@ class CASCache(ArtifactCache):
 
         assert digest.size_bytes == os.fstat(stream.fileno()).st_size
 
+    # _temporary_object():
+    #
+    # Returns:
+    #     (file): A file object to a named temporary file.
+    #
+    # Create a named temporary file with 0o0644 access rights.
+    @contextlib.contextmanager
+    def _temporary_object(self):
+        with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
+            os.chmod(f.name,
+                     stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
+            yield f
+
     # _ensure_blob():
     #
     # Fetch and add blob if it's not already local.
@@ -851,7 +862,7 @@ class CASCache(ArtifactCache):
             # already in local repository
             return objpath
 
-        with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
+        with self._temporary_object() as f:
             self._fetch_blob(remote, digest, f)
 
             added_digest = self.add_object(path=f.name, link_directly=True)
@@ -861,7 +872,7 @@ class CASCache(ArtifactCache):
 
     def _batch_download_complete(self, batch):
         for digest, data in batch.send():
-            with tempfile.NamedTemporaryFile(dir=self.tmpdir) as f:
+            with self._temporary_object() as f:
                 f.write(data)
                 f.flush()
 
diff --git a/tests/frontend/pull.py b/tests/frontend/pull.py
index c883e2030..6c1477596 100644
--- a/tests/frontend/pull.py
+++ b/tests/frontend/pull.py
@@ -1,5 +1,6 @@
 import os
 import shutil
+import stat
 import pytest
 from tests.testutils import cli, create_artifact_share, generate_junction
 
@@ -358,3 +359,70 @@ def test_pull_missing_notifies_user(caplog, cli, tmpdir, datafiles):
 
         assert "INFO    Remote ({}) does not have".format(share.repo) in result.stderr
         assert "SKIPPED Pull" in result.stderr
+
+
+@pytest.mark.datafiles(DATA_DIR)
+def test_pull_access_rights(caplog, cli, tmpdir, datafiles):
+    project = str(datafiles)
+    checkout = os.path.join(str(tmpdir), 'checkout')
+
+    # Work-around datafiles not preserving mode
+    os.chmod(os.path.join(project, 'files/bin-files/usr/bin/hello'), 0o0755)
+
+    # We need a big file that does not go into a batch to test a different
+    # code path
+    os.makedirs(os.path.join(project, 'files/dev-files/usr/share'), exist_ok=True)
+    with open(os.path.join(project, 'files/dev-files/usr/share/big-file'), 'w') as f:
+        buf = ' ' * 4096
+        for _ in range(1024):
+            f.write(buf)
+
+    with create_artifact_share(os.path.join(str(tmpdir), 'artifactshare')) as share:
+
+        cli.configure({
+            'artifacts': {'url': share.repo, 'push': True}
+        })
+        result = cli.run(project=project, args=['build', 'compose-all.bst'])
+        result.assert_success()
+
+        result = cli.run(project=project,
+                         args=['checkout', '--hardlinks', '--no-integrate',
+                               'compose-all.bst', checkout])
+        result.assert_success()
+
+        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
+
+        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
+
+        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
+
+        shutil.rmtree(checkout)
+
+        artifacts = os.path.join(cli.directory, 'artifacts')
+        shutil.rmtree(artifacts)
+
+        result = cli.run(project=project, args=['pull', 'compose-all.bst'])
+        result.assert_success()
+
+        result = cli.run(project=project,
+                         args=['checkout', '--hardlinks', '--no-integrate',
+                               'compose-all.bst', checkout])
+        result.assert_success()
+
+        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
+
+        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
+
+        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