mktempdir() now supports prefix. #23237
Conversation
|
With this #22998 can be closed. |
|
Added test cases which show behavioral differences across OS. For example, '*' is not an accepted prefix in Windows which is perfectly permitted in Linux as part of a filename. |
|
Error reported in Mac OS X is completely unrelated. It's an InterruptException which this code does not deal with. The Windows 32-bit case is more like incomplete run and is not a true failure. |
|
Closing as there is not much of activity in past week on this. If there is interest in this change let me know, I will be happy to reopen. |
|
Seems a bit premature to close this. @nalimilan, you seemed engaged on the original issue here. Can you review this and see if it addresses the problem? Frankly, I don't have enough context to understand what problem this is solving, but that doesn't mean it's not a good idea. |
|
PR #23259 affected the last test errors. Now fixed. |
|
Travis failure seems unrelated. |
|
@vtjnash has said he can take a look at this in a few days. Sorry for the delay, but this is a useful change. |
| req = Libc.malloc(_sizeof_uv_fs) | ||
| try | ||
| ret = ccall(:uv_fs_mkdtemp, Int32, | ||
| (Ptr{Void}, Ptr{Void}, Cstring, Ptr{Void}), |
There was a problem hiding this comment.
I followed the same indentation as readlink, although my personal preference would be to align to ccall.
+ ret = ccall(:uv_fs_mkdtemp, Int32,
+ (Ptr{Void}, Ptr{Void}, Cstring, Ptr{Void}),
| template = prefix*"XXXXXX" | ||
| tpath = joinpath(parent, template) | ||
|
|
||
| req = Libc.malloc(_sizeof_uv_fs) |
There was a problem hiding this comment.
I see you use the same approach as for readlink, but readdir uses a different strategy, by using zeros, which allows skipping the try... finally. @vtjnash Which approach is recommended?
There was a problem hiding this comment.
I guess I now get the approach here. In jl_uv.c all request structures are allocated in stack (declared as local var) and passed on to the function in the c-layer. The cleanup is carried out on the address to the request like &req. In the Julia layer you cannot have a stack variable so zero is a means to create a Julia array{UInt8} which will be memory managed in the Julia layer. Hence, no special action on a throw and clean up will be called on this object after the request object is used in the ccall. The approach is a bit hard to understand initially. Are try...catch overheads very significant? I can change the code to the zero approach.
There was a problem hiding this comment.
I would think the simplest approach is the best one, but others must know better.
test/file.jl
Outdated
| @test_throws Base.UVError mktempdir(; prefix="*") | ||
| @test_throws Base.UVError mktempdir(; prefix="cdcdccd/") | ||
| else | ||
| # '/' is accepted in a prefix but affects the overall path and permissions. |
There was a problem hiding this comment.
That comment sounds misleading, since / is not accepted by the next line.
There was a problem hiding this comment.
The whole behavior is very messy. I think we should just not accept '/' in prefix. Some samples:
julia> mktempdir("/";prefix="tmp/")
"/tmp/m12rZm"
julia> mktempdir("/tmp";prefix="/")
ERROR: mktempdir: permission denied (EACCES)
Stacktrace:
[1] uv_error at ./libuv.jl:68 [inlined]
[2] #mktempdir#12(::String, ::Function, ::String) at ./file.jl:359
[3] (::Base.Filesystem.#kw##mktempdir)(::Array{Any,1}, ::Base.Filesystem.#mktempdir, ::String) at ./<missing>:0
There was a problem hiding this comment.
I'm not too worried about this, as these behaviors make sense, but differences between OSes are annoying. What happens with \ on Windows?
| # Behavioral differences across OS types | ||
| if Sys.iswindows() | ||
| @test_throws Base.UVError mktempdir(; prefix="*") | ||
| @test_throws Base.UVError mktempdir(; prefix="cdcdccd/") |
There was a problem hiding this comment.
Does / without any characters before it also throw? What about \ or C:\?
There was a problem hiding this comment.
Give me a couple of days. My windows VM is needs some fixing before I can test this. I am a native Linux programmer :-).
test/file.jl
Outdated
| end | ||
|
|
||
| # Unicode test | ||
| tst_prefix="\u2200x\u2203y" |
There was a problem hiding this comment.
Directly use the Unicode chars, that's clearer.
* Doc checkout methods and type
Cf. problem reported there: #22948 (comment) Bracket pasting was not removing the indentation corresponding to the prompt (i.e. 7 spaces for "julia> ").
* Deprecate expm in favor of exp. * Remove reference to expm in the linalg docs
* REPL-documentation for more internals. * Copy edit 'REPL-documentation for more internals.' * New section "Special Types"
Previously the element type of the output was the smallest type that would fit the union of the input's individual element types. Now the output has an identical element type to the input. Fixes #22696.
The libuv is used for this implementation in a platform independent manner.
The libuv is used for this implementation in a platform independent manner.
The libuv is used for this implementation in a platform independent manner.
The libuv is used for this implementation in a platform independent manner.
The libuv is used for this implementation in a platform independent manner.
The libuv is used for this implementation in a platform independent manner.
The libuv is used for this implementation in a platform independent manner.
|
closing and resubmitting with rebasing. |
|
You can rebase and force-push to the same branch and keep the same PR. |
|
@fredrikekre The forced push detached the PR and could not reopen it. The new PR on this is #23634. |
|
Yea, I think you need to reopen before force-pushing in such a case. |
The libuv is used for this implementation in a platform independent manner. This is for the issue #22922. The
mktemp()ortempname()functions are not affected by this change.This may be still acceptable as a module owner can create her own temp directory and then create files inside that. Hence, the importance of prefix on files may not be as significant.
Edit: fix #22922