Make _is_writable_dir more flexible to obscure failure modes

[embray]

This is pretty obscure, but is something I've observed in testing out some installation issues.

When setuptools installs a package downloaded from a package index using easy_install--in particular if that package is listed in the setup_requires list of build-time requirements for another package, it uses a function called run_setup to run the setup.py script for the downloaded package. It runs the setup.py in the context of a DirectorySandbox which prevents the setup.py from writing files outside of the temporary directory set up for building the package.

If, in this case, the setup_requires package happens to import matplotlib and it goes down the right code path that his function is called, it will result in a SandboxViolation because matplotlib tried to write a file outside the sandbox. Unfortunately this is not an OSError, so the exception is raised rather than simply returning False is it should (indeed, the directory is not writable, so no attempt should be made to write a matplotlibrc file).

I think that if matplotlib is going to attempt to write files to the filesystem during import time this function should be prepared for any kind of obscure error condition that could result :)

[WeatherGod]

Actually, while we are at it... I have run into rare issues where the home
partition would be filled up by some other user (unquota'ed partitions,
grrrr), and then import of matplotlib would fail because it was trying to
test-write some file (a matplotlibrc, maybe?), even though it didn't need
to do that. I never tracked down the exact source of the problem, but it is
a weakness somewhere in the imports. Yes, the font-cache already existed at
the time.

On Wed, Feb 25, 2015 at 12:48 PM, Erik Bray notifications@github.com
wrote:

This is pretty obscure, but is something I've observed in testing out some
installation issues.

When setuptools installs a package downloaded from a package index using
easy_install--in particular if that package is listed in the
setup_requires list of build-time requirements for another package, it
uses a function called run_setup
https://bitbucket.org/pypa/setuptools/src/e50d57c3e4ed359316dc2ddce195ecfebe0784a9/setuptools/sandbox.py?at=default#cl-224
to run the setup.py script for the downloaded package. It runs the
setup.py in the context of a DirectorySandbox
https://bitbucket.org/pypa/setuptools/src/e50d57c3e4ed359316dc2ddce195ecfebe0784a9/setuptools/sandbox.py?at=default#cl-361
which prevents the setup.py from writing files outside of the temporary
directory set up for building the package.

If, in this case, the setup_requires package happens to import matplotlib
and it goes down the right code path that his function is called, it will
result in a SandboxViolation because matplotlib tried to write a file
outside the sandbox. Unfortunately this is not an OSError, so the
exception is raised rather than simply returning False is it should
(indeed, the directory is not writable, so no attempt should be made to
write a matplotlibrc file).

I think that if matplotlib is going to attempt to write files to the
filesystem during import time this function should be prepared for any kind

of obscure error condition that could result :)

You can view, comment on, or merge this pull request online at:

#4165
Commit Summary

• Make _is_writable_dir more flexible to obscure failure modes

File Changes

• M lib/matplotlib/init.py
  https://github.com/matplotlib/matplotlib/pull/4165/files#diff-0 (2)

Patch Links:

• https://github.com/matplotlib/matplotlib/pull/4165.patch
• https://github.com/matplotlib/matplotlib/pull/4165.diff

—
Reply to this email directly or view it on GitHub
#4165.

[embray]

Probably the same issue. I'm a little surprised that case wouldn't result in an OSError but it's not inconceivable either.

[mdboom]

Seems fine to me. @tacaswell

[efiring]

In the case where it is not an OSError, maybe it would be worth printing out the exception and then proceeding.

[embray]

In fact it does seem like this issue goes a bit deeper:

matplotlib/lib/matplotlib/__init__.py

Line 606 in 1fe5b9c

if _is_writable_dir(p):

This gets called every time matplotlib is imported, even if the config dir already exists and the matplotlibrc already exists. It seems to me it shouldn't be doing this _is_wrtitable_dir test until and unless it has determined it actually needs to write into that directory.

[tacaswell]

This is fine with me as is, but 👍 on printing exception in strange cases.

I am ambivalent on minor changes in the logic in __init__.py. I'm sure it can be improved, but it is probably also worth looking at a total overhaul/refactor as it is kind of terrifying that we have 1.4k lines of code in our init.

[efiring]

@tacaswell, Agreed--init.py is due for a close look. Related to that, it would be good to have a routine startup time benchmark test. For some applications startup time really matters. Part of it is out of our control (mainly numpy), but much of it isn't. A few years ago we were paying attention to this--@mdboom, I'm sure you remember.

[embray]

FWIW I was just testing this myself and found the time for import matplotlib.pyplot, for example, to be pretty negligible. I'm sure it could be improved, but it seems way better than import astropy :) On the other hand touching a temp file on every import seems unnecessary and I don't like system-level side-effects of my Python module imports (although in some cases it makes sense to so I'm not saying don't do it at all).

[efiring]

@embray, would you add a comment to the code, so that a later reader will know why a universal except: is being used, and/or some output for the non OSError case? I'm not sure the latter is actually worth the LOC; perhaps you can judge whether it is sufficiently likely to help someone in the future to understand what is going on. If it's not, then I would be happy to merge this with just a single added comment line.

[WeatherGod]

don't forget a "# noqa" flag here. I would also think it would be better to do catch Exception as a bare except catches everything.

[embray]

What would it catch besides a subclass of Exception?

[tacaswell]

On the other hand there is still code out there that raises strange things (like strings), catching everything here seems like the correct thing to do 'Can we write to this dir yes or no?' I don't think we really care why we can't write there.

[WeatherGod]

@embray, there is a subtle difference between a bare exception and catching Exception. A bare exception is basically equivalent to "except BaseException:", which isn't the same thing as "except Exception:". But, @tacaswell has a good point, we want to be able to catch any sort of failure, for whatever reason and just move on.

[embray]

Ah, I didn't there was a separate BaseException under Exception, but all the more reason not to explicitly catch just Exception.

I don't see how could could "raise strings"--that would result in a TypeError.

[tacaswell]

I thought the restriction that you could only raise Exception objects was new-ish and we support back to 2.6 (but I couldn't figure out quickly when that got changed).

[tacaswell]

I thought the restriction that you could only raise Exception objects was new-ish and we support back to 2.6 (but I couldn't figure out quickly when that got changed).

[WeatherGod]

Looks like we are clear on that. Just tried on py2.6.

[centos6] [broot@rd22 gust_checks]$ /usr/bin/python
Python 2.6.6 (r266:84292, Jan 22 2014, 09:42:36)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> raise "SomeString"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: exceptions must be old-style classes or derived from
BaseException, not str
>>>

On Mon, Mar 2, 2015 at 4:43 PM, Thomas A Caswell notifications@github.com
wrote:

In lib/matplotlib/init.py
#4165 (comment):

@@ -225,7 +225,7 @@ def _is_writable_dir(p):
t.write(b'1')
finally:
t.close()

• except OSError:
• except:

I thought the restriction that you could only raise Exception objects was
new-ish and we support back to 2.6 (but I couldn't figure out quickly when
that got changed).

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4165/files#r25641395.

[embray]

@efiring I might find it worth adding a message if I understood why this was being done at all. I guess it has something to do with writing a default matplotlibrc, but how frequently has anyone been worried about that?

[efiring]

Travis is happy; let's move on.