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

XDG_CONFIG_HOME, if set, should have priority over ~/.config and other platform-specific variants #98

Open
rpdelaney opened this issue Jun 11, 2020 · 6 comments

Comments

@rpdelaney
Copy link

rpdelaney commented Jun 11, 2020

Currently, we look for ~/.config/beets/config.yaml (on linux) and if it is not found, fall back to $XDG_CONFIG_HOME/beets/config.yaml. This priority is inverted.

From the freedesktop.org base directory specification:

There is a single base directory relative to which user-specific configuration files should be written. This directory is defined by the environment variable $XDG_CONFIG_HOME.
...
$XDG_CONFIG_HOME defines the base directory relative to which user specific configuration files should be stored. If [and only if] $XDG_CONFIG_HOME is either not set or empty, [then] a default equal to $HOME/.config should be used.

emphasis added

If a PR making this change would be accepted I can find time to put it together.

Also, it might be nice to make use of the appdirs module to offload some of the logic where the platform directory paths are hard-coded.

@sampsyo
Copy link
Member

sampsyo commented Jun 12, 2020

Thanks for pointing that out; it does seem like we should invert this.

One thing to be careful about when making the switch is that this prioritization also affects the directory that will be created if none exist yet. Of course, this should also probably also use $XDG_CONFIG_HOME—but it's worth just being careful that we don't have unintended consequences in this case.

appdirs does seem cool if it simplifies things! But I guess it remains to be seen how much code it really saves.

@rpdelaney
Copy link
Author

Ok, I'll take that as a green light to work something up in a fork.

@rpdelaney
Copy link
Author

rpdelaney commented Jun 17, 2020

I'm working in a local fork on migrating to appdirs, since redoing the priority logic as-is will be wasted effort if also migrating to appdirs and I wanted to see what the benefits might look like. I think it can make a bunch of this code for config directory discovery much simpler. For example, config_dirs() could possibly become just this or close to it:

def config_dirs():
    return [appdirs.user_config_dir(), appdirs.site_config_dir()]

However, I'm having difficulty with some of the tests failing. It's not helpful that I'm not really familiar with how to run tox locally. I'm trying to make do with pytest but that's clearly not how these tests were intended to be invoked. The tests already fail on master: I'm getting stuff like this:

AssertionError: Lists differ: ['\\home\\ryan\\src\\me\\beetbox-confuse\\~\\AppData\\Roaming'] != ['C:\\Users\\test\\AppData\\Roaming']

...which doesn't make much sense.

Running tox by itself failed hard since it couldn't find the interpreter versions it needs:

GLOB sdist-make: /home/ryan/src/me/beetbox-confuse/setup.py
py27-test inst-nodeps: /home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip
py27-test installed: DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 is no longer maintained. pip 21.0 will drop support for Python 2.7 in January 2021. More details about Python 2 support in pip, can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support,confuse @ file:///home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip,coverage==5.1,nose==1.3.7,nose-show-skipped==0.1,pathlib==1.0.1,PyYAML==5.3.1
py27-test run-test-pre: PYTHONHASHSEED='2247740019'
py27-test run-test: commands[0] | nosetests
.....................................................S..............................................................................SS.........................................................................
----------------------------------------------------------------------
Ran 207 tests in 0.110s

