| msg208141 - (view) |
Author: Antony Lee (Antony.Lee) * |
Date: 2014-01-15 04:48 |
| 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. |
|
|
| msg208571 - (view) |
Author: Yury Selivanov (yselivanov) *  |
Date: 2014-01-20 19:47 |
| 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?) |
|
|
| msg208591 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2014-01-20 23:51 |
| The patch is ok for windows. |
|
|
| msg210581 - (view) |
Author: Antony Lee (Antony.Lee) * |
Date: 2014-02-08 01:28 |
| 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? |
|
|
| msg210589 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2014-02-08 05:05 |
| > 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 |
|
|
| msg227649 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2014-09-26 21:02 |
| A second version of the patch (tempfile_02), fixing more tempfile functions to properly support relative paths. Please review. |
|
|
| msg227653 - (view) |
Author: Antony Lee (Antony.Lee) * |
Date: 2014-09-26 21:35 |
| 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). |
|
|
| msg227659 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2014-09-26 21:54 |
| 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)']. |
|
|
| msg227662 - (view) |
Author: Antony Lee (Antony.Lee) * |
Date: 2014-09-26 22:08 |
| I don't feel strongly about this, so either way is fine. |
|
|
| msg227665 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-09-27 08:38 |
| Note that abspath() can return incorrect result in case of symbolic links to directories and pardir components. I.e. abspath('symlink/..'). |
|
|
| msg227701 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2014-09-27 16:46 |
| > 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? |
|
|
| msg228459 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-10-04 13:45 |
| > 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() |
|
|
| msg228513 - (view) |
Author: Antony Lee (Antony.Lee) * |
Date: 2014-10-05 00:20 |
| 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). |
|
|