[WIP] dvc: preserve exec bit for tracked files (#5061)

dudarev · efiop · web-flow · commit dcea83e75b75 · 2020-12-20T02:31:06.000+02:00
* #4578 added is_user_executable to dvc file * Addressed PR feedback and introduced test_add_executable Also fixed all tests in test_add.py * Added a test to checkout executable file * Pop isexec in dvc/output/__init__.py:loadd_from * Implement test that file is executable after checkout * Address PR feedback * Address PR feedback * Changed BaseTree.isexec to use path_info as an argument * Update tests/func/test_add.py * Update tests/func/test_add.py * Update tests/func/test_add.py * Update tests/func/test_add.py * fix formatting * add: preserve exec * checkout: fix setting exec * tests: refactor checkout exec test * tests: fix windows cases * tests: add: fix exec test on windows * Update dvc/tree/local.py * Update dvc/output/base.py * Update dvc/output/base.py * Update dvc/cache/base.py * Update tests/func/test_add.py * tests: add: skip whole exec test on windows * save isexec to lockfiles * fix formatting Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com> Co-authored-by: Ruslan Kuprieiev <ruslan@iterative.ai>
diff --git a/dvc/output/__init__.py b/dvc/output/__init__.py
@@ -69,6 +69,7 @@
 SCHEMA[HashInfo.PARAM_SIZE] = int
 SCHEMA[HashInfo.PARAM_NFILES] = int
 SCHEMA[BaseOutput.PARAM_DESC] = str
+SCHEMA[BaseOutput.PARAM_ISEXEC] = bool
 
 
 def _get(
@@ -81,6 +82,7 @@ def _get(
     persist=False,
     checkpoint=False,
     desc=None,
+    isexec=False,
 ):
     parsed = urlparse(p)
 
@@ -97,6 +99,7 @@ def _get(
             persist=persist,
             checkpoint=checkpoint,
             desc=desc,
+            isexec=isexec,
         )
 
     for o in OUTS:
@@ -112,6 +115,7 @@ def _get(
                 persist=persist,
                 checkpoint=checkpoint,
                 desc=desc,
+                isexec=isexec,
             )
     return LocalOutput(
         stage,
@@ -124,6 +128,7 @@ def _get(
         persist=persist,
         checkpoint=checkpoint,
         desc=desc,
+        isexec=isexec,
     )
 
 
@@ -137,6 +142,7 @@ def loadd_from(stage, d_list):
         persist = d.pop(BaseOutput.PARAM_PERSIST, False)
         checkpoint = d.pop(BaseOutput.PARAM_CHECKPOINT, False)
         desc = d.pop(BaseOutput.PARAM_DESC, False)
+        isexec = d.pop(BaseOutput.PARAM_ISEXEC, False)
         ret.append(
             _get(
                 stage,
@@ -148,6 +154,7 @@ def loadd_from(stage, d_list):
                 persist=persist,
                 checkpoint=checkpoint,
                 desc=desc,
+                isexec=isexec,
             )
         )
     return ret
@@ -161,6 +168,7 @@ def loads_from(
     plot=False,
     persist=False,
     checkpoint=False,
+    isexec=False,
 ):
     return [
         _get(
@@ -172,6 +180,7 @@ def loads_from(
             plot=plot,
             persist=persist,
             checkpoint=checkpoint,
+            isexec=isexec,
         )
         for s in s_list
     ]
diff --git a/dvc/output/base.py b/dvc/output/base.py
@@ -76,6 +76,7 @@ class BaseOutput:
     PARAM_PLOT_HEADER = "header"
     PARAM_PERSIST = "persist"
     PARAM_DESC = "desc"
+    PARAM_ISEXEC = "isexec"
 
     METRIC_SCHEMA = Any(
         None,
@@ -105,6 +106,7 @@ def __init__(
         persist=False,
         checkpoint=False,
         desc=None,
+        isexec=False,
     ):
         self._validate_output_path(path, stage)
         # This output (and dependency) objects have too many paths/urls
@@ -136,6 +138,8 @@ def __init__(
         if self.use_cache and self.cache is None:
             raise RemoteCacheRequiredError(self.path_info)
 
+        self.isexec = False if self.IS_DEPENDENCY else isexec
+
     def _parse_path(self, tree, path):
         if tree:
             parsed = urlparse(path)
@@ -287,6 +291,11 @@ def save(self):
             return
 
         self.hash_info = self.get_hash()
+        self.isexec = self.isfile() and self.tree.isexec(self.path_info)
+
+    def set_exec(self):
+        if self.isfile() and self.isexec:
+            self.tree.set_exec(self.path_info)
 
     def commit(self):
         if not self.exists:
@@ -295,6 +304,7 @@ def commit(self):
         assert self.hash_info
         if self.use_cache:
             self.cache.save(self.path_info, self.cache.tree, self.hash_info)
+            self.set_exec()
 
     def dumpd(self):
         ret = copy(self.hash_info.to_dict())
@@ -328,6 +338,9 @@ def dumpd(self):
         if self.checkpoint:
             ret[self.PARAM_CHECKPOINT] = self.checkpoint
 
+        if self.isexec:
+            ret[self.PARAM_ISEXEC] = self.isexec
+
         return ret
 
     def verify_metric(self):
@@ -353,7 +366,7 @@ def checkout(
             return None
 
         try:
-            return self.cache.checkout(
+            res = self.cache.checkout(
                 self.path_info,
                 self.hash_info,
                 force=force,
@@ -366,6 +379,8 @@ def checkout(
             if allow_missing or self.checkpoint:
                 return None
             raise
+        self.set_exec()
+        return res
 
     def remove(self, ignore_remove=False):
         self.tree.remove(self.path_info)
diff --git a/dvc/schema.py b/dvc/schema.py
@@ -25,6 +25,7 @@
     Required("path"): str,
     HashInfo.PARAM_SIZE: int,
     HashInfo.PARAM_NFILES: int,
+    BaseOutput.PARAM_ISEXEC: bool,
 }
 LOCK_FILE_STAGE_SCHEMA = {
     Required(StageParams.PARAM_CMD): Any(str, list),
diff --git a/dvc/stage/loader.py b/dvc/stage/loader.py
@@ -61,6 +61,7 @@ def fill_from_lock(stage, lock_data=None):
             info = get_in(checksums, [key, path], {})
             info = info.copy()
             info.pop("path", None)
+            item.isexec = info.pop("isexec", None)
             item.hash_info = HashInfo.from_dict(info)
 
     @classmethod
diff --git a/dvc/stage/serialize.py b/dvc/stage/serialize.py
@@ -150,6 +150,9 @@ def _dumpd(item):
             *item.hash_info.to_dict().items(),
         ]
 
+        if item.isexec:
+            ret.append((item.PARAM_ISEXEC, True))
+
         return OrderedDict(ret)
 
     res = OrderedDict([("cmd", stage.cmd)])
diff --git a/dvc/stage/utils.py b/dvc/stage/utils.py
@@ -179,6 +179,7 @@ def compute_md5(stage):
             BaseOutput.PARAM_METRIC,
             BaseOutput.PARAM_PERSIST,
             BaseOutput.PARAM_CHECKPOINT,
+            BaseOutput.PARAM_ISEXEC,
             HashInfo.PARAM_SIZE,
             HashInfo.PARAM_NFILES,
         ],
diff --git a/dvc/tree/base.py b/dvc/tree/base.py
@@ -195,6 +195,15 @@ 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.
+        """
+        return False
+
     def iscopy(self, path_info):
         """Check if this file is an independent copy."""
         return False  # We can't be sure by default
diff --git a/dvc/tree/dvc.py b/dvc/tree/dvc.py
@@ -234,7 +234,7 @@ def isdvc(self, path, recursive=False, strict=True):
         recurse = recursive or not strict
         return meta.output_exists if recurse else meta.is_output
 
-    def isexec(self, path):  # pylint: disable=unused-argument
+    def isexec(self, path_info):  # pylint: disable=unused-argument
         return False
 
     def get_dir_hash(self, path_info, **kwargs):
diff --git a/dvc/tree/git.py b/dvc/tree/git.py
@@ -131,8 +131,12 @@ def walk(
                 )
             yield root, dirs, files
 
-    def isexec(self, path):
-        return is_exec(self.stat(path).st_mode)
+    def isexec(self, path_info):
+        if not self.exists(path_info):
+            return False
+
+        mode = self.stat(path_info).st_mode
+        return is_exec(mode)
 
     def stat(self, path):
         key = self._get_key(path)
diff --git a/dvc/tree/local.py b/dvc/tree/local.py
@@ -152,8 +152,12 @@ def remove(self, path_info):
     def makedirs(self, path_info):
         makedirs(path_info, exist_ok=True, mode=self.dir_mode)
 
-    def isexec(self, path):
-        mode = os.stat(path).st_mode
+    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)
 
     def stat(self, path):
diff --git a/dvc/tree/repo.py b/dvc/tree/repo.py
@@ -225,11 +225,11 @@ def isfile(self, path):  # pylint: disable=arguments-differ
             return False
         return meta.isfile
 
-    def isexec(self, path):
-        tree, dvc_tree = self._get_tree_pair(path)
-        if dvc_tree and dvc_tree.exists(path):
-            return dvc_tree.isexec(path)
-        return tree.isexec(path)
+    def isexec(self, path_info):
+        tree, dvc_tree = self._get_tree_pair(path_info)
+        if dvc_tree and dvc_tree.exists(path_info):
+            return dvc_tree.isexec(path_info)
+        return tree.isexec(path_info)
 
     def stat(self, path):
         tree, _ = self._get_tree_pair(path)
diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py
@@ -462,7 +462,7 @@ def parse_target(
 
 
 def is_exec(mode):
-    return mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
+    return bool(mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH))
 
 
 def glob_targets(targets, glob=True, recursive=True):
diff --git a/tests/func/test_add.py b/tests/func/test_add.py
@@ -2,6 +2,7 @@
 import logging
 import os
 import shutil
+import stat
 import textwrap
 import time
 
@@ -57,6 +58,26 @@ def test_add(tmp_dir, dvc):
     }
 
 
+@pytest.mark.skipif(os.name == "nt", reason="can't set exec bit on Windows")
+def test_add_executable(tmp_dir, dvc):
+    tmp_dir.gen("foo", "foo")
+    st = os.stat("foo")
+    os.chmod("foo", st.st_mode | stat.S_IEXEC)
+    dvc.add("foo")
+
+    assert load_yaml("foo.dvc") == {
+        "outs": [
+            {
+                "md5": "acbd18db4cc2f85cedef654fccc4a4d8",
+                "path": "foo",
+                "size": 3,
+                "isexec": True,
+            }
+        ],
+    }
+    assert os.stat("foo").st_mode & stat.S_IEXEC
+
+
 def test_add_unicode(tmp_dir, dvc):
     with open("\xe1", "wb") as fd:
         fd.write(b"something")
diff --git a/tests/func/test_checkout.py b/tests/func/test_checkout.py
@@ -3,6 +3,7 @@
 import logging
 import os
 import shutil
+import stat
 
 import pytest
 from mock import patch
@@ -862,3 +863,20 @@ def test_checkout_external_modified_file(tmp_dir, dvc, scm, mocker, workspace):
     dvc.checkout()
 
     assert (workspace / "foo").read_text() == "foo"
+
+
+def test_checkout_executable(tmp_dir, dvc):
+    tmp_dir.dvc_gen("foo", "foo")
+
+    contents = load_yaml("foo.dvc")
+    contents["outs"][0]["isexec"] = True
+    dump_yaml("foo.dvc", contents)
+
+    dvc.checkout("foo")
+
+    isexec = os.stat("foo").st_mode & stat.S_IEXEC
+    if os.name == "nt":
+        # NOTE: you can't set exec bits on Windows
+        assert not isexec
+    else:
+        assert isexec
diff --git a/tests/unit/stage/test_loader_pipeline_file.py b/tests/unit/stage/test_loader_pipeline_file.py
@@ -39,6 +39,24 @@ def test_fill_from_lock_deps_outs(dvc, lock_data):
     assert stage.outs[0].hash_info == HashInfo("md5", "bar_checksum")
 
 
+def test_fill_from_lock_outs_isexec(dvc):
+    stage = create_stage(PipelineStage, dvc, PIPELINE_FILE, outs=["foo"])
+
+    assert not stage.outs[0].isexec
+
+    StageLoader.fill_from_lock(
+        stage,
+        {
+            "cmd": "command",
+            "outs": [{"path": "foo", "md5": "foo_checksum", "isexec": True}],
+        },
+    )
+
+    assert stage.outs[0].def_path == "foo"
+    assert stage.outs[0].hash_info == HashInfo("md5", "foo_checksum")
+    assert stage.outs[0].isexec
+
+
 def test_fill_from_lock_params(dvc, lock_data):
     stage = create_stage(
         PipelineStage,
diff --git a/tests/unit/stage/test_serialize_pipeline_lock.py b/tests/unit/stage/test_serialize_pipeline_lock.py
@@ -132,6 +132,30 @@ def test_lock_outs(dvc, typ):
     )
 
 
+@pytest.mark.parametrize("typ", ["plots", "metrics", "outs"])
+def test_lock_outs_isexec(dvc, typ):
+    stage = create_stage(PipelineStage, dvc, **{typ: ["input"]}, **kwargs)
+    stage.outs[0].hash_info = HashInfo("md5", "md-five")
+    stage.outs[0].isexec = True
+    assert to_single_stage_lockfile(stage) == OrderedDict(
+        [
+            ("cmd", "command"),
+            (
+                "outs",
+                [
+                    OrderedDict(
+                        [
+                            ("path", "input"),
+                            ("md5", "md-five"),
+                            ("isexec", True),
+                        ]
+                    )
+                ],
+            ),
+        ]
+    )
+
+
 @pytest.mark.parametrize("typ", ["plots", "metrics", "outs"])
 def test_lock_outs_order(dvc, typ):
     stage = create_stage(

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@`
`25`	`25`	`Required("path"): str,`
`26`	`26`	`HashInfo.PARAM_SIZE: int,`
`27`	`27`	`HashInfo.PARAM_NFILES: int,`
	`28`	`+ BaseOutput.PARAM_ISEXEC: bool,`
`28`	`29`	`}`
`29`	`30`	`LOCK_FILE_STAGE_SCHEMA = {`
`30`	`31`	`Required(StageParams.PARAM_CMD): Any(str, list),`
Original file line number	Diff line number	Diff line change
`@@ -150,6 +150,9 @@ def _dumpd(item):`
`150`	`150`	`*item.hash_info.to_dict().items(),`
`151`	`151`	`]`
`152`	`152`
	`153`	`+ if item.isexec:`
	`154`	`+ ret.append((item.PARAM_ISEXEC, True))`
	`155`	`+`
`153`	`156`	`return OrderedDict(ret)`
`154`	`157`
`155`	`158`	`res = OrderedDict([("cmd", stage.cmd)])`