OK (SKIP=3)
py34-test create: /home/ryan/src/me/beetbox-confuse/.tox/py34-test
ERROR: InterpreterNotFound: python3.4
py35-test create: /home/ryan/src/me/beetbox-confuse/.tox/py35-test
ERROR: InterpreterNotFound: python3.5
py36-test create: /home/ryan/src/me/beetbox-confuse/.tox/py36-test
ERROR: InterpreterNotFound: python3.6
py27-flake8 recreate: /home/ryan/src/me/beetbox-confuse/.tox/py27-flake8
py27-flake8 installdeps: flake8, flake8-future-import, pep8-naming
py27-flake8 inst: /home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip
py27-flake8 installed: DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 is no longer maintained. pip 21.0 will drop support for Python 2.7 in January 2021. More details about Python 2 support in pip, can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support,configparser==4.0.2,confuse @ file:///home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip,contextlib2==0.6.0.post1,enum34==1.1.10,flake8==3.8.3,flake8-future-import==0.4.6,flake8-polyfill==1.0.2,functools32==3.2.3.post2,importlib-metadata==1.6.1,mccabe==0.6.1,pathlib2==2.3.5,pep8-naming==0.11.1,pycodestyle==2.6.0,pyflakes==2.2.0,PyYAML==5.3.1,scandir==1.10.0,six==1.15.0,typing==3.7.4.1,zipp==1.2.0
py27-flake8 run-test-pre: PYTHONHASHSEED='2247740019'
py27-flake8 run-test: commands[0] | flake8 --min-version 2.7 example confuse.py test setup.py docs
docs inst-nodeps: /home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip
docs installed: DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 is no longer maintained. pip 21.0 will drop support for Python 2.7 in January 2021. More details about Python 2 support in pip, can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support,-onfuse @ file:///home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip,alabaster==0.7.12,Babel==2.8.0,certifi==2020.4.5.2,chardet==3.0.4,confuse @ file:///home/ryan/src/me/beetbox-confuse/.tox/.tmp/package/1/confuse-1.1.0.zip,docutils==0.16,idna==2.9,imagesize==1.2.0,Jinja2==2.11.2,MarkupSafe==1.1.1,packaging==20.4,Pygments==2.5.2,pyparsing==2.4.7,pytz==2020.1,PyYAML==5.3.1,requests==2.23.0,six==1.15.0,snowballstemmer==2.0.0,Sphinx==1.8.5,sphinxcontrib-websupport==1.1.2,typing==3.7.4.1,urllib3==1.25.9
docs run-test-pre: PYTHONHASHSEED='2247740019'
docs run-test: commands[0] | sphinx-build -W -q -b html docs /home/ryan/src/me/beetbox-confuse/.tox/docs/tmp/html
___________________________________ summary ____________________________________
  py27-test: commands succeeded
ERROR:  py34-test: InterpreterNotFound: python3.4
ERROR:  py35-test: InterpreterNotFound: python3.5
ERROR:  py36-test: InterpreterNotFound: python3.6
  py27-flake8: commands succeeded
  docs: commands succeeded

If you have any pointers on how I could get spun up on tox (especially if it's different with Linux) I'd surely appreciate it. I'll look up some tutorials as I get time, but it's a blocker for now.

@sampsyo
Copy link
Member

sampsyo commented Jun 17, 2020

Hello! Tox wants to run the tests under different versions of Python by default, so what it's telling you here is that you need to first install that version of Python. However, if you just want to run a specific version of Python, try something like tox -e py27 or tox -e py38.

I unfortunately don't immediately see the problem with the test that's leading to that failure.

More broadly, given what you've shown so far, I'm also not 100% convinced that depending on appdirs is really worth it… if it really only saves us a ~10-line function and some hard-coded environment variable names, taking an entirely new dependency seems like too steep a price. I think appdirs looks awesome for programs that need to "manually" figure out their own configuration, but for Confuse, it's not that hard to provide (and precisely tune!) that behavior ourselves. I'm also concerned about behavior changes: the simpler version above searches and merges configurations from fewer directories than our current implementation.

So maybe let's hold off on appdirs, at least for now, and focus on fixing the ordering problem directly?

@rpdelaney
Copy link
Author

I probably shouldn't have mixed the threads, because it gave a wrong impression that there is nothing else to be gained from using appdirs than that one example. The thing blocking me working is that I can't get the tests to pass locally on master without any changes. I only have python 3.8.3 installed, and there is no corresponding tox environment configured here. I don't know a way to have various interpreters running concurrently except pyenv, but that's a bit of a can of worms.

@sampsyo
Copy link
Member

sampsyo commented Jun 18, 2020

Hmm… are you sure tox -e py38 doesn't just work? It does here, and I think it doesn't need any extra configuration…

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

No branches or pull requests

2 participants