diff --git a/dvc/cache/base.py b/dvc/cache/base.py index d0bc7650fe..7f2876766c 100644 --- a/dvc/cache/base.py +++ b/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. diff --git a/dvc/cache/local.py b/dvc/cache/local.py index d3cb477608..c35f67208c 100644 --- a/dvc/cache/local.py +++ b/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, + ) diff --git a/dvc/output/base.py b/dvc/output/base.py index 4dea170a79..7242e84783 100644 --- a/dvc/output/base.py +++ b/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: diff --git a/dvc/tree/base.py b/dvc/tree/base.py index 6bbb36c6d2..af5dca59f9 100644 --- a/dvc/tree/base.py +++ b/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. diff --git a/dvc/tree/local.py b/dvc/tree/local.py index 71a51510eb..813f9f0198 100644 --- a/dvc/tree/local.py +++ b/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],) diff --git a/tests/unit/cache/test_local.py b/tests/unit/cache/test_local.py index d50354293a..7f7818f8f8 100644 --- a/tests/unit/cache/test_local.py +++ b/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