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

RFC: f(::Function) variants for mktemp and mktempdir #9017

Merged
merged 3 commits into from Nov 18, 2014
Merged

RFC: f(::Function) variants for mktemp and mktempdir #9017

merged 3 commits into from Nov 18, 2014

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2014

While making some test code requiring temporary directories I remembered that Python has some awesome temporary file objects that you can combine with with-statements to make the clean-up automagical. With this PR, we can be just as awesome with our do-blocks:

mktempdir() do d
    touch(joinpath(d, "foo.md"))
end

mktemp() do p, io
    print(io, "鴨かも?") # Silly Japanese play on words.
end

"Bonus commits":

  1. Re-enabling several tests in test/file.jl, that were disabled back in 02a6aab. For Windows compatibility? Now that Windows support the operations we should be able to re-enable them safely.
  2. Moving several Unix-only tmp functions in file.jl into a single block protected by a @unix_only to follow the same pattern that was used for the corresponding Windows-only code.

@ghost
Copy link
Author

ghost commented Nov 15, 2014

Travis appears to have some sort of issue, I will try to force-push and see if it will make it pick up the PR.

@ghost
Copy link
Author

ghost commented Nov 15, 2014

Success for Linux, failed (timed out?) on OSX. Seems unrelated.

@ghost
Copy link
Author

ghost commented Nov 16, 2014

Travis passing, seems to have been a temporary hiccup.

@ghost
Copy link
Author

ghost commented Nov 18, 2014

There seems to be considerably less excitement about this than I expected. Am I really the only one that has had to deal with wrapping a ton of badly behaving third-party tools over the years? These use-cases, along with testing code is really where this kind of functionality shines. I really feel that small improvements like this showcases the awesomeness our language and makes our API appealing and keeps it on-par with "lesser" modern languages.

end

function mktemp(fn::Function)
const (tmp_path, tmp_io) = mktemp()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the consts, they don't do anything inside of a function.

@jakebolewski
Copy link
Member

I agree, this would be nice to have.

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

I like the idea quite a bit, just haven't gotten a chance to test the PR.

@jakebolewski
Copy link
Member

Not to sidetrack this discussion, but I'm wondering about the security concerns raised in the python docs for tempory files https://docs.python.org/3.4/library/tempfile.html#tempfile.mkstemp. Do we address any of them wrt tmp file creation?

@ghost
Copy link
Author

ghost commented Nov 18, 2014

@jakebolewski: Thank you for the comment about const, I have force-pushed an updated version.

Also, sorry to the community for calling out the lack of activity. I do realise that your time is valuable and limited. I am still a bit anxious as a contributor I guess, not trusting my own intuition.

@jakebolewski
Copy link
Member

Needs a squash, but LGTM.

Pontus Stenetorp added 3 commits November 18, 2014 16:36
Appears to have been disabled since
02a6aab, or, for about two years.
Variable renaming to resolve conflicts with newer code.
@ghost
Copy link
Author

ghost commented Nov 18, 2014

@jakebolewski: You mean squash the three distinct commits into a single commit? Wouldn't that make tracking changes and reverts considerably more difficult? Sorry to ask, but I don't think there is an authoritative statement in CONTRIBUTING.md.

@ghost
Copy link
Author

ghost commented Nov 18, 2014

Just to add very quickly so that I get this right in the future, if there was to be a squash, then the code move and re-enabling the tests should have been submitted as separate PRs? Sorry for adding them as "bonuses" in that case, I thought that they were just too minor to warrant their own PRs.

@nalimilan
Copy link
Member

Nice addition. Could you add documentation for the new variants?

@nalimilan
Copy link
Member

@jakebolewski Regarding security concerns, this looks OK to me on Unix as the code calls mkdtemp (I don't know on Windows though).

@staticfloat
Copy link
Member

@ninjin I think you're okay with the three separate commits; you've got a good separation of concerns with these commits. Your other two commits are indeed minor, which is often a good indicator for a squash candidate, but really it's often a matter of taste. Don't worry about making PRs that are too minor, it makes it very easy to hit the "merge" button. :P

We already satisfy all of the python concerns that I can see: No race conditions, readable and writable only by owner, not executable, etc...

staticfloat added a commit that referenced this pull request Nov 18, 2014
RFC: f(::Function) variants for `mktemp` and `mktempdir`
@staticfloat staticfloat merged commit 3b61724 into JuliaLang:master Nov 18, 2014
@ghost
Copy link
Author

ghost commented Nov 18, 2014

@nalimilan: I'll push the documentation on my stack.

We already satisfy all of the python concerns that I can see: No race conditions, readable and writable only by owner, not executable, etc...

@staticfloat: I love this, it is exactly the mindset one should have with an API.

@ghost ghost deleted the nin/mktmp_do branch November 18, 2014 09:02
@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

fails on windows at line 274, ispath(path) is true

@ghost
Copy link
Author

ghost commented Nov 18, 2014

fails on windows at line 274, ispath(path) is true

That is really odd and probably deserves an issue on its own. What this would mean is that tempname() on Windows actually creates a file, unlike its *NIX counterpart.

path = tempname()
@test ispath(path) == false

If someone with a Windows box and some Windows systems programming knowledge could have a look at the relevant code, it would be greatly appreciated.

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

See http://msdn.microsoft.com/en-us/library/windows/desktop/aa364991(v=vs.85).aspx

If a unique file name is generated, an empty file is created and the handle to it is released

@ghost
Copy link
Author

ghost commented Nov 18, 2014

@tkelman: Thanks a bundle, I will file a PR to disable the offending test for now and open a separate issue to discuss what to do about this discrepancy.

@nalimilan
Copy link
Member

tempname is racy anyways, wouldn't it be safer to deprecate it? (Though of course a fix is needed until it's removed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants