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

Fix calls to realpath for shared network drives #34132

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

corakingdon
Copy link
Contributor

see #33956 for discussion

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let us know when you've tested with the CI-built binary.

@fredrikekre
Copy link
Member

Answering #33956 (comment) here:

Thanks @fredrikekre! where on the PR can I find the option to download?

https://s3.amazonaws.com/julialangnightlies/assert_pretesting/winnt/x64/1.4/julia-fe14bfcf42-win64.exe

@corakingdon
Copy link
Contributor Author

Thank you! for future reference though, where did that link come from?

@KristofferC
Copy link
Member

KristofferC commented Dec 18, 2019

Click Details on the buildbot/tester_win64 (https://build.julialang.org/#/builders/32/builds/6845)

Then go to the Build properties tab and look at

download_url | "https://s3.amazonaws.com/julialangnightlies/assert_pretesting/winnt/x64/1.4/julia-fe14bfcf42-win64.exe"

@corakingdon
Copy link
Contributor Author

@StefanKarpinski in combination with the changes made to Pkg, this seems to work fully on my network drive! I'm wondering though if this is a strange work around, and if it would be better to change the realpath definition instead, since it's a julia function? thoughts?

julia/base/path.jl

Lines 409 to 434 in 8707744

"""
realpath(path::AbstractString) -> String
Canonicalize a path by expanding symbolic links and removing "." and ".." entries.
On case-insensitive case-preserving filesystems (typically Mac and Windows), the
filesystem's stored case for the path is returned.
(This function throws an exception if `path` does not exist in the filesystem.)
"""
function realpath(path::AbstractString)
req = Libc.malloc(_sizeof_uv_fs)
try
ret = ccall(:uv_fs_realpath, Cint,
(Ptr{Cvoid}, Ptr{Cvoid}, Cstring, Ptr{Cvoid}),
C_NULL, req, path, C_NULL)
if ret < 0
ccall(:uv_fs_req_cleanup, Cvoid, (Ptr{Cvoid},), req)
uv_error("realpath", ret)
end
path = unsafe_string(ccall(:jl_uv_fs_t_ptr, Cstring, (Ptr{Cvoid},), req))
ccall(:uv_fs_req_cleanup, Cvoid, (Ptr{Cvoid},), req)
return path
finally
Libc.free(req)
end
end

@StefanKarpinski
Copy link
Member

It's true that the realpath function is kind of problematic, but it's a very low level function and I don't think we want our realpath to deviate too much from the system one, which has this behavior. Honestly, in this case we maybe shouldn't be calling realpath at all. In any case, improving/changing realpath is a much bigger discussion and since this fixes your issue, let's go with it. We can merge the two fixes independently and then once they're both included in master, the issue will be fixed. Another question is whether these are backportable fixes or not. I guess I don't really see why not, but it would be good to get some other opinions on backporting.

@StefanKarpinski
Copy link
Member

I've approved but, @ckingdon95, I need you to make this no longer a draft before I can merge.

@corakingdon corakingdon marked this pull request as ready for review December 18, 2019 17:10
@StefanKarpinski
Copy link
Member

I've opened #34135 to discuss improving realpath or offering a better replacement for it.

@StefanKarpinski StefanKarpinski merged commit 5da74be into JuliaLang:master Dec 18, 2019
@corakingdon corakingdon deleted the safe-realpath branch December 18, 2019 17:45
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
* Add try/catch around call to realpath in dummy_uuid
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.

4 participants