dvc: ignore errors on protect/set_exec

dvc/cache/base.py

@@ -503,6 +503,9 @@ def is_protected(self, path_info):  # pylint: disable=unused-argument
     def unprotect(self, path_info):  # pylint: disable=unused-argument
         pass
 
+    def set_exec(self, path_info):  # pylint: disable=unused-argument
+        pass
+
     def changed_cache_file(self, hash_info):
         """Compare the given hash with the (corresponding) actual one.
 

dvc/cache/local.py

@@ -1,5 +1,6 @@
 import logging
 import os
+import stat
 
 from funcy import cached_property
 from shortuuid import uuid
@@ -154,14 +155,30 @@ def unprotect(self, path_info):
             self._unprotect_file(path_info)
 
     def protect(self, path_info):
-        self.tree.chmod(path_info, self.CACHE_MODE)
+        try:
+            os.chmod(path_info, self.CACHE_MODE)
+        except OSError:
+            # NOTE: not being able to protect cache file is not fatal, it
+            # might happen on funky filesystems (e.g. Samba, see #5255),
+            # read-only filesystems or in a shared cache scenario.
+            logger.trace("failed to protect '%s'", path_info, exc_info=True)
 
     def is_protected(self, path_info):
-        import stat
-
         try:
             mode = os.stat(path_info).st_mode
         except FileNotFoundError:
             return False
 
         return stat.S_IMODE(mode) == self.CACHE_MODE
+
+    def set_exec(self, path_info):
+        mode = os.stat(path_info).st_mode | stat.S_IEXEC
+        try:
+            os.chmod(path_info, mode)
+        except OSError:
+            logger.trace(
+                "failed to chmod '%s' '%s'",
+                oct(mode),
+                path_info,
+                exc_info=True,
+            )

dvc/output/base.py

@@ -300,7 +300,7 @@ def save(self):
 
     def set_exec(self):
         if self.isfile() and self.isexec:
-            self.tree.set_exec(self.path_info)
+            self.cache.set_exec(self.path_info)
 
     def commit(self, filter_info=None):
         if not self.exists:

dvc/tree/base.py

@@ -181,9 +181,6 @@ def isfile(self, path_info):
         """
         return True
 
-    def set_exec(self, path_info):
-        raise RemoteActionNotImplemented("set_exec", self.scheme)
-
     def isexec(self, path_info):
         """Optional: Overwrite only if the remote has a way to distinguish
         between executable and non-executable file.

dvc/tree/local.py

@@ -1,7 +1,5 @@
-import errno
 import logging
 import os
-import stat
 
 from funcy import cached_property
 
@@ -136,10 +134,6 @@ def remove(self, path_info):
     def makedirs(self, path_info):
         makedirs(path_info, exist_ok=True)
 
-    def set_exec(self, path_info):
-        mode = self.stat(path_info).st_mode
-        self.chmod(path_info, mode | stat.S_IEXEC)
-
     def isexec(self, path_info):
         mode = self.stat(path_info).st_mode
         return is_exec(mode)
@@ -213,23 +207,6 @@ def is_hardlink(path_info):
     def reflink(self, from_info, to_info):
         System.reflink(from_info, to_info)
 
-    def chmod(self, path_info, mode):
-        try:
-            os.chmod(path_info, mode)
-        except OSError as exc:
-            # There is nothing we need to do in case of a read-only file system
-            if exc.errno == errno.EROFS:
-                return
-
-            # In shared cache scenario, we might not own the cache file, so we
-            # need to check if cache file is already protected.
-            if exc.errno not in [errno.EPERM, errno.EACCES]:
-                raise
-
-            actual = stat.S_IMODE(os.stat(path_info).st_mode)
-            if actual != mode:
-                raise
-
     def get_file_hash(self, path_info):
         hash_info = HashInfo(self.PARAM_CHECKSUM, file_md5(path_info)[0],)
 

tests/unit/cache/test_local.py

@@ -66,26 +66,23 @@ def test_is_protected(tmp_dir, dvc, link_name):
         assert cache.is_protected(foo)
 
 
-@pytest.mark.parametrize("err", [errno.EPERM, errno.EACCES])
+@pytest.mark.parametrize("err", [errno.EPERM, errno.EACCES, errno.EROFS])
 def test_protect_ignore_errors(tmp_dir, dvc, mocker, err):
     tmp_dir.gen("foo", "foo")
-    foo = PathInfo("foo")
-
-    dvc.cache.local.protect(foo)
 
     mock_chmod = mocker.patch(
         "os.chmod", side_effect=OSError(err, "something")
     )
-    dvc.cache.local.protect(foo)
+    dvc.cache.local.protect(PathInfo("foo"))
     assert mock_chmod.called
 
 
-def test_protect_ignore_erofs(tmp_dir, dvc, mocker):
+@pytest.mark.parametrize("err", [errno.EPERM, errno.EACCES, errno.EROFS])
+def test_set_exec_ignore_errors(tmp_dir, dvc, mocker, err):
     tmp_dir.gen("foo", "foo")
-    foo = PathInfo("foo")
 
     mock_chmod = mocker.patch(
-        "os.chmod", side_effect=OSError(errno.EROFS, "read-only fs")
+        "os.chmod", side_effect=OSError(err, "something")
     )
-    dvc.cache.local.protect(foo)
+    dvc.cache.local.set_exec(PathInfo("foo"))
     assert mock_chmod.called