What to do when ~/.astropy is not writeable - #123

~/.astropy needs to be created upon first import... it should not be part of setup.py at all. Think of the use case of installing in a central location on a multi-user machine. It should resolve this situation, too, unless i'm missing something.

The trouble is deciding when "first import" is - after all, there's an import astropy in setup.py. The way it works now is that it is created at first import, if it isn't found already... its just that that first import is somewhere in the setup script (I'm not sure actually why it's doing it in build, but it doesn't necessarily surprise me).

My idea was that for multi-user machines, it will just always look for whatever the correct home is for the current user who's actually running that python process... as it stands right now, any .astropy directory created by the root user at install will not be used when some other user imports astropy - it will just create a new /home/whateveruser/.astropy directory.

So this could be changed to check for if it's in setup, and if so, not do any of the creating. I think that will break some of the tests, though... Alternatively, we could just have it fail gracefully if it can't access the .astropy directory, and just assume all defaults for the configuration and only fail if someone tries to use the cache stuff (which of course requires a writeable directory).

I think that whatever we do, if ~/.astropy is not writeable, astropy should still import, but with a warning. I don't think an exception should be raised (astropy can still function with the default config).

Yep, that definitely makes sense for config files... But what about accessing the cache? One thing that could be done might be to use the NamedTemporaryFile mechanism as a fallback if the cache can't be written to (remember that we sometimes need a file name), but to make that work we wouldn't be able to delete the temporary files through astropy (the user would have to know to clear their temp dir). Or we could just have it throw an exception whenever someone asks for an actual filename...

@eteq: Maybe we could have an atexit handler that removes the temporary cache directory if one was created.

@eteq: I think what you propose 3 comments up here seems reasonable.

Why are the tests relying on ~/.astropy in the first place?

I would say that import astropy from setup.py shouldn't rely on ~/.astropy at all--I thought we had a global variable for "in setup.py". The tests should use some temp dir in place of ~/.astropy, right? I've been a little out of the loop the last few weeks though, so I'm not sure if there's a problem with that...

The tests rely on ~/.astropy because that's how the real system works - and if the tests are supposed to make sure the correct astropy directory can be found, they need to actually go through all the motions. Remember that locating the "home" directory can be surprisingly non-trivial on some platforms... I suppose the test could be made to only use a temporary directory, but to me it seems like that's not actually testing the right functionality. And anyway, this is independent of that because one could imagine a scenario where the home directory is for some reason inacessible in normal use instead of just when the tests are being run (say the ~/.astropy directory is a symlink to a network drive or something).

I still think that in any case, related to the original ticket, astropy should never fail to import if ~/.astropy is not accessible, whether in setup.py or afterwards. I don't want the import to fail if as you said, the home directory is temporarily unavailable for e.g. network reasons.

Agreed - and the code I just attached does exactly that - if ~/.astropy is unavailable, ConfigurationItems will just use their default (unless set in user code), and remote data downloads will go to a temporary file. In both cases, warnings are given that the astropy directory is inacessible. In the latter case, the temporary file is also included in the warning so the user can go and delete it later on if they so desire.

I guess what I'm trying to say is that if ~/.astropy cannot be written/read, it is not crucial to astropy and should not raise an exception. In that case, the default config would be used, and no caching would take place.

@eteq - ok, that sounds sensible. I sent my last comment before seeing yours.

Ok, sounds good - I'll give it a few days for review, and assuming there aren't any big problems I'll merge and close the issue.

When the tests do use ~/.astropy they don't disturb anything in there though, do they? Wouldn't want that once people are actually using this and have files and configurations in there that shouldn't be disturbed.

I understand the argument that it is hard to test the configuration system itself without actually accessing the configuration files. However, I think, as @iguananaut suggested, we shouldn't touch those files during setup. I wouldn't expect setup.py build to create anything outside of the build directory, and Linux packaging systems, for example, generally enforce this policy.

There is the other issue that we don't want user's settings to impact the tests themselves. We had this issue in matplotlib. Imagine a setting that turns warnings into errors for example (one such setting already exists in astropy.io.vo). What matplotlib does there is use all default settings during test runs, and write tests that explicitly change some config values in order to test the effect of those config values. It should be possible to set a flag in conftest.py that would prevent the loading of configuration from files.

As for testing configuration itself, it should be possible to set $HOME to somewhere under tmp, check that it creates files there correctly etc. without touching the real home directory and without having settings that impact other tests in the test suite.

I agree with @mdboom's suggestion of setting $HOME to a tmp directory for the tests. There could (and should) still be a test or tests for the function that finds the user's $HOME (the result of such a test would depend entirely on which platform it's run on, but that's fine too).

@iguananaut and @mdboom - I see your point here - and pytest makes this pretty easy, actually, with the monkeypatch funcarg - if you look at the tests I added in this pull request, both new functions have a signature that includes monkeypatch, and then at the beginning I do monkeypatch.setenv('XDG_CONFIG_HOME', 'foo'). That sets the environment variable for that test, and then reverts it to what it should be for remaining tests. Should we do that, or instead just set use the test command or a py.test plugin to set $HOME before any of the tests run?

Also, perhaps one of you should create this as a separate issue/pull request (or I can - I just want to make sure you aren't attached to it being in this particular PR)? I think we want this pull request regardless, and while related, I think having an inaccessible .astropy fall back to defaults and having the tests run separately are slightly different issues.

@astrofrog - can you confirm that with the changes in this PR, the original issue is actually fixed?

It seems to work fine, and I'm getting two test failures:

http://paste.pocoo.org/show/539457
http://paste.pocoo.org/show/539458

The first one is expected, but is the second one normal?

The second one is happening because if it can't access the astropy directory, it leaves the ConfigObj filename as None (the default). So it's also expected behavior if the configuration directory is inaccessible.

So what do you think is the best way to deal with this? I could just add extra clauses to ignore OSErrors that include "permission denied" for the first one, and the second I could just check for None and have it do nothing if its None. But that could potentially mask future bugs, I suppose. Or I could just merge this as-is, as the suggestions of @mdboom and @iguananaut would make this problem irrelevant for you once we implemented that (a temporary directory for the config files, that is).

Ok, I agree that the second failed test is not a big issue, so as far as I'm concerned, this is fine to merge for now.