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

mktempdir() should provide an option of a prefix very similar to you can do with mkdtemp in posix #22922

Closed
sambitdash opened this issue Jul 23, 2017 · 11 comments

Comments

@sambitdash
Copy link
Contributor

commented Jul 23, 2017

I think it may help if mktempdir() can provide an option of a prefix very similar to you can do with mkdtemp in POSIX. This provides the module developers to create temp directories with their own prefix which can help them able to track the directories created by them than everything starting with tmp or jl_ prefix.

The discussion thread on this topic can be seen here: https://discourse.julialang.org/t/mktempdir-should-provide-scope-to-declare-a-prefix/5020

@nalimilan

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

Cf. the previous PR #13969: it tried to allow passing a suffix to mktemp, which was not possible with the Windows API. But it supports passing a prefix (though it only uses up to the three first characters), so we could allow changing tmp to a custom prefix.

@sambitdash

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2017

Line 220 file.jl has:
uunique = ccall(:GetTempFileNameW,stdcall,UInt32,(Cwstring,Ptr{UInt16},UInt32,Ptr{UInt16}), temppath,utf16("jul"),uunique,tname)

jul - is sent as the fixed prefix. The prefix could be made a parameter.

For non-windows platforms

mkdtemp with fixed "tmpXXXXXXX" format is used that can be changed to something like "cccXXXXXXX" where ccc will be provided by developer or can be tmp by default.

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

I think jl_ prefix makes sense.

@nalimilan

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

Would you make a pull request to add a prefix keyword argument?

@sambitdash

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2017

Please review the commit. If you are in agreement I will add a few test cases and create a pull request.

@ararslan ararslan added the filesystem label Jul 23, 2017

@nalimilan

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

Thanks. I've made a few comments, but it's OK to open a pull request even if it's not perfect yet.

@cstjean

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

Suffix/file extension support would be convenient to test FileIO.load/save implementations.

@sambitdash

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

Hi All,

I think it's important we clarify and agree on the ideal implementation.

My take:

Prefix provision is for module developers so that they can identify their temp files easily and do some debugging or cleanup activity. It's not going to be a most used functionality as such. Default will be most common.

Second part:

The whole question here should be whether you are looking at Julia as a platform or OS as the platform. Ideally, a cross platform behavior of APIs should not be dependent on the underlying API used in the platform. Julia platform can use a common minimum behavior but for any advanced user the system APIs are always there to implement additional behaviors.

Hence, my first check-in truncated the number of characters to 3 for character lengths higher than 3 and ignored the prefix for prefix length lesser than 3 characters.

One additional restriction that can be imposed is prefix can only be ASCII or in my opinion base85 charsets other than the characters that cannot be technically part of a file path like: "/|*$" etc. which is not supported either in any of the OS as a file path or can technically complicate shell script writing.

Non-printable characters should be avoided as those can be considered malicious by a discerning system admin.

Without this clarity it may be confusing for a user trying to achieve cross platform behavior.

I think the implementation is taking longer than what I originally estimated and may not have the time to complete the task. May request someone to take up from here and finish the implementation the way you feel ideal for Julia as a platform.

I will check-in all the code with style issues fixed to best my knowledge so that someone can look into addressing the most ideal behavior if that's agreed.

@nalimilan

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

Let's have that discussion on the PR (#22998) to avoid splitting it in two places. Regarding your available time to work on this, there's no hurry and I think the PR very close to merging, so you can start working on it again when you want.

@sambitdash

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

While I wished I could spend more time on the PR (#22998), with my personal engagements elsewhere it may not be practical for me to commit further on it now. My apologies for the same. Whoever is interested can work on the master branch and take the PR to a closure. I will be happy to blanket approve any pull request on that branch in case that comes my way. I have already branched out my sandbox so there are no issues there.

However, some implementation challenges came up due to system implementation of libc, glibc or system specific user APIs which led to some API behavioral differences in each platform.

  1. If the current behavior is used Window prefix will be 3 characters while non-Windows like methods will have unlimited prefix length.
  2. tempname() uses the unix method tempnam() which will trunctate the Unix prefix to 5-bytes (not characters). While this has no impact on mktemp() or mktempdir().
  3. There was a proposal to use libuv() which only has mkdtemp() to create directories and no mkstemp() implementation for temp files.

Here are certain behavioral expectations based on which the documentation or tests may change.

  1. Should non-ASCII characters be supported as prefix?

Ideally, if there is a possibility of filename not properly get printed on a terminal, there is a general confusion on malicious behavior.

Recommended: Use of base-85 charset can be safe and removal of support for all characters which cannot be on a file name like "/|$" etc. taking the combination of all such characters in all known platforms. This can be enforced in API or can be left to user as a best practice recommendation.

  1. Should mktemp() support a suffix?

If mktemp is not looked as a POSIX like mktemp() verbatim, then it technically can. However, the current Windows API used does not have ability to support a suffix. mktempdir() may not need a suffix but flexibility and alignment of behavior with mktemp() does no harm.

  1. Should tempname() be supported?

tempname() in Unix is based on tempnam() which is not safe, hence that method can be deprecated. Issue #22949 

  1. Is there a common platform approach possible?

All the user level APIs like tempname() can be carried out in the Julia code and only open and close kind of call can be used from the system. This kind of implementation is provided in Python. This may remove all system level limitations like:

a. 3 character prefix in Windows
b. lack of implementation for suffix in Windows
c. 5 byte tempnam() system limitation in Unix for prefix.

On implementing tempname() there may need to establish there are no potential race condition across 2 Julia processes.

@maleadt

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

Implemented by #31230

@maleadt maleadt closed this Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.