Add prefix argument to mktempdir() #23634

sambitdash · 2017-09-08T13:25:01Z

Continuation of #23237, which has all review comments (remerging and rebasing detached the old PR and I cannot reopen it).

Fixes #22922

sambitdash · 2017-09-08T13:48:19Z

@nalimilan, @fredrikekre, @vtjnash, @kshyatt, Please review #23237 here.

@nalimilan Comments on the test cases are incorporated.

On the base/file.jl waiting on the further comments on approach for readdir vs. readlink way of handling the req variable in libuv.

sambitdash · 2017-09-08T13:50:27Z

base/file.jl

+    template = prefix*"XXXXXX"
+    tpath = joinpath(parent, template)
+
+    req = Libc.malloc(_sizeof_uv_fs)


Decision to be taken if readdir or readlink approach be taken in allocating this variable. @vtjnash to recommend.

sambitdash · 2017-09-08T13:52:42Z

base/file.jl

+            eventloop(), req, tpath, C_NULL)
+        if ret < 0
+            ccall(:uv_fs_req_cleanup, Void, (Ptr{Void},), req)
+            uv_error("mktempdir", ret)


Formatting to be fixed.

nalimilan

Thanks, should be good after the few test fixes and validation by @vtjnash.

nalimilan · 2017-09-08T14:40:26Z

test/file.jl

+        end
+
+    else
+        # '/' is accepted in a prefix but affects the overall path and permissions.


/ does not seem to be accepted, according to the test.

/ only as the first character of a prefix creates problems. If a path is formed by $parent/someval/ where a path may exist in the system like say /tmp/. It will work. So technically, '/' is supported as a prefix character if the final path is writable by the process.

julia> mktempdir("/";prefix="tmp/") "/tmp/9dLil5" julia> julia> mktempdir("/";prefix="tmp/s") "/tmp/sZZDLWl" julia> mktempdir(;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] (::getfield(Base.Filesystem, Symbol("#kw##mktempdir")))(::Array{Any,1}, ::typeof(mktempdir), ::String) at ./<missing>:0 (repeats 2 times) julia> mktempdir(;prefix="ss/") ERROR: mktempdir: no such file or directory (ENOENT) Stacktrace: [1] uv_error at ./libuv.jl:68 [inlined] [2] #mktempdir#12(::String, ::Function, ::String) at ./file.jl:359 [3] (::getfield(Base.Filesystem, Symbol("#kw##mktempdir")))(::Array{Any,1}, ::typeof(mktempdir), ::String) at ./<missing>:0 (repeats 2 times) julia> mkdir("/tmp/ss");mktempdir(;prefix="ss/") "/tmp/ss/ZDAbPm" julia>

OK, so just make the comment more explicit.

nalimilan · 2017-09-08T14:42:11Z

test/file.jl

+        mkdir("c:/mydir")
+        # The temp directory created is of form "C:\\XXXXXX"
+        mktempdir("c:/mydir"; prefix="c:/") do tmpdir
+            filename = basename(tmpdir)


Better also test the path before the base name to ensure that what the comment says actually happens. Same below and on Unix.

Do you expect me to test for the following?

mktempdir("c:/mydir"; prefix="c:/") do tmpdir @test isdir(tmpdir) # Does not makes much of sense as mktempdir() will create the dir() anyway. # Our focus is mostly the name of the dir created. # Those must be tested as part of regular mktempdir (without prefix) test case. filename = basename(tmpdir)

The file already has a large number of mktempdir(without prefix) test cases.

I meant that since you say "The temp directory created is of form "C:\XXXXXX"", you should actually test that it's the case, i.e. that startswith(tempdir, C:\\").

JeffBezanson · 2017-09-08T14:45:23Z

base/file.jl

