Request cloning using COPYFILE_CLONE #232
Conversation
| @@ -33,7 +33,8 @@ bool CopyFile(const std::string &src, const std::string &dest) { | |||
| #ifdef __APPLE__ | |||
| // The `copyfile` function with `COPYFILE_ALL` mode preserves permissions and | |||
| // modification time. | |||
| return copyfile(src.c_str(), dest.c_str(), nullptr, COPYFILE_ALL) == 0; | |||
| return copyfile(src.c_str(), dest.c_str(), nullptr, | |||
| COPYFILE_ALL | COPYFILE_CLONE) == 0; | |||
There was a problem hiding this comment.
Does this need to be gated on an OS version?
There was a problem hiding this comment.
¯\_(ツ)_/¯ Nothing in copyfile.h mentions versions, there are no uses of availability attributes.
There was a problem hiding this comment.
We could add a #ifdef COPYFILE_CLONE to be extra safe.
There was a problem hiding this comment.
I don't have a machine on any older macOS version than Mojave but we don't expect it to be on any older version since it's related to APFS do we?
There was a problem hiding this comment.
For what it's worth, LLVM supports this and checks that it's on macOS 10.12 or greater: https://reviews.llvm.org/rL360174
There was a problem hiding this comment.
Does the worker support 10.11 or earlier?
There was a problem hiding this comment.
And to finish my thought, I don't know if we care about anyone running such an old version of macOS. I'd say leave it unconditional, and if someone hits a bug because of it, we can patch it then.
There was a problem hiding this comment.
How did we both manage to wait an hour to reply at exactly the same time?
There was a problem hiding this comment.
maybe it was lunch 😋
|
Tested with our build, checked with |
|
I initially had some concerns about the clone behavior of "fails if the file already exists", but as long as we can assume that Bazel does the appropriate cleanup of the copy destination (whether sandboxed or not), this should be safe. |
|
Should we add a fallback second call to |
I'm not that worried about it right now. I've also already started the internal submit that will merge this 😛 |
…yfile_clone PiperOrigin-RevId: 248588535
|
Thanks |
…-using-copyfile_clone PiperOrigin-RevId: 248588535
This should be a safe simple addition. From man
copyfile(3):