tempfile.mkdtemp() does not return absolute pathname when relative dir is specified

[roysmith]

BPO	7325
Nosy	@pitrou, @merwok, @bitdancer, @hynek, @1st1, @iritkatriel
Files	Py7325.diff Py(27)7325.diff issue7325.patch: updated patch based on Py(27)7325.diff issue7325.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2009-11-14.19:27:34.856>
labels = ['easy', 'type-bug', '3.9', '3.10', '3.11', 'library']
title = 'tempfile.mkdtemp() does not return absolute pathname when relative dir is specified'
updated_at = <Date 2021-12-06.14:33:45.013>
user = 'https://bugs.python.org/roysmith'

bugs.python.org fields:

activity = <Date 2021-12-06.14:33:45.013>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2009-11-14.19:27:34.856>
creator = 'roysmith'
dependencies = []
files = ['16005', '16028', '19610', '19737']
hgrepos = []
issue_num = 7325
keywords = ['patch', 'easy']
message_count = 19.0
messages = ['95256', '95257', '95307', '95308', '98316', '98317', '98342', '98405', '98406', '98457', '111828', '121214', '121734', '121764', '121906', '122038', '178291', '209838', '407816']
nosy_count = 11.0
nosy_names = ['roysmith', 'pitrou', 'dstanek', 'eric.araujo', 'r.david.murray', 'jesstess', 'thomas.holmes', 'elesbom', 'hynek', 'yselivanov', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue7325'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

• gh-51574: Make tempfile.mkdtemp() always return absolute paths #94612
• [3.11] gh-51574: Document behaviour of mkdtemp on 3.11 and lower #103844

[roysmith]

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')
>>>

[bitdancer]

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.

[brettcannon]

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.

[pitrou]

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.

[thomasholmes]

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.

[thomasholmes]

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.

[bitdancer]

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?

[thomasholmes]

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.

[thomasholmes]

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?

[thomasholmes]

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.

[BreamoreBoy]

Tried this on windows against 2.7 don't see why it can't go forward.

[jesstess]

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.

[elesbom]

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.

[merwok]

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.)

[elesbom]

You is right, i put this command on test and not the test.
thankss

[merwok]

os.path.abspath actually checks for isabs first, so I think you can leave the test out and always call abspath.

[hynek]

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?

[1st1]

bump? see also bpo-20267

[iritkatriel]

It's still the same in 3.11. I don't know whether this is something to fix in the code or in the docs.

>> import tempfile
>>> tempfile.mkdtemp(dir='.')
'./tmplvkt14za'
>>> tempfile.mkstemp(dir='.')
(3, '/Users/iritkatriel/src/cpython/tmp86ljh34k')

[merwok]

@kj7rrv made a pull request but that doesn’t send a notification to people here.

The PR needs tests and docs (with versionchanged note); I think some of the original patches here contained these, so you can take inspiration from them.

[kj7rrv]

@merwok Oh, I didn't see there is already a PR for this.

[merwok]

No, I was referring to your PR!

[merwok]

The PR is ready for a final review. I think it should not be backported, but a doc PR for older branches could be created.

[AlexWaygood]

The PR is ready for a final review. I think it should not be backported, but a doc PR for older branches could be created.

The PR has now been merged, fixing the issue for 3.12 and newer. It won't be backported. All that is left to do now is to file a docs PR to the 3.11 branch to clarify the behaviour for 3.11 and older.

@mblahay, would you be interested in doing that, maybe?

[kj7rrv]

I can do the doc PR if you want. Should it change "mkdtemp() returns the absolute pathname of the new directory" to say "mkdtemp() returns the pathname of the new directory" and add "Changed in version 3.12: the return value is always an absolute path, even if dir is relative"?

[AlexWaygood]

I can do the doc PR if you want. Should it change "mkdtemp() returns the absolute pathname of the new directory" to say "mkdtemp() returns the pathname of the new directory" and add "Changed in version 3.12: the return value is always an absolute path, even if dir is relative"?

I realised that getting the wording right here was quite tricky, since we don't usually "document bugs", so I went ahead and made a PR myself: #103844. Reviews are welcome! :)

[AlexWaygood]

The docs PR for 3.11 has been merged. Closing this issue as completed!