dvc: preserve exec bit for tracked files by dudarev · Pull Request #5061 · treeverse/dvc

dudarev · 2020-12-08T20:13:54Z

For ticket #4578

Based on the discussion in #5036 I've added isexec parameter.

The plan for this PR:

add test_add_executable with an executable file
make all tests pass
figure out how to test mode change on checkout
implement chmod on checkout
fix all tests if they will be broken on the previous step
~~disable this feature on Windows and create a separate ticket to enable it there~~

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

treeverse/dvc.org#2039

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

efiop · 2020-12-08T21:45:06Z

+        self.is_user_executable = os.path.isfile(path) and os.access(
+            path, os.X_OK
+        )


The file might not exist here or might not be in the same state as the rest of the info. In this place we should load the isexec from the info provided, e.g. how metric is loaded above.

The actual stat should happen in save() method, as pointed out in #5036 (comment) .

Thank you for the clarification. I moved stat to save().

efiop · 2020-12-08T21:48:09Z

+        ret[self.PARAM_IS_USER_EXECUTABLE] = self.is_user_executable
+


same as in other fields above

Suggested change

ret[self.PARAM_IS_USER_EXECUTABLE] = self.is_user_executable

if self.is_user_executable:

ret[self.PARAM_IS_USER_EXECUTABLE] = self.is_user_executable

Yes, I was leaving it outside of if to have the test that I had at that stage pass, put it behind if now.

efiop · 2020-12-08T22:01:29Z

    PARAM_PLOT_HEADER = "header"
    PARAM_PERSIST = "persist"
    PARAM_DESC = "desc"
+    PARAM_IS_USER_EXECUTABLE = "is_user_executable"


I guess we could even use exec: true/false, as it is specified for a particular output and it doesn't seem like we will have another use for exec here in the future. Could also use isexec. The current is_user_executable seems a bit long. WDYT?

Also, is is very much unnecessary. exec or executable is fine imo.

@efiop @skshetry good suggestion about a shorter variable name. It fits the style of this project well. I hesitated a lot, probably too much, between exec and isexec and decided to go with exec. Unfortunately, as a result, I've got a pylint error:

W0622: Redefining built-in 'exec' (redefined-builtin)

so I've started using isexec instead. Please let me know if you have a stronger opinion about using executable instead.

good suggestion about a shorter variable name.

The suggestion was regarding the keyword in the dvc.yaml/dvcfile, not the variable name. I have no strong opinion towards either exec or isexec, but the decision should not be based on that pylint complained.

I think that isexec seems better than exec as the latter sounds like "execute this output: true" which is confusing.

efiop

Thanks for the PR! The plan looks pretty good 🙂

Also fixed all tests in test_add.py

dudarev · 2020-12-13T19:40:29Z

 class HashInfo:
    PARAM_SIZE = "size"
    PARAM_NFILES = "nfiles"
+    PARAMS_TO_IGNORE = ("isexec",)


@efiop I'm not sure if this is a good approach and there could be a better way to ignore isexec from .dvc file in HashInfo. I plan to look at a better way to handle it later but maybe you have some quick suggestions?

@dudarev Take a look at loadd_from in dvc/output/__init__.py. You just need to pop the value for isexec from info, same as we do with things like desc.

Thanks! I've modified the code to pop isexec in loadd_from.

efiop · 2020-12-18T06:42:51Z

+        self.isexec = os.path.isfile(self.def_path) and os.access(
+            self.def_path, os.X_OK
+        )


Suggested change

self.isexec = os.path.isfile(self.def_path) and os.access(

self.def_path, os.X_OK

)

self.isexec = self.tree.isfile(self.path_info) and self.tree.isexec(self.path_info)

Good point! I've done this and also updated utils:is_exec function to return boolean by casting to bool:

https://github.com/iterative/dvc/blob/167ab79b8029d3272e6aa056c12305b6c9befeee/dvc/utils/__init__.py#L463

efiop · 2020-12-18T06:44:40Z

            metric="metrics" in key,
            plot="plots" in key,
            checkpoint="checkpoints" in key,
+            isexec="isexec" in key,


