TemporaryDirectory does not resolve path when created using a relative path

[anntzer]

BPO	20267
Nosy	@birkenfeld, @ncoghlan, @pitrou, @serhiy-storchaka, @1st1, @anntzer
Files	tempfile_01.patch tempfile_02.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 2014-01-15.04:48:28.375>
labels = ['library']
title = 'TemporaryDirectory does not resolve path when created using a relative path'
updated_at = <Date 2014-10-05.00:20:00.718>
user = 'https://github.com/anntzer'

bugs.python.org fields:

activity = <Date 2014-10-05.00:20:00.718>
actor = 'Antony.Lee'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2014-01-15.04:48:28.375>
creator = 'Antony.Lee'
dependencies = []
files = ['33570', '36740']
hgrepos = []
issue_num = 20267
keywords = ['patch']
message_count = 13.0
messages = ['208141', '208571', '208591', '210581', '210589', '227649', '227653', '227659', '227662', '227665', '227701', '228459', '228513']
nosy_count = 6.0
nosy_names = ['georg.brandl', 'ncoghlan', 'pitrou', 'serhiy.storchaka', 'yselivanov', 'Antony.Lee']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue20267'
versions = ['Python 3.5']

[anntzer]

Essentially, the following fails:

t = tempfile.TemporaryDirectory(dir=".")
os.chdir(some_other_dir)
t.cleanup()

because t.name is a relative path. Resolving the "dir" argument when the directory is created should(?) fix the issue.

[1st1]

This looks like a bug to me (or we should complain if a relative 'dir' was passed).

I'm attaching a patch that fixes this, although I'm not sure if the unittest I wrote will work on windows.

Moreover, 'mkstemp' and 'mktemp' have the same bug (separate issue for them?)

[1st1]

The patch is ok for windows.

[anntzer]

Thanks for the fix. The same fix seems should also work for mkstemp and mktemp -- is it really worth creating a new issue for them?

[1st1]

Thanks for the fix. The same fix seems should also work for mkstemp and mktemp -- is it really worth creating a new issue for them?

No, no new issue is necessary. I think I'll update the patch and merge it in 3.5, as it's too late for 3.4

[1st1]

A second version of the patch (tempfile_02), fixing more tempfile functions to properly support relative paths. Please review.

[anntzer]

This looks reasonable. Note that the output of gettempdir is always passed as first argument to os.path.join (possibly via _mkstemp_inner), so perhaps you should rather define something like

def _absolute_child_of_parent_or_tmpdir(parent, *args):
    """Return the absolute child of parent, or gettempdir() if parent is None, given by *args.
    """
    if parent is None:
        parent = <_sanitize_dir> # inline the code here
    return _os.path.join(parent, *args)

and use that function instead.

This factorizes the code a little bit more and makes intent clearer (I don't think _sanitize_dir is a very clear name).

[1st1]

Antony, I agree regarding the poor naming of '_sanitize_dir()' helper. As for your other suggestion, I think such a refactoring will actually make code harder to follow (+ it's more invasive). Generally, I'm in favour of transforming parameters like 'dir' closer to the beginning of the method's code, so that it's immediately obvious what's going on, and is also easier to put debug code [like 'print("mkdtemp call for: ", dir)'].

[anntzer]

I don't feel strongly about this, so either way is fine.

[serhiy-storchaka]

Note that abspath() can return incorrect result in case of symbolic links to directories and pardir components. I.e. abspath('symlink/..').

[1st1]

Note that abspath() can return incorrect result in case of symbolic links to directories and pardir components. I.e. abspath('symlink/..').

Good catch.. Should I use os.path.realpath?

[serhiy-storchaka]

Should I use os.path.realpath?

I don't see other way.

But there is other issue. Following code works now, but will fail with the patch:

import os, tempfile
os.mkdir('parent')
os.chdir('parent')
t = tempfile.TemporaryDirectory(dir=".")
os.rename('../parent', '../parent2')
t.cleanup()

[anntzer]

The change would be backwards-incompatible but also mimics the behavior of NamedTemporaryFile (which also fails to delete the file if the containing folder has been renamed -- this is easy to verify manually).
I guess the other option would be to use fd-based semantics? (but it'd be preferable if the behavior was kept the same between TemporaryDirectory and NamedTemporaryFile).