Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matplotlib useable on clusters with read-only resp. non-existing home directories on compute nodes. #2695

Closed
wants to merge 1 commit into from

Conversation

michs
Copy link
Contributor

@michs michs commented Dec 26, 2013

Details are in the comment of the commit.

A corresponding pull request for branch 'master' exixts: #2692

on clusters whith readonly home directories on compute nodes.

The standard allow on Linux to specify configuration and cache directories
independent from the existence of a home directory. The functions
_get_xdg_config_dir() and _get_xdg_cache_dir() respect this now.

Function _get_config_or_cache_dir() allow now to use also on Linux an
existing old style configuration directory "~/.matplotlib". If this does
not exist, the configuration directory will be expected in the location
specified by the XDG_CONFIG_HOME environment variable. If this location
is undefined or it cannot be found resp. created, a temp. configuration
directory will be created in the directory for temp. files.
@tacaswell
Copy link
Member

@michs Does this PR include the fix from the second commit from #2692 ? If so, you should close that PR in favor of this one. The tracking branch is periodically merged into master so fixing in on 1.3.x will also fix it on master.

@michs
Copy link
Contributor Author

michs commented Dec 26, 2013

Hi Thomas, yes the functionality o fboth PR is equivalent, and the implementation of get_xdg*_dir() in branch 1.3 even a bit more efficient. I close the PR for the master branch.

@tacaswell
Copy link
Member

Thanks!

It looks good to me, but I am not familiar with this particular corner of the library or desktop path standards so I am hesitant to merge it.

@michs
Copy link
Contributor Author

michs commented Dec 26, 2013

Just a bit background information for the potential merger/committer.

Here is the link to the Freedesktop standard that I mentioned in the now closed PR. Compare http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html#variables paragraphes 2 and 8.

The other part of the change was motivated by the comment of the function _get_configdir(), especially point 2:

2a. On Linux, if $HOME/.matplotlib exists, choose that, but warn that
that is the old location. Barring that, follow the XDG specification
and look first in $XDG_CONFIG_HOME, if defined, or $HOME/.config.

2b. On other platforms, choose $HOME/.matplotlib.

The comment seems to be a bit contradictionary: choose the old location and warn, and the next sentence says basically ignore it if XDG_CONFIG_HOME has been defined. I think one should choose as specified the old config and warn. This would not lead to a loss of configuration settings because a new config is written into a (potentially empty) directory specified by the environment variable. Due to our discussion here I now think that one even should warn that the existence of the old config and the environment variable at the same time is conflicting.

Alternatively, if we want to stick with behaviour that a XDG_CONFIG_DIR definition has higher priority than to use an existing old config, I can change the pull request so that it only contains what is needed to make it running on the cluster.

@mdboom
Copy link
Member

mdboom commented Dec 30, 2013

Here's the motivation for the current behavior:

Many users have existing configuraiton in ~/.matplotlib. If they have that, we want to continue to use that, but warn that it will be ignored in a future version.

The current XDG-compliant behavior that we are moving towards is that if there is an XDG_CONFIG_HOME variable (which most users don't have defined) use that, otherwise use ~/.config/matplotlib.

I'm not sure what you're saying is contradictory about the comment. We use ~/.matplotlib if we find it (and warn). If we don't find it, we move to what I believe to be the corrent behavior as defined y XDG.

matplotlib never writes configuration files, so I'm not sure what you mean when you say "a new config is written into a (potentially empty) directory specified by the environment variable".

@michs
Copy link
Contributor Author

michs commented Dec 30, 2013

Ok, thanks for the explanations. I agree that we should keep that.

I'm not sure what you mean when you say "a new config is written into a (potentially empty)
directory specified by the environment variable".

The assumption came from the observation that at least the config directory is created during the module import. But right, it was always empty after my tests. I supposed that this was the case because I only tested small things for the purpose of the setup test.

I propose that I change my commit in that way, that I only repair the crash due to the non-existing resp. non-readable home directory in the cluster environment and keep at the same time the current behaviour exactly.

@michs
Copy link
Contributor Author

michs commented Jan 11, 2014

As discussed, this pull request is replaed now by an improved variant #2719 -> closing the request here.

@michs michs closed this Jan 11, 2014
@michs michs deleted the cluster_init branch January 11, 2014 11:54
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.

None yet

3 participants