diff --git a/mopidy/__main__.py b/mopidy/__main__.py index a52c7ccc..cb9e2df8 100644 --- a/mopidy/__main__.py +++ b/mopidy/__main__.py @@ -39,7 +39,7 @@ def main(): loop = gobject.MainLoop() options = parse_options() - config_files = options.config.split(':') + config_files = options.config.split(b':') config_overrides = options.overrides enabled_extensions = [] # Make sure it is defined before the finally block @@ -162,7 +162,7 @@ def parse_options(): parser.add_option( b'--config', action='store', dest='config', - default='/etc/mopidy/mopidy.conf:$XDG_CONFIG_DIR/mopidy/mopidy.conf', + default=b'/etc/mopidy/mopidy.conf:$XDG_CONFIG_DIR/mopidy/mopidy.conf', help='config files to use, colon seperated, later files override') parser.add_option( b'-o', b'--option', @@ -174,7 +174,7 @@ def parse_options(): def show_config_callback(option, opt, value, parser): # TODO: don't use callback for this as --config or -o set after # --show-config will be ignored. - files = getattr(parser.values, 'config', '').split(':') + files = getattr(parser.values, 'config', b'').split(b':') overrides = getattr(parser.values, 'overrides', []) extensions = ext.load_extensions() @@ -196,14 +196,14 @@ def show_config_callback(option, opt, value, parser): def check_old_locations(): - dot_mopidy_dir = path.expand_path('~/.mopidy') + dot_mopidy_dir = path.expand_path(b'~/.mopidy') if os.path.isdir(dot_mopidy_dir): logger.warning( 'Old Mopidy dot dir found at %s. Please migrate your config to ' 'the ini-file based config format. See release notes for further ' 'instructions.', dot_mopidy_dir) - old_settings_file = path.expand_path('$XDG_CONFIG_DIR/mopidy/settings.py') + old_settings_file = path.expand_path(b'$XDG_CONFIG_DIR/mopidy/settings.py') if os.path.isfile(old_settings_file): logger.warning( 'Old Mopidy settings file found at %s. Please migrate your ' @@ -212,8 +212,8 @@ def check_old_locations(): def create_file_structures(): - path.get_or_create_dir('$XDG_DATA_DIR/mopidy') - path.get_or_create_file('$XDG_CONFIG_DIR/mopidy/mopidy.conf') + path.get_or_create_dir(b'$XDG_DATA_DIR/mopidy') + path.get_or_create_file(b'$XDG_CONFIG_DIR/mopidy/mopidy.conf') def setup_audio(config): diff --git a/mopidy/backends/local/actor.py b/mopidy/backends/local/actor.py index 1817e133..8f53af4d 100644 --- a/mopidy/backends/local/actor.py +++ b/mopidy/backends/local/actor.py @@ -1,6 +1,7 @@ from __future__ import unicode_literals import logging +import os import pykka @@ -19,7 +20,7 @@ class LocalBackend(pykka.ThreadingActor, base.Backend): self.config = config - self.create_dirs_and_files() + self.check_dirs_and_files() self.library = LocalLibraryProvider(backend=self) self.playback = base.BasePlaybackProvider(audio=audio, backend=self) @@ -27,13 +28,10 @@ class LocalBackend(pykka.ThreadingActor, base.Backend): self.uri_schemes = ['file'] - def create_dirs_and_files(self): - try: - path.get_or_create_dir(self.config['local']['media_dir']) - except EnvironmentError as error: - logger.warning( - 'Could not create local media dir: %s', - encoding.locale_decode(error)) + def check_dirs_and_files(self): + if not os.path.isdir(self.config['local']['media_dir']): + logger.warning('Local media dir %s does not exist.' % + self.config['local']['media_dir']) try: path.get_or_create_dir(self.config['local']['playlists_dir']) diff --git a/mopidy/backends/local/translator.py b/mopidy/backends/local/translator.py index 683ad6b4..c53011f6 100644 --- a/mopidy/backends/local/translator.py +++ b/mopidy/backends/local/translator.py @@ -31,6 +31,7 @@ def parse_m3u(file_path, media_dir): - This function does not bother with Extended M3U directives. """ + # TODO: uris as bytes uris = [] try: with open(file_path) as m3u: @@ -71,6 +72,7 @@ def parse_mpd_tag_cache(tag_cache, music_dir=''): current = {} state = None + # TODO: uris as bytes for line in contents.split(b'\n'): if line == b'songList begin': state = 'songs' diff --git a/mopidy/config/types.py b/mopidy/config/types.py index 29c41ae2..726d81f7 100644 --- a/mopidy/config/types.py +++ b/mopidy/config/types.py @@ -24,12 +24,11 @@ def encode(value): class ExpandedPath(bytes): - def __new__(self, value): - expanded = path.expand_path(value) + def __new__(self, original, expanded): return super(ExpandedPath, self).__new__(self, expanded) - def __init__(self, value): - self.original = value + def __init__(self, original, expanded): + self.original = original class ConfigValue(object): @@ -241,20 +240,18 @@ class Path(ConfigValue): - ``$XDG_DATA_DIR`` according to the XDG spec - ``$XDG_MUSIC_DIR`` according to the XDG spec - - Supported kwargs: ``optional``, ``choices``, and ``secret`` """ - def __init__(self, optional=False, choices=None): + def __init__(self, optional=False): self._required = not optional - self._choices = choices def deserialize(self, value): value = value.strip() + expanded = path.expand_path(value) validators.validate_required(value, self._required) - validators.validate_choice(value, self._choices) - if not value: + validators.validate_required(expanded, self._required) + if not value or expanded is None: return None - return ExpandedPath(value) + return ExpandedPath(value, expanded) def serialize(self, value, display=False): if isinstance(value, ExpandedPath): diff --git a/mopidy/frontends/mpd/translator.py b/mopidy/frontends/mpd/translator.py index 7cf5b0c6..e164883d 100644 --- a/mopidy/frontends/mpd/translator.py +++ b/mopidy/frontends/mpd/translator.py @@ -236,7 +236,7 @@ def tracks_to_tag_cache_format(tracks, media_dir): _add_to_tag_cache(result, dirs, files, media_dir) return result - +# TODO: bytes only def _add_to_tag_cache(result, dirs, files, media_dir): base_path = media_dir.encode('utf-8') diff --git a/mopidy/scanner.py b/mopidy/scanner.py index cb19c5fd..fc80f95b 100644 --- a/mopidy/scanner.py +++ b/mopidy/scanner.py @@ -35,8 +35,8 @@ from mopidy.utils import log, path, versioning def main(): options = parse_options() # TODO: support config files and overrides (shared from main?) - config_files = ['/etc/mopidy/mopidy.conf', - '$XDG_CONFIG_DIR/mopidy/mopidy.conf'] + config_files = [b'/etc/mopidy/mopidy.conf', + b'$XDG_CONFIG_DIR/mopidy/mopidy.conf'] config_overrides = [] # TODO: decide if we want to avoid this boilerplate some how. @@ -50,6 +50,10 @@ def main(): config_files, extensions, config_overrides) log.setup_log_levels(config) + if not config['local']['media_dir']: + logging.warning('Config value local/media_dir is not set.') + return + # TODO: missing error checking and other default setup code. tracks = [] diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index 164b114c..8ad9b31a 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -22,8 +22,13 @@ XDG_DIRS = { 'XDG_MUSIC_DIR': glib.get_user_special_dir(glib.USER_DIRECTORY_MUSIC), } +# XDG_MUSIC_DIR can be none, so filter out any bad data. +XDG_DIRS = dict((k, v) for k, v in XDG_DIRS.items() if v is not None) + def get_or_create_dir(dir_path): + if not isinstance(dir_path, bytes): + raise ValueError('Path is not a bytestring.') dir_path = expand_path(dir_path) if os.path.isfile(dir_path): raise OSError( @@ -36,6 +41,8 @@ def get_or_create_dir(dir_path): def get_or_create_file(file_path): + if not isinstance(file_path, bytes): + raise ValueError('Path is not a bytestring.') file_path = expand_path(file_path) get_or_create_dir(os.path.dirname(file_path)) if not os.path.isfile(file_path): @@ -93,8 +100,13 @@ def split_path(path): def expand_path(path): - # TODO: expandvars as well? - path = string.Template(path).safe_substitute(XDG_DIRS) + # TODO: document as we want people to use this. + if not isinstance(path, bytes): + raise ValueError('Path is not a bytestring.') + try: + path = string.Template(path).substitute(XDG_DIRS) + except KeyError: + return None path = os.path.expanduser(path) path = os.path.abspath(path) return path diff --git a/tests/__init__.py b/tests/__init__.py index b4e1d283..98df0370 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -10,8 +10,10 @@ else: def path_to_data_dir(name): + if not isinstance(name, bytes): + name = name.encode(sys.getfilesystemencoding()) path = os.path.dirname(__file__) - path = os.path.join(path, 'data') + path = os.path.join(path, b'data') path = os.path.abspath(path) return os.path.join(path, name) diff --git a/tests/backends/local/events_test.py b/tests/backends/local/events_test.py index e09bf4b9..1218afc4 100644 --- a/tests/backends/local/events_test.py +++ b/tests/backends/local/events_test.py @@ -11,7 +11,7 @@ class LocalBackendEventsTest(events.BackendEventsTest, unittest.TestCase): config = { 'local': { 'media_dir': path_to_data_dir(''), - 'playlists_dir': '', + 'playlists_dir': b'', 'tag_cache_file': path_to_data_dir('empty_tag_cache'), } } diff --git a/tests/backends/local/library_test.py b/tests/backends/local/library_test.py index 74635b3e..1cb933ce 100644 --- a/tests/backends/local/library_test.py +++ b/tests/backends/local/library_test.py @@ -11,7 +11,7 @@ class LocalLibraryControllerTest(LibraryControllerTest, unittest.TestCase): config = { 'local': { 'media_dir': path_to_data_dir(''), - 'playlists_dir': '', + 'playlists_dir': b'', 'tag_cache_file': path_to_data_dir('library_tag_cache'), } } diff --git a/tests/backends/local/playback_test.py b/tests/backends/local/playback_test.py index 16592539..7381113a 100644 --- a/tests/backends/local/playback_test.py +++ b/tests/backends/local/playback_test.py @@ -15,7 +15,7 @@ class LocalPlaybackControllerTest(PlaybackControllerTest, unittest.TestCase): config = { 'local': { 'media_dir': path_to_data_dir(''), - 'playlists_dir': '', + 'playlists_dir': b'', 'tag_cache_file': path_to_data_dir('empty_tag_cache'), } } diff --git a/tests/backends/local/tracklist_test.py b/tests/backends/local/tracklist_test.py index 2d8e87b6..44e4d004 100644 --- a/tests/backends/local/tracklist_test.py +++ b/tests/backends/local/tracklist_test.py @@ -13,7 +13,7 @@ class LocalTracklistControllerTest(TracklistControllerTest, unittest.TestCase): config = { 'local': { 'media_dir': path_to_data_dir(''), - 'playlists_dir': '', + 'playlists_dir': b'', 'tag_cache_file': path_to_data_dir('empty_tag_cache'), } } diff --git a/tests/config/types_test.py b/tests/config/types_test.py index de800e98..65732f56 100644 --- a/tests/config/types_test.py +++ b/tests/config/types_test.py @@ -324,47 +324,43 @@ class PortTest(unittest.TestCase): class ExpandedPathTest(unittest.TestCase): def test_is_bytes(self): - self.assertIsInstance(types.ExpandedPath('/tmp'), bytes) + self.assertIsInstance(types.ExpandedPath(b'/tmp', b'foo'), bytes) - @mock.patch('mopidy.utils.path.expand_path') - def test_defaults_to_expanded(self, expand_path_mock): - expand_path_mock.return_value = 'expanded_path' - self.assertEqual('expanded_path', types.ExpandedPath('~')) + def test_defaults_to_expanded(self): + original = b'~' + expanded = b'expanded_path' + self.assertEqual(expanded, types.ExpandedPath(original, expanded)) @mock.patch('mopidy.utils.path.expand_path') def test_orginal_stores_unexpanded(self, expand_path_mock): - self.assertEqual('~', types.ExpandedPath('~').original) + original = b'~' + expanded = b'expanded_path' + result = types.ExpandedPath(original, expanded) + self.assertEqual(original, result.original) class PathTest(unittest.TestCase): def test_deserialize_conversion_success(self): - result = types.Path().deserialize('/foo') + result = types.Path().deserialize(b'/foo') self.assertEqual('/foo', result) self.assertIsInstance(result, types.ExpandedPath) self.assertIsInstance(result, bytes) - def test_deserialize_enforces_choices(self): - value = types.Path(choices=['/foo', '/bar', '/baz']) - self.assertEqual('/foo', value.deserialize('/foo')) - self.assertRaises(ValueError, value.deserialize, '/foobar') - def test_deserialize_enforces_required(self): value = types.Path() - self.assertRaises(ValueError, value.deserialize, '') + self.assertRaises(ValueError, value.deserialize, b'') def test_deserialize_respects_optional(self): value = types.Path(optional=True) - self.assertIsNone(value.deserialize('')) - self.assertIsNone(value.deserialize(' ')) + self.assertIsNone(value.deserialize(b'')) + self.assertIsNone(value.deserialize(b' ')) - @mock.patch('mopidy.utils.path.expand_path') - def test_serialize_uses_original(self, expand_path_mock): - expand_path_mock.return_value = 'expanded_path' - path = types.ExpandedPath('original_path') + def test_serialize_uses_original(self): + path = types.ExpandedPath(b'original_path', b'expanded_path') value = types.Path() self.assertEqual('expanded_path', path) self.assertEqual('original_path', value.serialize(path)) def test_serialize_plain_string(self): value = types.Path() - self.assertEqual('path', value.serialize('path')) + self.assertEqual('path', value.serialize(b'path')) diff --git a/tests/utils/path_test.py b/tests/utils/path_test.py index bd2e794b..f7306f3d 100644 --- a/tests/utils/path_test.py +++ b/tests/utils/path_test.py @@ -22,7 +22,7 @@ class GetOrCreateDirTest(unittest.TestCase): shutil.rmtree(self.parent) def test_creating_dir(self): - dir_path = os.path.join(self.parent, 'test') + dir_path = os.path.join(self.parent, b'test') self.assert_(not os.path.exists(dir_path)) created = path.get_or_create_dir(dir_path) self.assert_(os.path.exists(dir_path)) @@ -30,8 +30,8 @@ class GetOrCreateDirTest(unittest.TestCase): self.assertEqual(created, dir_path) def test_creating_nested_dirs(self): - level2_dir = os.path.join(self.parent, 'test') - level3_dir = os.path.join(self.parent, 'test', 'test') + level2_dir = os.path.join(self.parent, b'test') + level3_dir = os.path.join(self.parent, b'test', b'test') self.assert_(not os.path.exists(level2_dir)) self.assert_(not os.path.exists(level3_dir)) created = path.get_or_create_dir(level3_dir) @@ -48,11 +48,20 @@ class GetOrCreateDirTest(unittest.TestCase): self.assertEqual(created, self.parent) def test_create_dir_with_name_of_existing_file_throws_oserror(self): - conflicting_file = os.path.join(self.parent, 'test') + conflicting_file = os.path.join(self.parent, b'test') open(conflicting_file, 'w').close() - dir_path = os.path.join(self.parent, 'test') + dir_path = os.path.join(self.parent, b'test') self.assertRaises(OSError, path.get_or_create_dir, dir_path) + def test_create_dir_with_unicode(self): + with self.assertRaises(ValueError): + dir_path = unicode(os.path.join(self.parent, b'test')) + path.get_or_create_dir(dir_path) + + def test_create_dir_with_none(self): + with self.assertRaises(ValueError): + path.get_or_create_dir(None) + class GetOrCreateFileTest(unittest.TestCase): def setUp(self): @@ -63,7 +72,7 @@ class GetOrCreateFileTest(unittest.TestCase): shutil.rmtree(self.parent) def test_creating_file(self): - file_path = os.path.join(self.parent, 'test') + file_path = os.path.join(self.parent, b'test') self.assert_(not os.path.exists(file_path)) created = path.get_or_create_file(file_path) self.assert_(os.path.exists(file_path)) @@ -71,8 +80,8 @@ class GetOrCreateFileTest(unittest.TestCase): self.assertEqual(created, file_path) def test_creating_nested_file(self): - level2_dir = os.path.join(self.parent, 'test') - file_path = os.path.join(self.parent, 'test', 'test') + level2_dir = os.path.join(self.parent, b'test') + file_path = os.path.join(self.parent, b'test', b'test') self.assert_(not os.path.exists(level2_dir)) self.assert_(not os.path.exists(file_path)) created = path.get_or_create_file(file_path) @@ -83,7 +92,7 @@ class GetOrCreateFileTest(unittest.TestCase): self.assertEqual(created, file_path) def test_creating_existing_file(self): - file_path = os.path.join(self.parent, 'test') + file_path = os.path.join(self.parent, b'test') path.get_or_create_file(file_path) created = path.get_or_create_file(file_path) self.assert_(os.path.exists(file_path)) @@ -94,6 +103,15 @@ class GetOrCreateFileTest(unittest.TestCase): conflicting_dir = os.path.join(self.parent) self.assertRaises(IOError, path.get_or_create_file, conflicting_dir) + def test_create_dir_with_unicode(self): + with self.assertRaises(ValueError): + file_path = unicode(os.path.join(self.parent, b'test')) + path.get_or_create_file(file_path) + + def test_create_dir_with_none(self): + with self.assertRaises(ValueError): + path.get_or_create_file(None) + class PathToFileURITest(unittest.TestCase): def test_simple_path(self): @@ -219,8 +237,7 @@ class ExpandPathTest(unittest.TestCase): path.expand_path(b'$XDG_DATA_DIR/foo')) def test_xdg_subsititution_unknown(self): - self.assertEqual( - b'/tmp/$XDG_INVALID_DIR/foo', + self.assertIsNone( path.expand_path(b'/tmp/$XDG_INVALID_DIR/foo'))