classification
Title: tempfile.mkdtemp() does not return absolute pathname when relative dir is specified
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: dstanek, elesbom, eric.araujo, hynek, jesstess, pitrou, r.david.murray, roysmith, thomas.holmes, yselivanov
Priority: normal Keywords: easy, patch

Created on 2009-11-14 19:27 by roysmith, last changed 2014-02-03 15:33 by BreamoreBoy.

Files
File name Uploaded Description Edit
Py7325.diff thomas.holmes, 2010-01-26 06:57
Py(27)7325.diff thomas.holmes, 2010-01-28 04:58
issue7325.patch jesstess, 2010-11-15 04:04 updated patch based on Py(27)7325.diff review
issue7325.patch elesbom, 2010-11-21 12:00 review
Messages (18)
msg95256 - (view) Author: Roy Smith (roysmith) Date: 2009-11-14 19:27
The docs (http://www.python.org/doc/2.5.1/lib/module-tempfile.html) specify that 
mkdtemp(), "returns the absolute pathname of the new directory".  It does that in 
the default case, but if you specify a relative path for 'dir', you get back a 
relative path.


$ python
Python 2.5.1 (r251:54863, Oct 17 2008, 14:39:09) 
[GCC 3.4.6 20060404 (Red Hat 3.4.6-10)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import tempfile
>>> tempfile.mkdtemp(dir='.')
'./tmpHk1pBD'

similar results were obtained on:

Python 2.5.1 (r251:54863, Feb  6 2009, 19:02:12) 
[GCC 4.0.1 (Apple Inc. build 5465)] on darwin

Note that mkstemp() gets it right:

>>> tempfile.mkdtemp(dir='.')
'./tmpoPXdL7'
>>> tempfile.mkstemp(dir='.')
(3, '/Users/roy2/tmpwTGZ2y')
>>>
msg95257 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-11-14 19:51
This is true on trunk and py3k as well.  2.5 is in security fix only
mode, so I've removed it from the versions list.

Since mkstemp does return in the absolute path in this case, I think
this is a code rather than a documentation bug.  However, changing it
would be backward incompatible, so it may not be possible to fix it in
2.6 and 3.1.
msg95307 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2009-11-15 20:56
It's a border case for backporting. The docs do say it should be returning 
an absolute path, so that is a bug. And chances are anyone who has 
discovered this and is working around it simply called os.path.abspath() 
on the value instead of some manual calculation (if at all; bigger chance 
no one simply noticed).

My vote is to backport. Otherwise the docs should be updated for 2.6/3.1 
to mention the bug.
msg95308 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-15 21:03
I think we should be conservative. People may be relying on it *not*
returning an absolute path; besides, the bug itself wasn't a problem in
practice, so backporting the fix doesn't bring any added value.
msg98316 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2010-01-26 06:57
I have created a patch for this for Python 3.1 and included an update to the unit tests.  The tests were never checking for a relative path and if they did would pas it even when it would have failed due to liberal use of os.path.abspath()

The test fix is probably not as clean as it could be by I did not want to severely alter its state.  The change to tempfile.mkdtemp() was clear and followed the current method tempfile.mkstemp() uses.
msg98317 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2010-01-26 07:07
As a side note, this was done mostly as an exercise for myself and as a learning experience.  Any feedback would be appreciated regardless of any decision on the status of this bug.
msg98342 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-01-26 16:17
First of all, thanks for working on this.  Now for some feedback :)

It is most helpful if you provide patches against trunk (which is 2.7 alpha right now).  We then forward port them to py3k.  (This will change after the release of 2.7, when py3k will become trunk.)

Isn't the assert you added to nameCheck redundant?  It seems to me it would be better just to remove the abspath call on ndir from the existing assert.

I haven't applied and tested the patch, but the other changes to the tests look odd to me.  Why are you obtaining the abspath of the current directory?  And why do those tests need to be changed at all?

Your additional test case looks like it might be some exploration code you forgot to delete before generating the patch, maybe those other changes are as well?
msg98405 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2010-01-27 03:39
I agree, I do not feel like the precise changes to the tests feel completely ideal.  I feel that this problem stems from the fact that the nameCheck function as originally written doesn't seem to completely serve its originally intended purpose.

The original issue that caused the modifications of the tests were as follows:

* After adding "test_abs_path()" namecheck would pass the test when it should actually fail due to the original assert performing os.path.abspath() on both paths.  The obviously solution to this seemed to be to take the abspath of the user supplied path (the variable dir in this function) relative or otherwise and compare to ndir which is derived from the passed in "name" variable, populated by the mkdtemp call.

* The second issue is a somewhat complex case of those asserts passing the atypical odd calls of nameCheck from the "test__RandomNameSequence" suite.  Without absolute pathing of both parameters in the new first assert this test began to fail.  This is because the original call was self.namecheck(<file name>, '', '', '').  These empty parameters when called with os.path.abspath() would return valid paths and these asserts would succeed.  As a result, the abspath(join()) call built a proper path into the "name" parameter allowing the tests to pass in this case.

* For the sake of cleanliness I decided it made more sense to split the two apart.  Make one assert that specifically verifies the path the file/folder is being placed in is both absolute and matching and one that will pass the file name.  Perhaps the old first assert (or second assert in the patch) can actually be removed but I think it may be getting properly tested against in one of the other test classes.

I am quite confident there is a much better way to accomplish this but I did not wish to change _too_ much of the test on my first stab at this.

I appreciate your feedback very much.  I will work on setting up the 2.7 environment for working on issues that span the 2x/3x gap.
msg98406 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2010-01-27 03:46
One other thing that crossed my mind while I was thinking over this today.  Instead of just relative pathing to '.' I should probably change my path to gettempdir() and then relative path to it since the location of the python executable may not be writable.

Any thoughts on how to go about testing this in a way that is most likely to succeed in all cases and respect OS permissions?
msg98457 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2010-01-28 04:58
I made a new patch off of the 2.7 trunk version.  I think I have handled some of the issues more cleanly.

Please see Py(27)7325.diff

I addressed the issue of using a relative path in the tempdir to achieve the 'guaranteed' ability to write rather than assuming the location of the python executable was writable.
msg111828 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-28 16:05
Tried this on windows against 2.7 don't see why it can't go forward.
msg121214 - (view) Author: Jessica McKellar (jesstess) * (Python triager) Date: 2010-11-15 04:04
Thomas, I think the weirdness you were sensing when trying to adapt the nameCheck calls in test__RandomNameSequence after adding the abspath check is that those test cases really don't need nameCheck.

For example, test_get_six_char_str should be testing that _RandomNameSequence returns something that is both a string and 6 characters, not properties of some path manufactured around it.

I've attached a patch based on Thomas's that factors out the string check in nameCheck and has the tests in test__RandomNameSequence just use the string check.

The patch is against release27-maint.
msg121734 - (view) Author: Rafael dos Santos Gonçalves (elesbom) Date: 2010-11-20 19:36
Hi people,
I insert a simple code in tempfile.py, that verify if the return starts with ./, and concatenate using the abspath function.
I changed the variable name file to temp_dir_abspath, because file is a built-in function and it's not recommended, overwrite it with local variables.
And at least i add one test, that verify if temp_directory returns an absolut path.

thanks.
msg121764 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-20 20:43
Looks good to me, with one exception:

 if temp_dir_abspath.startswith('./'):

Wouldn’t this be better:

 if not _os.path.isabs(temp_dir_abspath)

(P.S. file is not a builtin anymore in 3.x, it’s used for example as an argument to the print function.)
msg121906 - (view) Author: Rafael dos Santos Gonçalves (elesbom) Date: 2010-11-21 12:00
You is right, i put this command on test and not the test.
thankss
msg122038 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-22 00:14
os.path.abspath actually checks for isabs first, so I think you can leave the test out and always call abspath.
msg178291 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-12-27 10:26
I think we should resolve this one line change.

Jessica’s patch looks just fine, so I tend to apply it.  However, I’d like to document the current behavior in 2.7, 3.2, 3.3 and 3.4.

Am I missing anything?
msg209838 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-01-31 22:21
bump? see also #20267
History
Date User Action Args
2014-02-03 15:33:39BreamoreBoysetnosy: - BreamoreBoy
2014-01-31 22:21:08yselivanovsetnosy: + yselivanov
messages: + msg209838
2012-12-28 20:02:04brett.cannonsetnosy: - brett.cannon
2012-12-27 10:26:37hyneksetnosy: + hynek

messages: + msg178291
versions: + Python 2.7, Python 3.3, Python 3.4
2010-11-22 00:14:55eric.araujosetmessages: + msg122038
2010-11-21 12:00:29elesbomsetfiles: + issue7325.patch

messages: + msg121906
2010-11-21 11:59:09elesbomsetfiles: - issue7325.diff
2010-11-20 20:43:41eric.araujosetmessages: + msg121764
2010-11-20 19:36:08elesbomsetfiles: + issue7325.diff
nosy: + elesbom
messages: + msg121734

2010-11-17 22:42:29eric.araujosetversions: - Python 3.1, Python 2.7
2010-11-17 22:41:57eric.araujosetversions: + Python 3.1, Python 2.7
2010-11-16 09:48:56eric.araujosetnosy: + eric.araujo

versions: - Python 2.6, Python 3.1, Python 2.7
2010-11-15 04:04:33jesstesssetfiles: + issue7325.patch
title: tempfile.mkdtemp() does not return absolute pathname when dir is specified -> tempfile.mkdtemp() does not return absolute pathname when relative dir is specified
nosy: + jesstess

messages: + msg121214
2010-07-31 18:26:28dstaneksetnosy: + dstanek
2010-07-28 16:05:38BreamoreBoysetnosy: + BreamoreBoy
messages: + msg111828
2010-01-28 04:58:17thomas.holmessetfiles: + Py(27)7325.diff

messages: + msg98457
2010-01-27 03:46:35thomas.holmessetmessages: + msg98406
2010-01-27 03:39:17thomas.holmessetmessages: + msg98405
2010-01-26 16:17:18r.david.murraysetmessages: + msg98342
stage: test needed -> patch review
2010-01-26 07:07:59thomas.holmessetmessages: + msg98317
2010-01-26 06:57:56thomas.holmessetfiles: + Py7325.diff

nosy: + thomas.holmes
messages: + msg98316

keywords: + patch
2009-11-15 21:03:30pitrousetnosy: + pitrou
messages: + msg95308
2009-11-15 20:56:34brett.cannonsetnosy: + brett.cannon
messages: + msg95307
2009-11-14 19:51:30r.david.murraysetpriority: normal

versions: + Python 2.6, Python 3.1, Python 2.7, Python 3.2, - Python 2.5
keywords: + easy
nosy: + r.david.murray

messages: + msg95257
stage: test needed
2009-11-14 19:27:34roysmithcreate