Clean revision of temporary file and directory functions #1440

johnmyleswhite · 2012-10-24T16:17:12Z

Here is a clean pull request for working with temporary files and directories.

I've left the tests inactive until others confirm that they pass on other machines.

staticfloat · 2012-10-25T01:58:38Z

Sorry to nitpick, but if we honor environment variables, I feel they should override even what tmpnam does. Other than that, this looks excellent to me.

johnmyleswhite · 2012-10-25T02:00:30Z

Ok. I can search environment variables in tempdir and test for their presence in tmpnam.

johnmyleswhite · 2012-10-25T02:08:19Z

Another revision made. See what you think of this.

staticfloat · 2012-10-25T02:13:53Z

Looks good to me! Thanks for all your work on this. :)

ViralBShah · 2012-10-25T04:57:25Z

base/file.jl

We really need a better way to get the value of L_tmpnam rather than hardcoding 1024. This could lead to security and/or corruption issues at a later date. I don't know of a good way to do this though. :-(

Yes, this is not a great long term strategy. It would be good to find a way to learn the true value of L_tmpnam.

But, for now, how about being a little wasteful and using 4096? Based on this list from Wikipedia (http://en.wikipedia.org/wiki/Comparison_of_file_systems), no existing filename should be longer than 4096. While this doesn't apply to full paths, it's at least a little safer at the expense of 4x as much memory.

I would feel safer with 4096 in that case, even though that is not the greatest. I can't think of a way to get this except to compile a C program and have it print the value. Perhaps there are better ways.

I agree that adding a C function that returns the value is the only surefire to do this. Should we go ahead with that?

Just pass C_NULL to tmpnam, and copy the return string.

Kudos to @nolta for actually reading the manpage. :P

Change made.

nolta · 2012-10-25T16:33:41Z

base/file.jl

We also need a check if the path already exists. So why don't we just go back to your original tempname, and move the tmpnam call into tempdir?

Why not just use the looping approach from the previous tempname with this call to tmpnam? I might just be missing something, but it seems useful to have everything boil down to a call to tmpnam here.

Do we even want to use tmpnam, since it doesn't work on windows?

There's an equivalent implementation on Windows: http://msdn.microsoft.com/en-us/library/18x8h1bh(v=vs.80).aspx. (Why? Why are there parentheses in that URL?!) There's an insecure version as well, but Microsoft doesn't like you to use those.

I'm not sure. There were some concerns about synchrony among RNG's corrupting the names produced by randstring, which is why I moved away from it. I don't really know what the right direction is for going forward. When some of the Windows developers last chimed in, I had the impression that they would recommend using special calls on Windows, but I don't have the bandwidth or experience to implement that myself.

GetTempPath and GetTempFileName are the functions to use on Windows. If you want to use tmpnam, I'm ok with you just marking it linux_only and making a windows_only version that raises a NotImplementedError. I''ll just open an issue against the windows version that it needs to be eventually implemented.

(@staticfloat to make matters worse, msdn is using that as a param string for the version, so the url works without it)

Ok. I'll go ahead and do that then.

Made changes. Didn't see a NotImplementedError type exception, so I'm just raising error("not yet implemented").

nolta · 2012-10-25T22:45:57Z

@unix_only begin
    function tempname()
        d = get(ENV, "TMPDIR", C_NULL) # darwin workaround
        p = ccall(:tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
        s = bytestring(p)
        c_free(p)
        s
    end
    tempdir() = dirname(tempname())
end

Works on linux, freebsd, & darwin.

vtjnash · 2012-10-26T01:10:54Z

@nolta that should work on windows too, with the addendum that the function is called as _tempnam

johnmyleswhite · 2012-10-26T13:19:11Z

@vtjnash So the final versions would be the following?

@unix_only function tempname()
    d = get(ENV, "TMPDIR", C_NULL) # darwin workaround
    p = ccall(:tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
    s = bytestring(p)
    c_free(p)
    s
end

@windows_only function tempname()
    d = get(ENV, "TMPDIR", C_NULL) # darwin workaround
    p = ccall(:_tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
    s = bytestring(p)
    c_free(p)
    s
end

tempdir() = dirname(tempname())

pao · 2012-10-26T14:53:56Z

function tempname()
    d = get(ENV, "TMPDIR", C_NULL) # darwin workaround
    @windows_only p = ccall(:_tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
    @unix_only p = ccall(:tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
    s = bytestring(p)
    c_free(p)
    s
end

?

nolta · 2012-10-26T15:17:43Z

tempnam = (OS_NAME == :Windows) ? :_tempnam : :tempnam
@eval begin
    function tempname()
        d = get(ENV, "TMPDIR", C_NULL) # tempnam ignores TMPDIR on darwin
        p = ccall($tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
        ...

johnmyleswhite · 2012-10-26T15:23:29Z

Why do we need the @eval?

nolta · 2012-10-26T15:26:05Z

ccall is special.

johnmyleswhite · 2012-10-26T15:31:14Z

Ok. Changes made.

nolta · 2012-10-26T15:32:46Z

Hmm, actually we don't. I added eval because this fails:

function tempname()
    tempnam = (OS_NAME == :Windows) ? :_tempnam : :tempnam
    d = get(ENV, "TMPDIR", C_NULL) # tempnam ignores TMPDIR on darwin
    p = ccall(tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
    ...

But it works if tempnam is a global:

tempnam = (OS_NAME == :Windows) ? :_tempnam : :tempnam
function tempname()
    d = get(ENV, "TMPDIR", C_NULL) # tempnam ignores TMPDIR on darwin
    p = ccall(tempnam, Ptr{Uint8}, (Ptr{Uint8},Ptr{Uint8}), d, "julia")
    ...

johnmyleswhite · 2012-10-26T15:35:35Z

Ok. The eval's gone and tempnam is a global now.

pao · 2012-10-26T16:08:15Z

Thanks, @nolta and @johnmyleswhite, that's even better!

nolta · 2012-10-26T16:09:44Z

base/file.jl

p = ccall(:mkstemp, Int32, (Ptr{Uint8},), b)

vtjnash · 2012-10-26T16:54:12Z

ccall is special because it requires that the first three arguments be known (constant) at the compile time of the function. I was going to suggest Pao's code, to avoid the global and minimize duplication of code. But anything that accomplishes that is sufficient.

vtjnash · 2012-10-26T17:05:09Z

mkdtemp and mkstemp do not exist on windows, so can you surround those functions with unix_only and then squash the history on this pull request so it is ready to merge?

Use tempname() in tempdir(); tempdir() in mktemp*() Environment variables take precendence for temporary directories Use C_NULL for arbitrary length filenames Insure that path doesn't exist Made candidate_tempname() a unix_only function tempname and tempdir for both Windows and UNIX Removed superfluous eval Revised tests Specify 32-bit ints Made mkstemp and mkdtemp wrappers UNIX only functions

johnmyleswhite · 2012-10-26T19:56:46Z

I corrupted my fork doing the squashing. I'm closing this one and opening one last pull request with the revised code.

@IanButterworth

) Stdlib: LinearAlgebra URL: https://github.com/JuliaLang/LinearAlgebra.jl.git Stdlib branch: master Julia branch: master Old commit: d568106 New commit: 7e11b5e Julia version: 1.13.0-DEV LinearAlgebra version: 1.13.0 Bump invoked by: @IanButterworth Powered by: [BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl) Diff: JuliaLang/LinearAlgebra.jl@d568106...7e11b5e ``` $ git log --oneline d568106..7e11b5e 7e11b5e Add `AbstractArray` conversions for `AbstractQ` (#1470) 52c41f7 Add missing methods to `diagind` documentation (#1473) 28ee87e Overload array constructors for BunchKaufman (#1461) (#1466) 6f73f65 Add 'eigmin'/'eigmax' methods for 'Eigen' (#1468) 7b21cab Use `Iterators.rest` within `generic_norm` to simplify code (#1459) 57ac0eb Make `parentof_applytri` fully type-stable (#1243) 880a9fe Add `_sym_uplo` to skip validation (#1441) 8d6ca14 Public function to access the `uplo` for `Symmetric`/`Hermitian` (#1440) 7a4b27e Make `dot` with Bool-arrays type-stable (#1456) 6fe77f8 Make `dot` with Bool-arrays type-stable 5af75df Remove zeroing in `similar` for `Hermitian` (#1455) 5685390 Index into diag in `Tridigaonal` * `Diagonal` (#1454) 51923a5 Forward structure-preserving broadcasting to diag for `Diagonal` (#1423) 35a4427 Iterator norm in `isapprox` for `Array`s (#1378) 98723df Structured broadcasting for UpperHessenberg (#1325) ``` Co-authored-by: IanButterworth <1694067+IanButterworth@users.noreply.github.com>

johnmyleswhite added 2 commits October 24, 2012 12:15

Revised temporary file and directory functions, incl. inactive tests

46959b6

Use tempname() in tempdir(); tempdir() in mktemp*()

4ad6001

Environment variables take precendence for temporary directories

b01b862

ViralBShah reviewed Oct 25, 2012
View reviewed changes

Use C_NULL for arbitrary length filenames

7e91535

nolta reviewed Oct 25, 2012
View reviewed changes

johnmyleswhite added 2 commits October 25, 2012 14:07

Insure that path doesn't exist

0866fd5

Made candidate_tempname() a unix_only function

bc02012

tempname and tempdir for both Windows and UNIX

77a3ed0

Removed superfluous eval

5c2275e

nolta reviewed Oct 26, 2012
View reviewed changes

johnmyleswhite added 2 commits October 26, 2012 12:11

Revised tests

bd7b6c6

Specify 32-bit ints

8a4fd8c

johnmyleswhite added 3 commits October 26, 2012 15:42

Made mkstemp and mkdtemp wrappers UNIX only functions

ef8ad76

Merge branch 'master' of github.com:johnmyleswhite/julia

0e56afd

johnmyleswhite closed this Oct 26, 2012

Uh oh!

Clean revision of temporary file and directory functions #1440

Clean revision of temporary file and directory functions #1440

Uh oh!

Conversation

johnmyleswhite commented Oct 24, 2012

Uh oh!

staticfloat commented Oct 25, 2012

Uh oh!

johnmyleswhite commented Oct 25, 2012

Uh oh!

johnmyleswhite commented Oct 25, 2012

Uh oh!

staticfloat commented Oct 25, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolta commented Oct 25, 2012

Uh oh!

vtjnash commented Oct 26, 2012

Uh oh!

johnmyleswhite commented Oct 26, 2012

Uh oh!

pao commented Oct 26, 2012

Uh oh!

nolta commented Oct 26, 2012

Uh oh!

johnmyleswhite commented Oct 26, 2012

Uh oh!

nolta commented Oct 26, 2012

Uh oh!

johnmyleswhite commented Oct 26, 2012

Uh oh!

nolta commented Oct 26, 2012

Uh oh!

johnmyleswhite commented Oct 26, 2012

Uh oh!

pao commented Oct 26, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtjnash commented Oct 26, 2012

Uh oh!

vtjnash commented Oct 26, 2012

Uh oh!

johnmyleswhite commented Oct 26, 2012

Uh oh!

Reviewers

Assignees

Labels