Merge pull request #1266 from jodal/fix/1259-respect-core-dir-configs

Fix #1259 by respecting core dir configs

docs/changelog.rst

@@ -14,6 +14,27 @@ Bug fix release.
   backends with a library provider. Previously, it wrongly worked for all
   backends with a playlists provider. (Fixes: :issue:`1257`)
 
+- Core: Respect :confval:`core/cache_dir` and :confval:`core/data_dir` config
+  values added in 1.1.0 when creating the dirs Mopidy need to store data. This
+  should not change the behavior for desktop users running Mopidy. When running
+  Mopidy as a system service installed from a package which sets the core dir
+  configs properly (e.g. Debian and Arch packages), this fix avoids the
+  creation of a couple of directories that should not be used, typically
+  :file:`/var/lib/mopidy/.local` and :file:`/var/lib/mopidy/.cache`. (Fixes:
+  :issue:`1259`, PR: :issue:`1266`)
+
+- Local: Deprecate :confval:`local/data_dir` and respect
+  :confval:`core/data_dir` instead. This does not change the defaults for
+  desktop users, only system services installed from packages that properly set
+  :confval:`core/data_dir`, like the Debian and Arch packages. (Fixes:
+  :issue:`1259`, PR: :issue:`1266`)
+
+- M3U: Changed default for the :confval:`m3u/playlists_dir` from
+  ``$XDG_DATA_DIR/mopidy/m3u`` to unset, which now means the extension's data
+  dir. This does not change the defaults for desktop users, only system
+  services installed from packages that properly set :confval:`core/data_dir`,
+  like the Debian and Arch pakages. (Fixes: :issue:`1259`, PR: :issue:`1266`)
+
 - Stream: If "file" is present in the :confval:`stream/protocols` config value
   and the :ref:`ext-file` extension is enabled, we exited with an error because
   two extensions claimed the same URI scheme. We now log a warning recommending

docs/ext/local.rst

@@ -47,8 +47,8 @@ active at a time.
 To create a new library provider you must create class that implements the
 :class:`mopidy.local.Library` interface and install it in the extension
 registry under ``local:library``. Any data that the library needs to store on
-disc should be stored in :confval:`local/data_dir` using the library name as
-part of the filename or directory to avoid any conflicts.
+disc should be stored in the extension's data dir, as returned by
+:meth:`~mopidy.ext.Extension.get_data_dir`.
 
 
 Configuration

docs/ext/m3u.rst

@@ -52,4 +52,5 @@ See :ref:`config` for general help on configuring Mopidy.
 
 .. confval:: m3u/playlists_dir
 
-    Path to directory with M3U files.
+    Path to directory with M3U files. Unset by default, in which case the
+    extension's data dir is used to store playlists.

mopidy/__main__.py

@@ -75,15 +75,16 @@ def main():
 
         args = root_cmd.parse(mopidy_args)
 
-        create_file_structures_and_config(args, extensions_data)
-        check_old_locations()
-
         config, config_errors = config_lib.load(
             args.config_files,
             [d.config_schema for d in extensions_data],
             [d.config_defaults for d in extensions_data],
             args.config_overrides)
 
+        create_core_dirs(config)
+        create_initial_config_file(args, extensions_data)
+        check_old_locations()
+
         verbosity_level = args.base_verbosity_level
         if args.verbosity_level:
             verbosity_level += args.verbosity_level
@@ -166,12 +167,17 @@ def main():
         raise
 
 
-def create_file_structures_and_config(args, extensions_data):
-    path.get_or_create_dir(b'$XDG_DATA_DIR/mopidy')
-    path.get_or_create_dir(b'$XDG_CONFIG_DIR/mopidy')
+def create_core_dirs(config):
+    path.get_or_create_dir(config['core']['cache_dir'])
+    path.get_or_create_dir(config['core']['config_dir'])
+    path.get_or_create_dir(config['core']['data_dir'])
+
+
+def create_initial_config_file(args, extensions_data):
+    """Initialize whatever the last config file is with defaults"""
 
-    # Initialize whatever the last config file is with defaults
     config_file = args.config_files[-1]
+
     if os.path.exists(path.expand_path(config_file)):
         return
 

mopidy/local/__init__.py

@@ -23,7 +23,7 @@ class Extension(ext.Extension):
         schema = super(Extension, self).get_config_schema()
         schema['library'] = config.String()
         schema['media_dir'] = config.Path()
-        schema['data_dir'] = config.Path()
+        schema['data_dir'] = config.Deprecated()
         schema['playlists_dir'] = config.Deprecated()
         schema['tag_cache_file'] = config.Deprecated()
         schema['scan_timeout'] = config.Integer(

mopidy/local/ext.conf

@@ -2,7 +2,6 @@
 enabled = true
 library = json
 media_dir = $XDG_MUSIC_DIR
-data_dir = $XDG_DATA_DIR/mopidy/local
 scan_timeout = 1000
 scan_flush_threshold = 1000
 scan_follow_symlinks = false

mopidy/local/json.py

@@ -12,7 +12,7 @@ import tempfile
 import mopidy
 from mopidy import compat, local, models
 from mopidy.internal import encoding, timer
-from mopidy.local import search, storage, translator
+from mopidy.local import Extension, search, storage, translator
 
 logger = logging.getLogger(__name__)
 
@@ -116,7 +116,7 @@ class JsonLibrary(local.Library):
         self._browse_cache = None
         self._media_dir = config['local']['media_dir']
         self._json_file = os.path.join(
-            config['local']['data_dir'], b'library.json.gz')
+            Extension().get_data_dir(config), b'library.json.gz')
 
         storage.check_dirs_and_files(config)
 

mopidy/local/storage.py

@@ -3,8 +3,6 @@ from __future__ import absolute_import, unicode_literals
 import logging
 import os
 
-from mopidy.internal import encoding, path
-
 logger = logging.getLogger(__name__)
 
 
@@ -13,10 +11,3 @@ def check_dirs_and_files(config):
         logger.warning(
             'Local media dir %s does not exist.' %
             config['local']['media_dir'])
-
-    try:
-        path.get_or_create_dir(config['local']['data_dir'])
-    except EnvironmentError as error:
-        logger.warning(
-            'Could not create local data dir: %s',
-            encoding.locale_decode(error))

mopidy/m3u/__init__.py

@@ -21,7 +21,7 @@ class Extension(ext.Extension):
 
     def get_config_schema(self):
         schema = super(Extension, self).get_config_schema()
-        schema['playlists_dir'] = config.Path()
+        schema['playlists_dir'] = config.Path(optional=True)
         return schema
 
     def setup(self, registry):

mopidy/m3u/actor.py

@@ -4,7 +4,7 @@ import logging
 
 import pykka
 
-from mopidy import backend
+from mopidy import backend, m3u
 from mopidy.internal import encoding, path
 from mopidy.m3u.library import M3ULibraryProvider
 from mopidy.m3u.playlists import M3UPlaylistsProvider
@@ -21,12 +21,16 @@ class M3UBackend(pykka.ThreadingActor, backend.Backend):
 
         self._config = config
 
-        try:
-            path.get_or_create_dir(config['m3u']['playlists_dir'])
-        except EnvironmentError as error:
-            logger.warning(
-                'Could not create M3U playlists dir: %s',
-                encoding.locale_decode(error))
+        if config['m3u']['playlists_dir'] is not None:
+            self._playlists_dir = config['m3u']['playlists_dir']
+            try:
+                path.get_or_create_dir(self._playlists_dir)
+            except EnvironmentError as error:
+                logger.warning(
+                    'Could not create M3U playlists dir: %s',
+                    encoding.locale_decode(error))
+        else:
+            self._playlists_dir = m3u.Extension().get_data_dir(config)
 
         self.playlists = M3UPlaylistsProvider(backend=self)
         self.library = M3ULibraryProvider(backend=self)

mopidy/m3u/ext.conf

@@ -1,3 +1,3 @@
 [m3u]
 enabled = true
-playlists_dir = $XDG_DATA_DIR/mopidy/m3u
+playlists_dir =

mopidy/m3u/playlists.py

@@ -23,7 +23,7 @@ class M3UPlaylistsProvider(backend.PlaylistsProvider):
     def __init__(self, *args, **kwargs):
         super(M3UPlaylistsProvider, self).__init__(*args, **kwargs)
 
-        self._playlists_dir = self.backend._config['m3u']['playlists_dir']
+        self._playlists_dir = self.backend._playlists_dir
         self._playlists = {}
         self.refresh()
 

tests/local/test_json.py

@@ -45,10 +45,11 @@ class BrowseCacheTest(unittest.TestCase):
 class JsonLibraryTest(unittest.TestCase):
 
     config = {
+        'core': {
+            'data_dir': path_to_data_dir(''),
+        },
         'local': {
             'media_dir': path_to_data_dir(''),
-            'data_dir': path_to_data_dir(''),
-            'playlists_dir': b'',
             'library': 'json',
         },
     }

tests/local/test_library.py

@@ -65,10 +65,11 @@ class LocalLibraryProviderTest(unittest.TestCase):
     ]
 
     config = {
+        'core': {
+            'data_dir': path_to_data_dir(''),
+        },
         'local': {
             'media_dir': path_to_data_dir(''),
-            'data_dir': path_to_data_dir(''),
-            'playlists_dir': b'',
             'library': 'json',
         },
     }
@@ -105,11 +106,15 @@ class LocalLibraryProviderTest(unittest.TestCase):
 
         tmpdir = tempfile.mkdtemp()
         try:
-            tmplib = os.path.join(tmpdir, 'library.json.gz')
-            shutil.copy(path_to_data_dir('library.json.gz'), tmplib)
+            tmpdir_local = os.path.join(tmpdir, 'local')
+            shutil.copytree(path_to_data_dir('local'), tmpdir_local)
 
-            config = {'local': self.config['local'].copy()}
-            config['local']['data_dir'] = tmpdir
+            config = {
+                'core': {
+                    'data_dir': tmpdir,
+                },
+                'local': self.config['local'],
+            }
             backend = actor.LocalBackend(config=config, audio=None)
 
             # Sanity check that value is in the library
@@ -117,6 +122,7 @@ class LocalLibraryProviderTest(unittest.TestCase):
             self.assertEqual(result, self.tracks[0:1])
 
             # Clear and refresh.
+            tmplib = os.path.join(tmpdir_local, 'library.json.gz')
             open(tmplib, 'w').close()
             backend.library.refresh()
 

tests/local/test_playback.py

@@ -23,12 +23,11 @@ from tests.local import generate_song, populate_tracklist
 class LocalPlaybackProviderTest(unittest.TestCase):
     config = {
         'core': {
+            'data_dir': path_to_data_dir(''),
             'max_tracklist_length': 10000,
         },
         'local': {
             'media_dir': path_to_data_dir(''),
-            'data_dir': path_to_data_dir(''),
-            'playlists_dir': b'',
             'library': 'json',
         }
     }

tests/local/test_tracklist.py

@@ -18,11 +18,11 @@ from tests.local import generate_song, populate_tracklist
 class LocalTracklistProviderTest(unittest.TestCase):
     config = {
         'core': {
+            'data_dir': path_to_data_dir(''),
             'max_tracklist_length': 10000
         },
         'local': {
             'media_dir': path_to_data_dir(''),
-            'data_dir': path_to_data_dir(''),
             'playlists_dir': b'',
             'library': 'json',
         }