-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
use uv_fs_copyfile for cp #59662
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
base: master
Are you sure you want to change the base?
use uv_fs_copyfile for cp #59662
Conversation
|
The failing tests are due to the mode not respecting umask, as noted above. @vtjnash, any advice on why libuv might be ignoring the umask, here? |
|
It is implemented to copy permissions, not the umask: libuv/libuv#4396 |
|
So should our Or should I submit a PR to libuv to add a flag to have copyfile to not copy ownership and permissions? (But it seems like this could introduce an inconsistency with Windows, according to the issue linked below.) @vtjnash filed libuv/libuv#3125 requesting that libuv copy over the permissions and ownership to match |
|
IIRC, we don't care, but want to ensure it is consistent across platforms whichever way it goes so it isn't a weird gotcha when testing different platforms |
|
IMO we should generally follow whatever coreutils does, so that our cp() is similar to their cp. |
Our existing behavior of applying the
whereas I believe we do not do this (in |
|
I think it's fine to be similar to |
|
Okay, I've changed the docstring and test to document that it acts like This doesn't seem like a breaking change since the previous behavior was undocumented. |
|
CI failure seems to be an unrelated |
|
hi all, floswald@PTL11077 ~/g/julia (sgj/uv_fs_copyfile)> ./julia
_
_ _ _(_)_ | Documentation: https://docs.julialang.org
(_) | (_) (_) |
_ _ _| |_ __ _ | Type "?" for help, "]?" for Pkg help.
| | | | | | |/ _` | |
| | |_| | | | (_| | | Version 1.13.0-DEV.1204 (2025-09-30)
_/ |\__'_|_|_|\__'_| | sgj/uv_fs_copyfile/fc4d77672f (fork: 4 commits, 33 days)
|__/ |
julia> versioninfo()
Julia Version 1.13.0-DEV.1204
Commit fc4d77672f (2025-09-30 05:27 UTC)
Platform Info:
OS: macOS (arm64-apple-darwin24.6.0)
CPU: 10 × Apple M1 Pro
WORD_SIZE: 64
LLVM: libLLVM-20.1.8 (ORCJIT, apple-m1)
GC: Built with stock GC
Threads: 1 default, 1 interactive, 1 GC (on 8 virtual cores)
julia> cp("/Users/floswald/Dropbox (Personal)/Apps/JPE-packages/JPE/Green-20220767/2/replication-package/Ben Sand - Union_Rep_Package.zip", "/Users/floswald/Downloads/test.zip")
"/Users/floswald/Downloads/test.zip"
julia> exit()
floswald@PTL11077 ~/g/julia (sgj/uv_fs_copyfile)> julia
The latest version of Julia in the `release` channel is 1.12.1+0.aarch64.apple.darwin14. You currently have `1.11.4+0.aarch64.apple.darwin14` installed. Run:
juliaup update
to install Julia 1.12.1+0.aarch64.apple.darwin14 and update the `release` channel to that version.
_
_ _ _(_)_ | Documentation: https://docs.julialang.org
(_) | (_) (_) |
_ _ _| |_ __ _ | Type "?" for help, "]?" for Pkg help.
| | | | | | |/ _` | |
| | |_| | | | (_| | | Version 1.11.4 (2025-03-10)
_/ |\__'_|_|_|\__'_| | Official https://julialang.org/ release
|__/ |
julia> cp("/Users/floswald/Dropbox (Personal)/Apps/JPE-packages/JPE/Green-20220767/2/replication-package/Ben Sand - Union_Rep_Package.zip", "/Users/floswald/Downloads/test.zip", force = true)
ERROR: IOError: sendfile: Unknown system error -184335295 (Unknown system error -184335295)
Stacktrace:
[1] uv_error
@ ./libuv.jl:106 [inlined]
[2] sendfile(dst::Base.Filesystem.File, src::Base.Filesystem.File, src_offset::Int64, bytes::Int64)
@ Base.Filesystem ./filesystem.jl:224
[3] sendfile(src::String, dst::String)
@ Base.Filesystem ./file.jl:1131
[4] cp(src::String, dst::String; force::Bool, follow_symlinks::Bool)
@ Base.Filesystem ./file.jl:386
[5] top-level scope
@ REPL[2]:1
julia>
floswald@PTL11077 ~/g/julia (sgj/uv_fs_copyfile)> du -h /Users/floswald/Downloads/test.zip
3.8G /Users/floswald/Downloads/test.zip |
|
CI failure looks unrelated: |
This PR changes
cp, and other uses of the internal functionBase.sendfile(srcpath, destpath), to use theuv_fs_copyfilefunction.This should be faster and more reliable, hopefully — on many filesystems,
uv_fs_copyfilecan just create a copy-on-write link. Hopefully, this should fix longstanding problems withcpof large files: fixes #56537 (macos), fixes #39868 (linux), fixes #30723.In particular, I noticed two problems with our previous
sendfileimplementation, which called a lower-levelsendfilefunction on the file descriptors that in turn calleduv_fs_sendfile. First,uv_fs_sendfiletakes aCsize_targument for the number of bytes, which is clearly too small on 32-bit systems. Second, it assumes that the return value ofuv_fs_sendfileis the number of bytes written, but the return value is aCint, which is clearly too small for the number of bytes in a large file (> 2GiB). The PR therefore fixed theuv_fs_sendfilecall to pass at mosttypemax(Cssize_t)(which is the maximum value on a 32-bit Unix system, since the underlying libcsendfilereturns anssize_t) in a single call (writing larger files in chunks), and second to only use the return value as an error code — ifuv_fs_sendfilesucceeds, it looks like we can assume that it wrote the requested number of bytes.In principle, we could delete this lower-level
Base.sendfilemethod completely, since it is undocumented and we don't use it. I couldn't find any external packages that use it either. But I left it in for now, to be conservative.I'm still seeing a test failure with this PR related to file mode: on my macos system, it seems to be ignoring the
umaskfor the permissions of the copied file? I'm not sure why — libuv'scopyfileimplementation on Unix should callopenwith the mode of the source file, which should mask out theumask, no? See also #27295.cc @vtjnash, who suggested using
uv_fs_copyfilein #27295 (review).