821362 - migrate NamedTemporaryFile improvement to mozfile

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

A python stdlib method doesn't work on windows!  Wonderful!

Luckily we have a great place to put these hacks: mozfile.  Let's
throw it there.

For context, see:

https://bugzilla.mozilla.org/show_bug.cgi?id=818777
https://bug818777.bugzilla.mozilla.org/attachment.cgi?id=691701

Comment 1

[Jeff Hammel]

Attachment: port from https://bug818777.bugzilla.mozilla.org/attachment.cgi?id=691701

the unittest is a joke but I figured better than no test at all (it actually helped me catch a missing import of tempfile, fwiw).

I also corrected the rmtree test name, since it actually was never run before!

Comment 2

[Gregory Szorc [:gps]]

Be very careful with the naive implementation. I purposefully only put it in mozunit because it doesn't behave exactly like the stdlib does. Specifically, it doesn't do things like prevent fd inheritance by children, etc. Use at your own risk.

Comment 3

[William Lachance (:wlach)]

Comment on attachment 691884 [details] [diff] [review]
port from https://bug818777.bugzilla.mozilla.org/attachment.cgi?id=691701

I'm giving this an r- just because I'd like to see better documentation. I don't want people to blindly start using this function everywhere without having a realistic view of its pros and cons.

>+class NamedTemporaryFile(object):
>+    """Like tempfile.NamedTemporaryFile except it works on Windows.

This comment isn't quite true, from what I can gather from the original bug: NamedTemporaryFile *does* work on Windows, you just can't open the created file a second time.

>+    This behaves very similarly to tempfile.NamedTemporaryFile but may
>+    not behave exactly the same.

We should be more explicit about the limitations of this function here (e.g. what :gps said about it not preventing fd inheritance by children)

>+    Example usage:
>+
>+    with NamedTemporaryFile() as fh:
>+        fh.write(b'foobar')
>+
>+        print('Filename: %s' % fh.name)
>+
>+    see https://bugzilla.mozilla.org/show_bug.cgi?id=821362
>+    """

Comment 4

[Jeff Hammel]

So...I didn't write this class or docstring!  In fact the only change I made from the code already checked in on m-c is noting the bug # in the docstring.  I'll edit the patch to try to intuit what :gps meant to say but its not really my forte.

Comment 5

[Jeff Hammel]

Attachment: docstring changes

If this isn't good enough, I recommend :wlach or :gps submit an appropriate patch with a more pallatable docstring.  My main goal is to get this into mozfile, release a new mozfile, mirror to m-c, and get mozbuild/mach to use this code so that we have one place to fix things.

Comment 6

[William Lachance (:wlach)]

Comment on attachment 692067 [details] [diff] [review]
docstring changes

This is better, thanks! I just have one nit about the phrasing in the comments. r+ with that addressed.

>diff --git a/mozfile/mozfile/mozfile.py b/mozfile/mozfile/mozfile.py

>+    This behaves very similarly to tempfile.NamedTemporaryFile but may
>+    not behave exactly the same.  This does not prevent fd inheritence
>+    by children.

I would reword this last sentence to:

"For example, this function does not prevent fd inheritance by children."

I think that makes it a little more clear that this may not be the only difference.

Comment 7

[Jeff Hammel]

> I would reword this last sentence to:
> 
> "For example, this function does not prevent fd inheritance by children."
> 
> I think that makes it a little more clear that this may not be the only difference.

SGTM

Comment 8

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/24096932cf4a56556e455649f6143fc848b0649f