Skip to content

Conversation

@cgohlke
Copy link
Contributor

@cgohlke cgohlke commented Oct 20, 2015

On Windows, a open NamedTemporaryFile can not be used to open the file a second time.

Fixes one test error:

======================================================================
ERROR: matplotlib.tests.test_font_manager.test_json_serialization
----------------------------------------------------------------------
Traceback (most recent call last):
  File "X:\Python35\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "X:\Python35\lib\site-packages\matplotlib\tests\test_font_manager.py", line 27, in test_json_serialization
    json_dump(fontManager, temp.name)
  File "X:\Python35\lib\site-packages\matplotlib\font_manager.py", line 978, in json_dump
    with open(filename, 'w') as fh:
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\gohlke\\AppData\\Local\\Temp\\tmpc1j8d7dc'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From docs:

If delete is true (the default), the file is deleted as soon as it is closed.

That sounds like the temporary file would be cleaned up here. Then the write in the next line will be to a file that isn't cleaned up. Or does the context manager try to delete the file again even though it's been closed already?

Since json_load/json_dump are new, maybe they should be modified to take a file object instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on modifying json_load/json_dump.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really care how to resolve this, but we should confirm that this doesn't leave a file hanging around on disk, which is what I think this change does.

@mdboom mdboom added this to the next point release (1.5.0) milestone Oct 22, 2015
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in a try/finally so it always runs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh... we are making this overly complicated.

try:
    temp = tempfile.NamedTemporaryFile(delete=False)
    temp.close()
    json_dump(fontManager, temp.name)
    copy = json_load(temp.name)
finally:
    if os.path.exists(temp.name):
        os.remove(temp.name)

@mdboom
Copy link
Member

mdboom commented Oct 22, 2015

@cgohlke: Did you close this by accident?

@jankatins
Copy link
Contributor

Included a version of this PR in #5604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants