-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[RFC] Tests and thread-safety for stat.jl
#17166
Conversation
@@ -61,13 +61,14 @@ show(io::IO, st::StatStruct) = print(io, "StatStruct(mode=$(oct(filemode(st),6)) | |||
|
|||
# stat & lstat functions | |||
|
|||
const stat_buf = Array{UInt8}(ccall(:jl_sizeof_stat, Int32, ())) | |||
const stat_buf_sz = Int(ccall(:jl_sizeof_stat, Int32, ())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably use a NTuple
instead of an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're misreading this or I'm misunderstanding you. This look correct to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look correct. I'm just saying that we don't need to allocate an array here since the size is known at compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do this, but I could not get the NTuple{N,UInt8}
into Ptr{UInt8}
conversion to work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Ref{NTuple{N,UInt8}}()
and the buffer and Ref{NTuple{N,UInt8}}
as the ccall
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just stick with the Array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ninjin was trying to do that yesterday and having issues with getting it to work. How about we merge this if it's working and then you can make that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless, of course, @ninjin wants to keep going back and forth to get to that result here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with a merge once CI completes, I have another more extensive stat.jl
PR coming up.
stat.jl
stat.jl
@@ -0,0 +1,65 @@ | |||
# This file is a part of Julia. License is MIT: http://julialang.org/license | |||
|
|||
using Base.Filesystem: StatStruct, samefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file isn't actually getting executed? fixed
There are a fair number of lines here that are just doing something to make sure it doesn't error and not checking that it returns anything sane, but we can improve this over several rounds. |
@test samefile(f, f) | ||
@test samefile(d, d) | ||
@test !samefile(f, d) | ||
if is_unix() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sockets don't work on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they should work, they just require a specific formatting to the file name (they can't be put in an arbitrary directory)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there examples of that somewhere in our tests yet?
Good old Windows got me yet again (I could kill for SSH access to a Windows box to try out behaviour on that platform), please feel free to cancel the Travis job for the previous push. |
You can turn on appveyor on your Julia fork and use https://www.appveyor.com/docs/how-to/rdp-to-build-worker |
@test ctime(f) > 0 | ||
if !is_windows() | ||
m = mtime(f) | ||
# Is `touch` a NOP on Windows or just unreliable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
windows filesystem is slow to update
backed the osx log up to https://gist.github.com/50bae1b9ae4c39d9fde4d546f43e3f47 and restarted, failure was due to #17823 |
I don't remember why this got stalled, but seems like we've implemented this now, and probably have many tests, e.g. 72cf128 |
No description provided.