isexec can't be set through CLI

Suggested change

isexec="isexec" in key,

efiop · 2020-12-18T06:46:49Z



+def test_add_executable(tmp_dir, dvc):
+    tmp_dir.dvc_gen_exec({"foo": "foo"})


This is the only place where dvc_gen_exec is used, better to just inline it here

I inlined dvc_gen_exec code here.

efiop · 2020-12-18T06:48:46Z

+    def setUp(self):
+        super().setUp()
+
+        self.create_executable(self.EXECUTABLE, self.EXECUTABLE_CONTENT)


The only place where create_executable is used, better to inline it here too :)

Btw, we are migrating from unittest to pytest tests, so it would be great to re-write this one with pytest too.

Inlined created_executable here too.

efiop · 2020-12-18T06:59:05Z

+        if isexec:
+            st = os.stat(path_info.fspath)
+            os.chmod(path_info.fspath, st.st_mode | stat.S_IEXEC)
+


IIRC chmod won't modify the mtime(we use it to detect changes in the workspace), so there is nothing cache should worry about when doing chmod +x. The chmod part is more related to the output itself, than to the cache (notice how we are just passing through the isexec, but don't really use anything cache-specific). That's why the saving of isexec is not in cache.save but is actually in output.save. So let's move this to OutputBase.checkout, right after cache.checkout call. Also note that it is better to use tree.* methods instead of os.*, as that helps to keep the abstraction for different types of outputs 🙂

Good point! I moved this code to BaseOutput.checkout and used tree.* methods. There is one minor stylistic issue: all methods in BaseTree are using path_info as an argument, I've added BaseTree.isexec(path) since it was not there and all children classes use path argument in that method:

https://github.com/iterative/dvc/blob/2ade7f7ba56c1b76924f9b5b7f379225b89e1914/dvc/tree/base.py#L198

I modified isexec to use path_info as an argument

efiop

Also need to add PARAM_ISEXEC to https://github.com/iterative/dvc/blob/76c6c5427816b25a81d90f86d3af1cb19779802f/dvc/stage/utils.py#L178 , so it is not taking it into account when computing stage hash. This combined with os.access(X_OK) returning True for non-exec files, is resulting in windows failures, so we can't really skip those tests.

dudarev · 2020-12-19T20:14:38Z

test_add_executable fails on Windows, because isexec is not set on Windows. I need to investigate:

https://github.com/iterative/dvc/blob/2ade7f7ba56c1b76924f9b5b7f379225b89e1914/tests/func/test_add.py#L70

E         {'outs': [{'md5': 'acbd18db4cc2f85cedef654fccc4a4d8', 'path': 'foo', 'size': 3}]} != {'outs': [{'isexec': True, 'md5': 'acbd18db4cc2f85cedef654fccc4a4d8', 'path': 'foo', 'size': 3}]}

