818777 - Wean off tempfile.NamedTemporaryFile

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Guess what! tempfile.NamedTemporaryFile doesn't work on Windows (the file can't be opened a second time)!

http://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile

I wish Python had a mode where it disabled functions with known cross-platform issues so you couldn't foot gun yourself like this.

I've been using NamedTemporaryFile a lot in unit tests in /python/{mach,mozbuild}. We will need to replace them with something else. I feel dumb for not seeing this warning or running the tests on Windows earlier to discover it.

Ted/Jeff: I'd like your opinions on what to do here. One way or another I see myself implementing an equivalent of NamedTemporaryFile via tempfile.mkstemp(). Since I use NamedTemporaryFile primarily (solely?) in unit tests today, I could add this to our unittest base class for a quick and dirty fix. Long term, I foresee a need for a "toolbox" of generic Python utility classes and functions shared across the tree. That sounds an awful lot like mozbase. However, I don't want to have to support Python < 2.7 and there are "urgency" issues with mozbase (mainly waiting for uplifts to m-c). Should I bite the bullet and land these things in mozbase. Or, should I create something like /python/moz{util,tools,toolbox} and start amassing things there? Or, are there other options?

Comment 1

[Gregory Szorc [:gps]]

I suppose I could also use MockedOpen from config/unittest.py as an even quicker dirty solution. Although, I worry that MockedFile doesn't implement all the intricacies of "real" I/O (I've been bit on things like encodings and subtle cross-platform differences plenty of times). I worry how MockedOpen globally replaces open() for everything, not just the mocked paths.

I'm not looking for excuses to not use MockedOpen - it's just that any time you mock something you are playing with fire. Globally mocking I/O scares me, honestly.

Comment 2

[Jeff Hammel]

My blind advise is to use tempfile.mkstemp() (assumes that is windows.kosher).  There is a package, mozfile, that is meant for just this sort of thing if you do write a wrapper: https://github.com/mozilla/mozbase/tree/master/mozfile .

Comment 3

[(not currently active) Ted Mielczarek]

I think there are two other ways to solve this:
1) os.close the fd returnded from NamedTemporaryFile. That basically nulls out the security portion of using it and makes it no better than tempfile.mkstemp() (except it will probably auto-clean-up for you).
2) Just use tempfile.mkdtemp() and create files inside that temporary directory. Somewhere I wrote a tiny little TempDirectory contextmanager class, but I can't find it right now. If this is just for unit tests it's simple enough to do mkdtemp in setUp and shutil.rmtree it in tearDown.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Reimplement NamedTemporaryFile, v1

This /seems/ to work. There are some corner cases it won't handle. But, I think it's good enough to be part of just the tests. Most importantly, it causes tests to pass on Windows.

See http://hg.python.org/cpython/file/5435a9278028/Lib/tempfile.py for reference.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 691701 [details] [diff] [review]
Reimplement NamedTemporaryFile, v1

Review of attachment 691701 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/test/compilation/test_warnings.py
@@ -225,3 @@
>          # to go away.
>          old_filename = source_files[0].name
> -        source_files[0].close()

Is there any reason not to make your NamedTemporaryFile.close() do the unlink just like the stdlib version?

Comment 6

[Jeff Hammel]

I've ticketed adding this to mozbase: https://bugzilla.mozilla.org/show_bug.cgi?id=821362 ; once it lands there we should do the whole upstreamin' tango to get this on m-c and being used

Comment 7

[Gregory Szorc [:gps]]

jhammel and friends ported NamedTemporaryFile to mozfile. So, I refactored the patch to use mozfile and I removed NamedTemporaryFile from mozunit. I'm fairly confident this DRY violation is within bounds of the original r+.

https://hg.mozilla.org/projects/build-system/rev/fac2900af6a0

I'll file a follow-up to have mozfile implement .close().

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/fac2900af6a0