@@ -356,12 +339,33 @@ is an open file object for this path.
 mktemp(parent)

 """
-    mktempdir(parent=tempdir())
+    mktempdir(parent=tempdir(); prefix="$temp_prefix")


"$temp_prefix" == temp_prefix

The idea is to use the value of the global variable rather than repeating it. Seems to work as expected:

julia> x = 1 1 julia> """x = "$x" """ x x help?> x x = "1"

My mistake --- I didn't notice this was in a doc string :)

Technically $(repr(temp_prefix)) is more correct in case there are weird characters in the prefix (seems unlikely, but it is possible).

nalimilan · 2017-09-08T15:24:35Z

test/file.jl

+        # The API accepts "c:/ and c:\\" as valid prefixes.
+        # The parent directory is ignored in that case.
+
+        mkdir("c:/mydir")


In most cases the user running Julia won't have access to that path (fortunately), so we'll have to find something else. Maybe pass tempdir() as a prefix, since we know we can write there?

nalimilan · 2017-09-08T15:25:26Z

test/file.jl

+            filename = basename(tmpdir)
+            @test length(filename) == 6
+        end
+        rm("c:/mydir")


Would be nice to also remove the directory in other cases.

vtjnash · 2017-09-08T17:49:53Z

On the base/file.jl waiting on the further comments on approach for readdir vs. readlink way of handling the req variable in libuv.

Yikes! That's quite a mess of different approaches in there. OTOH, you can use either approach here.

sambitdash · 2017-09-08T18:39:35Z

Considering the comment from @vtjnash, I have committed only the formatting changes needed in the base/file.jl. That should address all the comments so far received on the PR.

nalimilan

Looks good to me, as long as @vtjnash agrees.

sambitdash · 2017-09-09T03:18:53Z

The errors on CircleCI do not seem relevant to the PR and Travis is stuck for some reason.

sambitdash · 2017-09-12T10:10:21Z

Both the Travis and CircleCI seem to report spurious errors.

sambitdash · 2017-09-22T16:49:26Z

closing and reopening to rerun tests.

sambitdash · 2017-09-23T05:20:46Z

Errors reported by Travis and CircleCI seem to be unrelated to the check-in. In fact in the earlier iterations the 64-bit version on Travis was passing while the Mac OS X tests didn't even kick off. Please review the changes for merge if any changes are needed let me know.

nalimilan · 2017-09-24T15:47:01Z

@vtjnash Merge?

vtjnash · 2017-09-25T16:56:41Z

base/file.jl

+        if ret < 0
+            ccall(:uv_fs_req_cleanup, Void, (Ptr{Void},), req)
+            uv_error("mktempdir", ret)
+            assert(false)


don't need this assert

removed in latest check-in.

vtjnash · 2017-09-25T17:13:56Z

base/file.jl


 Create a temporary directory in the `parent` directory and return its path.
-If `parent` does not exist, throw an error.
+If `parent` does not exist, throw an error. An optional `prefix` to the directory name can


The wrapping seems odd here. If you wrap once at the period, I think the rest will fit on the following line.

Fixed in latest check-in.

vtjnash · 2017-09-25T17:15:50Z

base/file.jl

-mktempdir(parent)
+function mktempdir(parent=tempdir(); prefix="$temp_prefix")
+    template = prefix*"XXXXXX"
+    tpath = joinpath(parent, template)


I think the intention of this API was to compute simply:
tpath = "$(parent)$(path_separator)$(template)XXXXXX"

You mean that it's simpler/more efficient to allocate the string in a single pass?

That doesn't matter here. However, the goal of the API appears to be to compute the simpler operation, not joinpath.

joinpath has 2 functions:

Use the relevant $(path_separator)

Provide a sane path relevant to the OS by stripping end separator.

ismatch(path_separator_re, a[end:end]) ? string(C,a,b) : string(C,a,pathsep(a,b),b)

If the input to parent variable ends with a $(path_separator) then the path created will have 2 consecutive $path_separator values. It does not fail but is not the correct return value.

Example:

julia> mktempdir("/tmp/"; prefix="abc") "/tmp//abclEMi2T" julia> mktempdir("/"; prefix="tmp/") "//tmp/rXRggu"

@vtjnash Can you review and suggest if joinpath should be removed from the code.

Yes, it should. But to your point, yes we can add some logic here to avoid duplicating the path separator if already present at the end of parent:

if ismatch(path_separator_re, parent[end:end]) tpath = "$(parent)$(template)XXXXXX" else tpath = "$(parent)$(path_separator)$(template)XXXXXX" end

vtjnash · 2017-09-25T17:17:00Z

Sorry for all of the incremental review. I know that's really annoying. But should be almost good to go :)

The libuv is used for this implementation in a platform independent manner.

1. drive letters as prefix 2. Unicode symbols as prefix instead of codes.

The libuv is used for this implementation in a platform independent manner.

Consolidate pinv tests (#23627)

1. drive letters as prefix 2. Unicode symbols as prefix instead of codes.

Revert "Consolidate pinv tests (#23627)" This reverts commit 0a8b120.

Revert "Revert "Consolidate pinv tests (#23627)"" This reverts commit 1c68698.

sambitdash · 2017-10-17T14:40:18Z

@StefanKarpinski, @vtjnash,

There has been almost a few months we have been working on this PR. And I see very sporadic reviews and review fixes invariably gets into merge conflicts and rebase resolutions that unnecessarily complicate the check-ins. If we do not see much priority in this feature, may be we should discard this feature from the product. I think I have checked-in the last functional code at least 10 days back yet no further update.

StefanKarpinski · 2017-10-17T15:52:53Z

It's not that we don't want this functionality, but that this needs careful review before merging and people are busy – we're trying to get Julia 1.0 ready very soon. Adding features can always be done in the future. Is there some part of this PR that breaks existing functionality?

nalimilan · 2017-10-17T16:10:03Z

AFAICT this should be ready to go, the remaining discussions where mostly about the best coding patterns.

StefanKarpinski · 2017-10-17T18:36:50Z

@nalimilan, could you review it, fix the conflicts and merge if it's good to go? (Please squash.)

nalimilan · 2017-10-17T19:26:46Z

I've approved it already, @vtjnash is the one who had comments.

StefanKarpinski · 2017-10-18T04:39:53Z

@vtjnash, please approve if you feel that your comments have been addressed.

ViralBShah · 2018-03-16T18:33:19Z

Any reason still pending?

JeffBezanson · 2018-04-05T19:12:38Z

base/file.jl

@@ -381,13 +393,14 @@ function mktemp(fn::Function, parent=tempdir())
 end

 """
-    mktempdir(f::Function, parent=tempdir())
+    mktempdir(f::Function, parent=tempdir(); prefix="$(repr(temp_prefix))")


I think this will include two sets of quotes (quotes are included in the output of repr of a string).

See previous comment: #23634 (comment)

StefanKarpinski · 2018-04-05T19:13:37Z

This is non-breaking and still not correct. Removing the triage label.

StefanKarpinski · 2018-04-05T19:15:59Z

base/file.jl


+    tpath = "$(parent)$(path_separator)$(prefix)XXXXXX"


If prefix contains the letter X this is going to be wrong; it's hard to say if that's avoidable or not, given the way the underlying uv_fs_mkdtemp API works.

My understanding is that the last 6 characters need to be X, but it doesn't matter what the other characters are.

Yes. It's not even clear why these chars need to be X rather than any other ASCII char.

Links to documentation for uv_fs_mkdtemp and mkdtemp given below:

http://docs.libuv.org/en/v1.x/fs.html
https://linux.die.net/man/3/mkdtemp

JeffBezanson · 2018-04-05T19:18:42Z

This is a new feature so doesn't need to be triaged right now.

tpapp · 2019-02-25T09:45:46Z

It would be great to revive this PR, as the feature is quite convenient.

sambitdash mentioned this pull request Sep 8, 2017

mktempdir() now supports prefix. #23237

Closed

sambitdash commented Sep 8, 2017

View reviewed changes

nalimilan reviewed Sep 8, 2017

View reviewed changes

nalimilan requested a review from vtjnash September 8, 2017 14:43

nalimilan changed the title ~~I22922 - Issue being addressed. The state is same as #23237~~ Add prefix argument to mktempdir() Sep 8, 2017

JeffBezanson reviewed Sep 8, 2017

View reviewed changes

nalimilan reviewed Sep 8, 2017

View reviewed changes

nalimilan approved these changes Sep 8, 2017

View reviewed changes

sambitdash closed this Sep 22, 2017

sambitdash reopened this Sep 22, 2017

StefanKarpinski assigned vtjnash Sep 25, 2017

vtjnash reviewed Sep 25, 2017

View reviewed changes

sambitdash added 8 commits October 3, 2017 18:39

mktempdir() now supports prefix.

3f0526f

The libuv is used for this implementation in a platform independent manner.

Documentation error fix.

155ec68

OS specific test anomalies detected.

e891f58

A test error previously not reported fixed.

7b371ab

modified test cases for Windows OS

536c3ca

1. drive letters as prefix 2. Unicode symbols as prefix instead of codes.

Removing the directory created by the testcase.

db4a9b5

Windows test case update.

48bd1f4

Clarifying the test cases further.

f9fc9aa

sambitdash added 10 commits October 6, 2017 13:32

mktempdir() now supports prefix.

7f08fdf

The libuv is used for this implementation in a platform independent manner.

OS specific test anomalies detected.

3e71966

Consolidate pinv tests (#23627)

modified test cases for Windows OS

033f6e7

1. drive letters as prefix 2. Unicode symbols as prefix instead of codes.

Removing the directory created by the testcase.

d585569

Removing the directory created by the testcase.

c4170f2

Revert "Consolidate pinv tests (#23627)" This reverts commit 0a8b120.

Fixing formating issue with the code.

c8d4c29

Fixed comment. Removed unreachable assert.

17f6597

Trimming file separator from parent as well as prefix.

e5efc91

Revert "Consolidate pinv tests (#23627)" This reverts commit 0a8b120.

Trimming file separator from parent as well as prefix.

5bb17e3

Revert "Revert "Consolidate pinv tests (#23627)"" This reverts commit 1c68698.

Adding repr to ensure invalid characters are cleared

2622ca2

Merge branch 'master' into I22922

70346a2

nalimilan requested a review from vtjnash March 21, 2018 14:00

ViralBShah added the triage This should be discussed on a triage call label Apr 2, 2018

JeffBezanson reviewed Apr 5, 2018

View reviewed changes

StefanKarpinski removed the triage This should be discussed on a triage call label Apr 5, 2018

StefanKarpinski added this to the 1.x milestone Apr 5, 2018

StefanKarpinski reviewed Apr 5, 2018

View reviewed changes

vtjnash mentioned this pull request Mar 2, 2019

add prefix argument to mktempdir #31230

Merged

vtjnash closed this in #31230 Mar 22, 2019

sambitdash deleted the I22922 branch March 25, 2019 16:18

Add prefix argument to mktempdir() #23634

Add prefix argument to mktempdir() #23634

Conversation

sambitdash commented Sep 8, 2017 • edited by nalimilan Loading

sambitdash commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambitdash Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Sep 8, 2017

sambitdash commented Sep 8, 2017

nalimilan left a comment

Choose a reason for hiding this comment

sambitdash commented Sep 9, 2017

sambitdash commented Sep 12, 2017

sambitdash commented Sep 22, 2017

sambitdash commented Sep 23, 2017

nalimilan commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Sep 25, 2017

sambitdash commented Oct 17, 2017

StefanKarpinski commented Oct 17, 2017

nalimilan commented Oct 17, 2017

StefanKarpinski commented Oct 17, 2017 • edited Loading

nalimilan commented Oct 17, 2017

StefanKarpinski commented Oct 18, 2017

ViralBShah commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Apr 5, 2018

tpapp commented Feb 25, 2019

sambitdash commented Sep 8, 2017 •

edited by nalimilan

Loading

sambitdash Sep 8, 2017 •

edited

Loading

StefanKarpinski commented Oct 17, 2017 •

edited

Loading