efiop · 2020-12-19T22:09:41Z

        """
        return True

+    def set_exec(self, path_info):


Note for us, for the future: time to finally move BaseTree to ABC...

Per treeverse/dvc#5061

efiop

Thank you! 🙏

Per treeverse/dvc#5061

efiop · 2020-12-20T00:30:28Z

One more thing would be to use this in dvc list, but that could be added later.

jorgeorpinel · 2020-12-20T22:51:53Z

Will there be a WARN msg or something on Windows? Does it even matter that much? (I think you can usually try to execute any file on Win, it's more about the file extension).

efiop · 2020-12-20T22:57:51Z

@jorgeorpinel No warning. It is a minor feature that is safe to ignore on Windows. Same thing with git, so shouldn't be a big suprise for anyone, but we'll see if someone complains.

jorgeorpinel · 2020-12-20T23:51:12Z

K thanks. I was just double checking (for docs) as I read about that warning somewhere (in the OP I think).

* docs: add isexec field description Per treeverse/dvc#5061 * Update content/docs/user-guide/dvc-files-and-directories.md Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com> * Update content/docs/user-guide/dvc-files-and-directories.md * Update content/docs/user-guide/dvc-files-and-directories.md Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

treeverse#4578 added is_user_executable to dvc file

3d7bc3e

dudarev changed the title ~~#4578 added is_user_executable to dvc file~~ [WIP] #4578 added is_user_executable to dvc file Dec 8, 2020

efiop reviewed Dec 8, 2020

View reviewed changes

dudarev added 2 commits December 11, 2020 21:54

Addressed PR feedback and introduced test_add_executable

cc3d8aa

Also fixed all tests in test_add.py

Added a test to checkout executable file

ffad092

dudarev commented Dec 13, 2020

View reviewed changes

dudarev added 2 commits December 17, 2020 15:05

Pop isexec in dvc/output/__init__.py:loadd_from

76667d6

Implement test that file is executable after checkout

32d8148

efiop reviewed Dec 18, 2020

View reviewed changes

dudarev added 4 commits December 19, 2020 19:54

Address PR feedback

167ab79

Address PR feedback

2ade7f7

Changed BaseTree.isexec to use path_info as an argument

5419567

Merged master

8833678

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread tests/func/test_add.py Outdated

Update tests/func/test_add.py

73aba85

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread tests/func/test_add.py Outdated

Update tests/func/test_add.py

612706f

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread tests/func/test_add.py Outdated

Update tests/func/test_add.py

9d64258

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread tests/func/test_add.py

Update tests/func/test_add.py

c346f83

tests: fix windows cases

950ac14

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread dvc/cache/base.py Outdated

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread dvc/output/base.py Outdated

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread dvc/output/base.py Outdated

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread dvc/tree/local.py Outdated

Ruslan Kuprieiev and others added 5 commits December 20, 2020 00:21

tests: add: fix exec test on windows

3fe6241

Update dvc/tree/local.py

c7ca0ea

Update dvc/output/base.py

c9479d0

Update dvc/output/base.py

cc84975

Update dvc/cache/base.py

71573ec

efiop reviewed Dec 19, 2020

View reviewed changes

Comment thread tests/func/test_add.py

efiop and others added 2 commits December 20, 2020 00:52

Update tests/func/test_add.py

11200a8

tests: add: skip whole exec test on windows

5652889

efiop changed the title ~~[WIP] dvc: preserve exec bit for tracked files~~ dvc: preserve exec bit for tracked files Dec 19, 2020

efiop changed the title ~~dvc: preserve exec bit for tracked files~~ [WIP] dvc: preserve exec bit for tracked files Dec 19, 2020

Ruslan Kuprieiev added 2 commits December 20, 2020 01:52

save isexec to lockfiles

483252a

fix formatting

0a37c40

efiop added a commit to treeverse/dvc.org that referenced this pull request Dec 20, 2020

docs: add isexec field description

8f627ec

Per treeverse/dvc#5061

efiop mentioned this pull request Dec 20, 2020

[2.0] dvc.yaml: doc isexec field treeverse/dvc.org#2039

Merged

efiop approved these changes Dec 20, 2020

View reviewed changes

efiop added a commit to treeverse/dvc.org that referenced this pull request Dec 20, 2020

docs: add isexec field description

3155e31

Per treeverse/dvc#5061

efiop added a commit to treeverse/dvc.org that referenced this pull request Dec 20, 2020

docs: add isexec field description

c9fb219

Per treeverse/dvc#5061

efiop changed the title ~~[WIP] dvc: preserve exec bit for tracked files~~ dvc: preserve exec bit for tracked files Dec 20, 2020

efiop merged commit dcea83e into treeverse:master Dec 20, 2020

		ret[self.PARAM_IS_USER_EXECUTABLE] = self.is_user_executable



		def test_add_executable(tmp_dir, dvc):
		tmp_dir.dvc_gen_exec({"foo": "foo"})

Conversation

dudarev commented Dec 8, 2020 • edited by efiop Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dudarev commented Dec 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

efiop commented Dec 20, 2020

Uh oh!

jorgeorpinel commented Dec 20, 2020

Uh oh!

efiop commented Dec 20, 2020

Uh oh!

jorgeorpinel commented Dec 20, 2020

dudarev commented Dec 8, 2020 •

edited by efiop

Loading

efiop left a comment •

edited

Loading