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

Use stat to retain file permissions when copying files #259

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

lamcw
Copy link
Contributor

@lamcw lamcw commented Aug 12, 2022

With stat instead of chmod, this should improve the performance of copying many files with cp as chmod runs multiple syscalls when changing permissions.

Would be great if I can get your help to test these changes on Linux as well!

This is an alternative solution to #258.

Closes #256.

With stat instead of chmod, this should improve the performance of
copying many files with `cp` as chmod runs multiple syscalls when
changing permissions.

Signed-off-by: Thomas Lam <[email protected]>
@lamcw lamcw changed the title Use stat to retain file permissions Use stat to retain file permissions when copying files Aug 12, 2022
@benradf
Copy link
Member

benradf commented Aug 12, 2022

I gave it a try on Linux and it seems to work fine 👍

One thing that I think is okay but wanted to explicitly call out – the file copies will not retain exactly the same permissions as the originals now that we're no longer using chmod --reference. In practice I don't think this will matter though.

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you @lamcw for contributing this!
And thanks @benradf for looking into this and the alternative approach!

LGTM

Also pinging @guibou who had the original use-case for preserving the executable bit. Please let us know if this change causes any issues for your use-case.

@aherrmann aherrmann added the merge-queue merge on green CI label Aug 12, 2022
@mergify mergify bot merged commit d725aee into tweag:master Aug 12, 2022
@mergify mergify bot removed the merge-queue merge on green CI label Aug 12, 2022
@lamcw lamcw deleted the retain-mode-with-stat branch August 13, 2022 01:46
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.

Slow nixpkgs_package rule on macOS
3 participants