Skip to content

[WIP] save permissions in dvc files #5036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[WIP] save permissions in dvc files #5036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions dvc/hash_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@
class HashInfo:
PARAM_SIZE = "size"
PARAM_NFILES = "nfiles"
PARAM_MODE = "mode"

name: Optional[str]
value: Optional[str]
dir_info: Optional[DirInfo] = field(default=None, compare=False)
size: Optional[int] = field(default=None, compare=False)
nfiles: Optional[int] = field(default=None, compare=False)
mode: Optional[str] = field(default=None, compare=False)

def __bool__(self):
return bool(self.value)
Expand All @@ -43,6 +45,9 @@ def to_dict(self):
ret[self.PARAM_SIZE] = self.size
if self.nfiles is not None:
ret[self.PARAM_NFILES] = self.nfiles
if self.mode is not None:
ret[self.PARAM_MODE] = self.mode

return ret

@property
Expand Down
2 changes: 2 additions & 0 deletions dvc/stage/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ def __init__(
dvcfile=None,
desc=None,
meta=None,
mode=None,
):
if deps is None:
deps = []
Expand All @@ -154,6 +155,7 @@ def __init__(
self._dvcfile = dvcfile
self.desc = desc
self.meta = meta
self.mode = mode
self.raw_data = RawData()

@property
Expand Down
1 change: 1 addition & 0 deletions dvc/stage/params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ class StageParams:
PARAM_METRICS = "metrics"
PARAM_PLOTS = "plots"
PARAM_DESC = "desc"
PARAM_MODE = "mode"
1 change: 1 addition & 0 deletions dvc/stage/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ def get_dump(stage):
stage.PARAM_OUTS: [o.dumpd() for o in stage.outs],
stage.PARAM_ALWAYS_CHANGED: stage.always_changed,
stage.PARAM_META: stage.meta,
stage.PARAM_MODE: stage.mode,
}.items()
if value
}
Expand Down
5 changes: 4 additions & 1 deletion dvc/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,10 @@ def get(self, path_info):
return None

self._update_state_record_timestamp_for_inode(actual_inode)
return HashInfo("md5", value, size=int(actual_size))

mode = oct(os.stat(path).st_mode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is full mode intentional? Or did you only mean to include permission bits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning now more towards only saving user executable bit, so we could have a boolean parameter:

is_user_executable: false/true


return HashInfo("md5", value, size=int(actual_size), mode=mode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode is not really used by state, so we shouldn't include it here (we can probably do that in Output.save with self.tree.stat(self.path_info).st_mode. Also mode is not really related to HashInfo so it shoudn't be.a part of it, unless there are good reasons to include it. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense not to include mode as part of hash. Should content of output file looks like:

outs:
- md5: acbd18db4cc2f85cedef654fccc4a4d8
  size: 3
  path: foo
- is_user_executable: false

So there will be two separate outputs. Will it affect anything that uses those files?

Could you provide a link to a place where outs are set in the code? I'm having trouble finding it quickly.

Copy link
Contributor

@efiop efiop Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dudarev It should be more like:

outs:
- md5: acbd18db4cc2f85cedef654fccc4a4d8
  size: 3
  path: foo
  is_user_executable: false

Since that relates to the same output. Maybe you are confused about the way that entry is generated, see

dvc/dvc/output/base.py

Line 299 in f09c27e

def dumpd(self):
. We could just add ret[self.PARAM_ISEXEC] = self.isexec there and parse it on the roundtrip similar to other fields.

Could you provide a link to a place where outs are set in the code? I'm having trouble finding it quickly.

Sure!

dvc/dvc/output/base.py

Line 289 in f09c27e

self.hash_info = self.get_hash()


def save_link(self, path_info):
"""Adds the specified path to the list of links created by dvc. This
Expand Down
3 changes: 3 additions & 0 deletions tests/func/test_add.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def test_add(tmp_dir, dvc):

assert stage is not None

mode = oct(os.stat(stage.path).st_mode)

assert isinstance(stage, Stage)
assert os.path.isfile(stage.path)
assert len(stage.outs) == 1
Expand All @@ -52,6 +54,7 @@ def test_add(tmp_dir, dvc):
"md5": "acbd18db4cc2f85cedef654fccc4a4d8",
"path": "foo",
"size": 3,
"mode": mode
}
],
}
Expand Down