Skip to content
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.

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

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

Merged
merged 4 commits into from
Dec 21, 2020
Merged

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

merged 4 commits into from
Dec 21, 2020

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Dec 20, 2020

Per iterative/dvc#5061

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

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

@shcheklein shcheklein temporarily deployed to dvc-landing-efiop-patch-xxttjc December 20, 2020 00:18 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-efiop-patch-xxttjc December 20, 2020 00:26 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-efiop-patch-xxttjc December 20, 2020 00:29 Inactive
@efiop
Copy link
Contributor Author

efiop commented Dec 20, 2020

For the record: this is indeed for 2.0, but we have some plans to create a new dvc.lock schema for 2.0 too, so the location of isexec might adjust a bit, but that doesn't really affect this PR.

Also, not sure how deep to go explaining isexec and posix acl behind it.

@jorgeorpinel
Copy link
Contributor

Note: I'm repurposing the migration label for now to mark 2.0 PRs and help me manage the v1 release branch vs master as soon as the first 2.0 PR gets merged.

@jorgeorpinel
Copy link
Contributor

not sure how deep to go explaining isexec and posix acl

I see no need to go into much details at all. Will leave a suggestion with generic language ⌛

Comment on lines 76 to 78
- `isexec`: If a file, whether or not owner has execute permission. On Windows,
it is not saved and, if set manually, won't have any effect on `dvc checkout`
and `dvc pull`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `isexec`: If a file, whether or not owner has execute permission. On Windows,
it is not saved and, if set manually, won't have any effect on `dvc checkout`
and `dvc pull`.
- `isexec`: Whether this is an executable file. DVC preserves execute
permissions upon `dvc checkout` and `dvc pull`. This has no effect on
directories, or in general on Windows.

(same for deps)

Copy link
Contributor

Choose a reason for hiding this comment

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

Questions:

  • Do we want to specify "preserves POSIX execute permissions"? (possibly linked)
    TBH I see no need since we say it doesn't work on Windows so it's easy to infer why.
  • "upon dvc checkout and dvc pull" - are these the only commands it affects? What about add or commit (when tracking stuff which moves it to the cache and then links back @efiop?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to specify "preserves POSIX execute permissions"? (possibly linked)
TBH I see no need since we say it doesn't work on Windows so it's easy to infer why.

Thought about it too, but I feel it is generally understood as is, so I wouldn't clarify it more.

"upon dvc checkout and dvc pull" - are these the only commands it affects? What about add or commit (when tracking stuff which moves it to the cache and then links back @efiop?

Pretty much yes. add/commit take existing file and add/commit it, so on windows there is nothing to capture - no exec bit to set. checkout/pull is when the exec bit is coming from another machine (e.g. your collegue on linux) or set by hand, and thats where it is better to clarify it.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 20, 2020

Choose a reason for hiding this comment

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

Hmm. I just tried this on Ubuntu (WSL) and DVC (1.11.8) removes the exec bit upon add. No reflinks on my system though so it's a copy, but the cached file doesn't have exec on either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel 1.11.8 doesn't have that patch yet and it is expected for it to not preserve exc. You need to try upstream dvc.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Dec 20, 2020

Choose a reason for hiding this comment

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

Sure but my question was: since add does remove the exec bit, will the exec field affect add after all? Thinking about it, that doesn't make sense as there is no .dvc file prior to add/commit... so I'm re-approving this.

But (unrelated to this PR) should there perhaps be a new --isexec flag on those or other commands (run/repro?) to do this? Moved to iterative/dvc#4578 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel Do you mean a flag to set isexec value through CLI? Not really, there is no need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see you answered here. OK

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@shcheklein shcheklein temporarily deployed to dvc-landing-efiop-patch-xxttjc December 20, 2020 23:17 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-efiop-patch-xxttjc December 20, 2020 23:25 Inactive
jorgeorpinel
jorgeorpinel previously approved these changes Dec 20, 2020
@@ -92,6 +95,9 @@ A _dependency entry_ (`deps`) can have these fields:
_checksum_ for HDFS and WebHDFS. See `dvc import-url` for more information.
- `size`: Size of the file or directory (sum of all files).
- `nfiles`: If a directory, number of files inside.
- `isexec`: Whether this is an executable file. DVC preserves execute
Copy link
Member

Choose a reason for hiding this comment

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

hmm, sorry guys .. I'm way too late (and approving this PR) ... just to clarify - what does it mean for deps. I though pull/checkout do now do anything to deps?

How would it work if out say exec:True and dep says exec:False?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this was not meant to be here. Deleting.

@shcheklein
Copy link
Member

btw, @jorgeorpinel migration label looks strange, we are not migrating anything as far as I know - we are releasing a new version.

@jorgeorpinel jorgeorpinel changed the title docs: add isexec field description [2.0] docs: add isexec field description Dec 21, 2020
@jorgeorpinel jorgeorpinel changed the title [2.0] docs: add isexec field description [2.0] dvc.yaml: doc isexec field Dec 21, 2020
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 21, 2020

True. OK I made a new label and changed the title.

Let's not merge this yet? (because it won't be released until Jan per iterative/dvc#4578 (comment))

@shcheklein shcheklein temporarily deployed to dvc-landing-efiop-patch-xxttjc December 21, 2020 10:18 Inactive
@shcheklein shcheklein merged commit 6d37d00 into master Dec 21, 2020
@efiop efiop deleted the efiop-patch-2 branch December 21, 2020 21:59
@jorgeorpinel
Copy link
Contributor

@shcheklein This won't be released until Jan. Is it OK to merge these? I added the 2.0 label in part to delay merging some of these PRs at least for some time.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 21, 2020

UPDATE: I see your #2031 (comment) that we should start tracking and backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants