From 2447e2fa40f0945471e58d0958416e5262122d74 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 5 Oct 2014 23:38:19 +0200 Subject: [PATCH 01/19] util/path: Expose errors to callers of find helper --- mopidy/local/commands.py | 4 +++- mopidy/utils/path.py | 3 ++- tests/audio/test_scan.py | 3 ++- tests/utils/test_path.py | 3 ++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index f2a7ec24..a182aa25 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -74,9 +74,11 @@ class ScanCommand(commands.Command): uris_to_update = set() uris_to_remove = set() - file_mtimes = path.find_mtimes(media_dir) + file_mtimes, file_errors = path.find_mtimes(media_dir) logger.info('Found %d files in media_dir.', len(file_mtimes)) + # TODO: log file errors + num_tracks = library.load() logger.info('Checking %d tracks from library.', num_tracks) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index bad3b6c4..a9187d8f 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -184,7 +184,8 @@ def _find(root, thread_count=10, hidden=True, relative=False): def find_mtimes(root): results, errors = _find(root, hidden=False, relative=False) - return dict((f, int(st.st_mtime)) for f, st in results.iteritems()) + mtimes = dict((f, int(st.st_mtime)) for f, st in results.iteritems()) + return mtimes, errors def check_file_path_is_inside_base_dir(file_path, base_path): diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index 1e352991..2e91ce32 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -295,7 +295,8 @@ class ScannerTest(unittest.TestCase): def find(self, path): media_dir = path_to_data_dir(path) - for path in path_lib.find_mtimes(media_dir): + result, errors = path_lib.find_mtimes(media_dir) + for path in result: yield os.path.join(media_dir, path) def scan(self, paths): diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index b33c6681..51dad2cb 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -215,7 +215,8 @@ class FindMTimesTest(unittest.TestCase): maxDiff = None def find(self, value): - return path.find_mtimes(path_to_data_dir(value)) + result, errors = path.find_mtimes(path_to_data_dir(value)) + return result def test_basic_dir(self): self.assert_(self.find('')) From de4bdbec03d96ec8d5c2aeec7690e70b51325e6f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 14 Oct 2014 22:52:41 +0200 Subject: [PATCH 02/19] tests: Minor cleanup of the existing find tests --- mopidy/utils/path.py | 2 +- tests/utils/test_path.py | 37 ++++++++++++++++++++----------------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index a9187d8f..a633a041 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -142,7 +142,7 @@ def _find_worker(relative, hidden, done, work, results, errors): else: errors[path] = 'Not a file or directory' except os.error as e: - errors[path] = str(e) + errors[path] = e finally: work.task_done() diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 51dad2cb..fa2972fd 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -11,7 +11,7 @@ import glib from mopidy.utils import path -from tests import any_int, path_to_data_dir +import tests class GetOrCreateDirTest(unittest.TestCase): @@ -214,32 +214,35 @@ class ExpandPathTest(unittest.TestCase): class FindMTimesTest(unittest.TestCase): maxDiff = None - def find(self, value): - result, errors = path.find_mtimes(path_to_data_dir(value)) - return result + DOES_NOT_EXIST = tests.path_to_data_dir('does-no-exist') + SINGLE_FILE = tests.path_to_data_dir('blank.mp3') + DATA_DIR = tests.path_to_data_dir('') + FIND_DIR = tests.path_to_data_dir('find') def test_basic_dir(self): - self.assert_(self.find('')) + result, errors = path.find_mtimes(self.DATA_DIR) + self.assert_(result) def test_nonexistant_dir(self): - self.assertEqual(self.find('does-not-exist'), {}) + result, errors = path.find_mtimes(self.DOES_NOT_EXIST) + self.assertEqual(result, {}) + self.assertEqual(errors, {self.DOES_NOT_EXIST: tests.IsA(OSError)}) def test_file(self): - self.assertEqual({path_to_data_dir('blank.mp3'): any_int}, - self.find('blank.mp3')) + result, errors = path.find_mtimes(self.SINGLE_FILE) + self.assertEqual({self.SINGLE_FILE: tests.any_int}, result) def test_files(self): - mtimes = self.find('find') - expected_files = [ - b'find/foo/bar/file', b'find/foo/file', b'find/baz/file'] - expected = {path_to_data_dir(p): any_int for p in expected_files} - self.assertEqual(expected, mtimes) + result, errors = path.find_mtimes(self.FIND_DIR) + expected = { + tests.path_to_data_dir(b'find/foo/bar/file'): tests.any_int, + tests.path_to_data_dir(b'find/foo/file'): tests.any_int, + tests.path_to_data_dir(b'find/baz/file'): tests.any_int} + self.assertEqual(expected, result) def test_names_are_bytestrings(self): - is_bytes = lambda f: isinstance(f, bytes) - for name in self.find(''): - self.assert_( - is_bytes(name), '%s is not bytes object' % repr(name)) + for name in path.find_mtimes(self.DATA_DIR)[0]: + self.assertEqual(name, tests.IsA(bytes)) # TODO: kill this in favour of just os.path.getmtime + mocks From a8017dfef1c7456614d90ce3f34d21ce61b9f416 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 14 Oct 2014 23:34:37 +0200 Subject: [PATCH 03/19] tests: Start adding checks for find errors --- tests/utils/test_path.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index fa2972fd..f19b337b 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -222,6 +222,7 @@ class FindMTimesTest(unittest.TestCase): def test_basic_dir(self): result, errors = path.find_mtimes(self.DATA_DIR) self.assert_(result) + self.assertEqual(errors, {}) def test_nonexistant_dir(self): result, errors = path.find_mtimes(self.DOES_NOT_EXIST) @@ -230,7 +231,8 @@ class FindMTimesTest(unittest.TestCase): def test_file(self): result, errors = path.find_mtimes(self.SINGLE_FILE) - self.assertEqual({self.SINGLE_FILE: tests.any_int}, result) + self.assertEqual(result, {self.SINGLE_FILE: tests.any_int}) + self.assertEqual(errors, {}) def test_files(self): result, errors = path.find_mtimes(self.FIND_DIR) @@ -239,6 +241,7 @@ class FindMTimesTest(unittest.TestCase): tests.path_to_data_dir(b'find/foo/file'): tests.any_int, tests.path_to_data_dir(b'find/baz/file'): tests.any_int} self.assertEqual(expected, result) + self.assertEqual(errors, {}) def test_names_are_bytestrings(self): for name in path.find_mtimes(self.DATA_DIR)[0]: From de5fe5ebab40aedca5566777f5bc7a56f8b529a9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 14 Oct 2014 23:51:57 +0200 Subject: [PATCH 04/19] tests: Add test for current find symlink handling --- mopidy/utils/path.py | 2 +- tests/data/find2/bar | 1 + tests/data/find2/foo | 0 tests/utils/test_path.py | 9 ++++++++- 4 files changed, 10 insertions(+), 2 deletions(-) create mode 120000 tests/data/find2/bar create mode 100644 tests/data/find2/foo diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index a633a041..0730fa06 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -140,7 +140,7 @@ def _find_worker(relative, hidden, done, work, results, errors): elif stat.S_ISREG(st.st_mode): results[path] = st else: - errors[path] = 'Not a file or directory' + errors[path] = Exception('Not a file or directory') except os.error as e: errors[path] = e finally: diff --git a/tests/data/find2/bar b/tests/data/find2/bar new file mode 120000 index 00000000..19102815 --- /dev/null +++ b/tests/data/find2/bar @@ -0,0 +1 @@ +foo \ No newline at end of file diff --git a/tests/data/find2/foo b/tests/data/find2/foo new file mode 100644 index 00000000..e69de29b diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index f19b337b..19573003 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -216,11 +216,13 @@ class FindMTimesTest(unittest.TestCase): DOES_NOT_EXIST = tests.path_to_data_dir('does-no-exist') SINGLE_FILE = tests.path_to_data_dir('blank.mp3') + SINGLE_SYMLINK = tests.path_to_data_dir('find2/bar') DATA_DIR = tests.path_to_data_dir('') FIND_DIR = tests.path_to_data_dir('find') + FIND2_DIR = tests.path_to_data_dir('find2') def test_basic_dir(self): - result, errors = path.find_mtimes(self.DATA_DIR) + result, errors = path.find_mtimes(self.FIND_DIR) self.assert_(result) self.assertEqual(errors, {}) @@ -247,6 +249,11 @@ class FindMTimesTest(unittest.TestCase): for name in path.find_mtimes(self.DATA_DIR)[0]: self.assertEqual(name, tests.IsA(bytes)) + def test_symlinks_are_ignored(self): + result, errors = path.find_mtimes(self.SINGLE_SYMLINK) + self.assertEqual({}, result) + self.assertEqual({self.SINGLE_SYMLINK: tests.IsA(Exception)}, errors) + # TODO: kill this in favour of just os.path.getmtime + mocks class MtimeTest(unittest.TestCase): From d219bab0b29a30b8bbac68a3eac28eb7caae01d1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 00:06:42 +0200 Subject: [PATCH 05/19] tests: Add test for directory without permission behaviour --- tests/utils/test_path.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 19573003..8c9d8255 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -220,6 +220,7 @@ class FindMTimesTest(unittest.TestCase): DATA_DIR = tests.path_to_data_dir('') FIND_DIR = tests.path_to_data_dir('find') FIND2_DIR = tests.path_to_data_dir('find2') + NO_PERMISSION_DIR = tests.path_to_data_dir('no-permission') def test_basic_dir(self): result, errors = path.find_mtimes(self.FIND_DIR) @@ -254,6 +255,11 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual({}, result) self.assertEqual({self.SINGLE_SYMLINK: tests.IsA(Exception)}, errors) + def test_missing_permission_to_directory(self): + result, errors = path.find_mtimes(self.NO_PERMISSION_DIR) + self.assertEqual({}, result) + self.assertEqual({self.NO_PERMISSION_DIR: tests.IsA(OSError)}, errors) + # TODO: kill this in favour of just os.path.getmtime + mocks class MtimeTest(unittest.TestCase): From 682af273485b86c1fbba4fcae905148ac2acd2a0 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 00:17:59 +0200 Subject: [PATCH 06/19] tests: Add test case for file without permissions. --- tests/utils/test_path.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 8c9d8255..f5684049 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -216,6 +216,7 @@ class FindMTimesTest(unittest.TestCase): DOES_NOT_EXIST = tests.path_to_data_dir('does-no-exist') SINGLE_FILE = tests.path_to_data_dir('blank.mp3') + NO_PERMISSION_FILE = tests.path_to_data_dir('no-permission-file') SINGLE_SYMLINK = tests.path_to_data_dir('find2/bar') DATA_DIR = tests.path_to_data_dir('') FIND_DIR = tests.path_to_data_dir('find') @@ -255,6 +256,12 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual({}, result) self.assertEqual({self.SINGLE_SYMLINK: tests.IsA(Exception)}, errors) + def test_missing_permission_to_file(self): + # Note that we cannot know if we have access, but the stat will succeed + result, errors = path.find_mtimes(self.NO_PERMISSION_FILE) + self.assertEqual({self.NO_PERMISSION_FILE: tests.any_int}, result) + self.assertEqual({}, errors) + def test_missing_permission_to_directory(self): result, errors = path.find_mtimes(self.NO_PERMISSION_DIR) self.assertEqual({}, result) From ebb62885cd028f3d4894cc2b5adcffb619a6ce41 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 00:22:13 +0200 Subject: [PATCH 07/19] util/path: Add basic support for following symlinks --- mopidy/utils/path.py | 20 +++++++++++++------- tests/utils/test_path.py | 5 +++++ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index 0730fa06..02152fd4 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -110,11 +110,12 @@ def expand_path(path): return path -def _find_worker(relative, hidden, done, work, results, errors): +def _find_worker(relative, hidden, follow, done, work, results, errors): """Worker thread for collecting stat() results. :param str relative: directory to make results relative to :param bool hidden: whether to include files and dirs starting with '.' + :param bool follow: if symlinks should be followed :param threading.Event done: event indicating that all work has been done :param queue.Queue work: queue of paths to process :param dict results: shared dictionary for storing all the stat() results @@ -132,7 +133,11 @@ def _find_worker(relative, hidden, done, work, results, errors): path = entry try: - st = os.lstat(entry) + if follow: + st = os.stat(entry) + else: + st = os.lstat(entry) + if stat.S_ISDIR(st.st_mode): for e in os.listdir(entry): if hidden or not e.startswith(b'.'): @@ -147,7 +152,7 @@ def _find_worker(relative, hidden, done, work, results, errors): work.task_done() -def _find(root, thread_count=10, hidden=True, relative=False): +def _find(root, thread_count=10, hidden=True, relative=False, follow=False): """Threaded find implementation that provides stat results for files. Note that we do _not_ handle loops from bad sym/hardlinks in any way. @@ -157,6 +162,7 @@ def _find(root, thread_count=10, hidden=True, relative=False): mitigate network lag when scanning on NFS etc. :param bool hidden: whether to include files and dirs starting with '.' :param bool relative: if results should be relative to root or absolute + :param bool follow: if symlinks should be followed """ threads = [] results = {} @@ -168,9 +174,9 @@ def _find(root, thread_count=10, hidden=True, relative=False): if not relative: root = None + args = (root, hidden, follow, done, work, results, errors) for i in range(thread_count): - t = threading.Thread(target=_find_worker, - args=(root, hidden, done, work, results, errors)) + t = threading.Thread(target=_find_worker, args=args) t.daemon = True t.start() threads.append(t) @@ -182,8 +188,8 @@ def _find(root, thread_count=10, hidden=True, relative=False): return results, errors -def find_mtimes(root): - results, errors = _find(root, hidden=False, relative=False) +def find_mtimes(root, follow=False): + results, errors = _find(root, hidden=False, relative=False, follow=follow) mtimes = dict((f, int(st.st_mtime)) for f, st in results.iteritems()) return mtimes, errors diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index f5684049..1fdf0b27 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -267,6 +267,11 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual({}, result) self.assertEqual({self.NO_PERMISSION_DIR: tests.IsA(OSError)}, errors) + def test_basic_symlink(self): + result, errors = path.find_mtimes(self.SINGLE_SYMLINK, follow=True) + self.assertEqual({self.SINGLE_SYMLINK: tests.any_int}, result) + self.assertEqual({}, errors) + # TODO: kill this in favour of just os.path.getmtime + mocks class MtimeTest(unittest.TestCase): From b2419f98144b4552466000593e601216e7d4a4d4 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 00:26:39 +0200 Subject: [PATCH 08/19] tests: Add test case for a symlink pointing at itself --- tests/data/symlink-loop | 1 + tests/utils/test_path.py | 6 ++++++ 2 files changed, 7 insertions(+) create mode 120000 tests/data/symlink-loop diff --git a/tests/data/symlink-loop b/tests/data/symlink-loop new file mode 120000 index 00000000..c83815d9 --- /dev/null +++ b/tests/data/symlink-loop @@ -0,0 +1 @@ +symlink-loop \ No newline at end of file diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 1fdf0b27..ca913f86 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -222,6 +222,7 @@ class FindMTimesTest(unittest.TestCase): FIND_DIR = tests.path_to_data_dir('find') FIND2_DIR = tests.path_to_data_dir('find2') NO_PERMISSION_DIR = tests.path_to_data_dir('no-permission') + SYMLINK_LOOP = tests.path_to_data_dir('symlink-loop') def test_basic_dir(self): result, errors = path.find_mtimes(self.FIND_DIR) @@ -272,6 +273,11 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual({self.SINGLE_SYMLINK: tests.any_int}, result) self.assertEqual({}, errors) + def test_direct_symlink_loop(self): + result, errors = path.find_mtimes(self.SYMLINK_LOOP, follow=True) + self.assertEqual({}, result) + self.assertEqual({self.SYMLINK_LOOP: tests.IsA(OSError)}, errors) + # TODO: kill this in favour of just os.path.getmtime + mocks class MtimeTest(unittest.TestCase): From 93c0d6cc4405dcf38ab433eacbabf8dcf8503a39 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 00:32:12 +0200 Subject: [PATCH 09/19] tests: Update no permission test to use tempfile. --- tests/utils/test_path.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index ca913f86..91a1345b 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -214,9 +214,11 @@ class ExpandPathTest(unittest.TestCase): class FindMTimesTest(unittest.TestCase): maxDiff = None + # TODO: Consider if more of these directory structures should be created by + # the test. This would make it more obvious what our expected result is. + DOES_NOT_EXIST = tests.path_to_data_dir('does-no-exist') SINGLE_FILE = tests.path_to_data_dir('blank.mp3') - NO_PERMISSION_FILE = tests.path_to_data_dir('no-permission-file') SINGLE_SYMLINK = tests.path_to_data_dir('find2/bar') DATA_DIR = tests.path_to_data_dir('') FIND_DIR = tests.path_to_data_dir('find') @@ -259,9 +261,11 @@ class FindMTimesTest(unittest.TestCase): def test_missing_permission_to_file(self): # Note that we cannot know if we have access, but the stat will succeed - result, errors = path.find_mtimes(self.NO_PERMISSION_FILE) - self.assertEqual({self.NO_PERMISSION_FILE: tests.any_int}, result) - self.assertEqual({}, errors) + with tempfile.NamedTemporaryFile() as tmp: + os.chmod(tmp.name, 0) + result, errors = path.find_mtimes(tmp.name) + self.assertEqual({tmp.name: tests.any_int}, result) + self.assertEqual({}, errors) def test_missing_permission_to_directory(self): result, errors = path.find_mtimes(self.NO_PERMISSION_DIR) From 69fa6f4674326f880985c82d8a3017ac486e2821 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 00:44:19 +0200 Subject: [PATCH 10/19] tests: Test symlink that points to it's own parent. --- tests/data/find3/loop | 1 + 1 file changed, 1 insertion(+) create mode 120000 tests/data/find3/loop diff --git a/tests/data/find3/loop b/tests/data/find3/loop new file mode 120000 index 00000000..ba817196 --- /dev/null +++ b/tests/data/find3/loop @@ -0,0 +1 @@ +../find3 \ No newline at end of file From 9b1d20677d7e84f44320d663f88494408166e556 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 01:49:06 +0200 Subject: [PATCH 11/19] tests: Cleanup find tests to use tempfile all over. This should make it more clear what structure we expect. --- tests/data/find/.blank.mp3 | Bin 9360 -> 0 bytes tests/data/find/.hidden/.gitignore | 0 tests/data/find/baz/file | 0 tests/data/find/foo/bar/file | 0 tests/data/find/foo/file | 0 tests/data/find2/bar | 1 - tests/data/find2/foo | 0 tests/data/find3/loop | 1 - tests/data/symlink-loop | 1 - tests/utils/test_path.py | 167 +++++++++++++++++++---------- 10 files changed, 113 insertions(+), 57 deletions(-) delete mode 100644 tests/data/find/.blank.mp3 delete mode 100644 tests/data/find/.hidden/.gitignore delete mode 100644 tests/data/find/baz/file delete mode 100644 tests/data/find/foo/bar/file delete mode 100644 tests/data/find/foo/file delete mode 120000 tests/data/find2/bar delete mode 100644 tests/data/find2/foo delete mode 120000 tests/data/find3/loop delete mode 120000 tests/data/symlink-loop diff --git a/tests/data/find/.blank.mp3 b/tests/data/find/.blank.mp3 deleted file mode 100644 index ef159a700449f6a2bf4c03fc206be8f2ff1c7469..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 9360 zcmeHtXH*ki+ivJZ5Ru+Ok=~nhm4HBifRu!;0@ADWrcwn0(tGG7l+Zf_5J99X9qEc9 z3JQt{B0P{Y=(()ZEx8_x67N`Ez!DP5|CTjer?w$w?|J`H~8QSR{au!br(a zZo~^A!hkq*7Z)Ca8~+a$;Unu1grtkSjZGbRyAR?2t`i;sw{*{L&h)N%$F4P<_2x9> zpWk9tZ(6_K4_-;ERUdNe@AG->ra70nifVq<&9zo9fW5m z{j7b|XIZbQm*nksQ}atkRnw6&IR;uw#Y9G&48*3U*#%bgGMej5T(wUe%T7xjh^%0U z5*Zl0&t>3DL$gbjnof~=jf4MMd|FVH)~sv`TJk(fo?oMc=Y8yeFA;$f&=Y{0)EKoA1BfC*GJTS8cv+ z7__{E%OV@XU3W(3b%$d!AtbKzosYf3mO(u9?0>vQpu)4a?{&w)D)Mw?{tR@==!F!G z4TL2y1-N!ck5Mntsy(Vz4PukeAa|N~e?0#2iQx;F;JjqS;ULw`+dI6XBlAY+*00`g zXJRvJL+d_tZ%m!bt8!|T4k~#RjyD_|n+1iSt5e!DAw;##!F7Y&#BLBe7P4I5FsAm? z@y@OoX-u`VFsnE%h)0;SCUgXYyAv}}84-BZsg7>yZAIckHDINjSmRpn^kblT1oqm)zF{ET4G-)a9S zg9Gshaf}^LKuxrfwuW@^58$M;%(hUqQ&nPS!4lIzDefchKIfe_5gSdAVJHF}rFPH-?cY`dGT;THyrmQah**zTU zOuR0ZJjals$9PjYu6RdCBx>NmUUPo(N`=6&1AI`P^lO*eHIQc&R-fkp&#_7j`2=s~ zUmRNV4qplLJK^GaV8`VrA&p#Evv>?{A|;{=LUKC!wYJ~f zI3hiY45{>tcjK29_4nSrXc2|?MjDAch7YjtUvqp$KYS~Gd2#$6=P%a z`I;(WBRlANgE$6Vpdv1AaTy8~)~a>)s793~vIS6`24JmJPqTB@CXg9{vsi37=b=CK zB+e-$50s09E$zt^RB2MY7GJ!d3n5aytKwpLELv9=ruJ$`w!Y(~0X9;EW9un>^P=z$ zW+z>KOSMXA*o3(5oBOYBsnhZMMx1>@sRv_>n$yw@43yCMgj@I4-|3M$v$hA!3ZI2v z^y8~YIh=%dERSMwW+CxF2@nQHA-C~-P zyp=0f_oy85Jvz5PG;bN%@Tsp<#uQ7>Ma{|;_Ucwrn?JTu zlJj8aCf{UZn|qp{vdb8Ud@-0x9c(8h1Zm~OY^h$s)Q1xYgZ0eL2A+#%pj<>8J5d17 z2+H_8faznvyx!d(eY2fZ$}IH$XMUjM)<%Tq(5#>OQ?df-_b0YIp@IR*wrAMQ*9T$@ zmtlK9-p+pB{wj0&#j$K_^Af4nmGrAVB(!~#)@z+KIEjZt2~Cz?`X)2Rc6{u;EOm7wN8dEPb`~7a6 ztLk|WPWaPnbWpG-%^;pG=^T%)#T6g_10m|vnHU)_W&6dvzvb;|#)RE@s9~v9${dk; zX%D-}b#+^UA12YR4dRjJjtm6tkhXxG(w9A~x%CtIX}&)5{GPz%i;QR1w&`>au|{DC zy5;-l`FAhY{t)`>!==7+-tP@O`l-7fWAfR#@qqpF^1EoEKcSpTvr=d6PpGd>F*?Pg z^YJN1OMqd2t~~x$Wrlw%SEolsmckL2VWD0;-@UF%CO-J_GEo{dsC-8F=TiC9C_Sk6 z>+EP>5RWwX6L&c#B{U+KOggfL0>Sn0kN=P~llW;RC06R}+uN$J#rqpkv_d!dNFKf) zr0mzMN3q*uC z2759G$aUp9Pe^UK^VFi?>{*ia>afZfi=Ng>Eq^3qGMQCPR+6E_HD`%s`(p2NAZ%H* z4-Je+qhCv?zlvle`IkMjQAL4rsRaku;8QBV7>4_encpk2J)ay|seg8O#MB%hOxh9n z-S@lzy(Gh>-NNCYVFF*;w>r579ax#@bIB=EH08@4u38bSpX;Lt7sZY@ zin1T1lqD8h6?4u+85{eHnga%fPdQ$KkR0#TX4EJ*c|sA{^K$BX{lcrd+LIv9+?)fT z{mhEv`laVQv%Sb!GT8^|+kNH}4TG3;^R^$HpRBLlJeHp+EzzHnF-7zt*n@rFOMjtR z>0CK%=q>ekTaycJ=xE-Bdl5gbwVnyPmV}V-^Aen`a1cp03(CcGr?de=c5c>;nc8z1 zPIpK=Z$;Wue6&`zjAOWOGC{7M*R~m(9p7~FX#m6{#o+_+1Nggxqe&il-UU1DJ7tU4 zWW^-%n;$fjGx}WUqywt6Y$YLlmo9ZJPJhr1y1Tmm`&aIf{dm9~-vXgIwfH|*ON5Ke z{#JeMz^nUR{LxV9k>pg*>_-W2Cl7Gy^V1zVK2-2IG&$$h0Bo5ytST4HudW`~9wd^` z`0jpq^fLw}?qPK!(4KFz?}YxvBVA$x;iVx;XDoKd#HBWhc9uFV)J}io>Tfb#K6)NQ z!zuad#r|`GqYFB(cA^;IoQVH*!$_N4@vbo_7X$0eJ-~iu8L4I^V;gMji1JwvC`MrI zo-v-(G3jb|sj9oiT@I8d0@Gn;30d>yNrh_irOb(@SJ>7pwdtDNXM!nC8}4ffKR=HT zjIV0T@mL&R?=NrlPsnmcC*ZnMj(Z_Qf>rS6$73kN#u8NS-bPEBT1h|4GuLY-y~J4^ zb7OY0@D1g0C=Ff%#3Rd@b`Q+&oJ*Yua*FlPN~)oVljQ=$Z*nBkDAnu6OzG3^gxvO1 z))9%Cr)(#=)EWWOJfjZ5iq}Mk=(MmO(`Rcpgsx4=0Q z75(H=aUX26hqcQa&$L^u+FrH(_a5wP z>dqT8+xRr{Tt{MC4ih1j0NCd`c*phmPg#Ha#gfL9*}~`9SleI>5KDfoL5Ng9xh`=w zgK@)z#`TXnGNQ1FR5YSI6_>z(UQ`iHSobHxE0B-UU@P_f-5VO3GMuj&r>9!pdfq83 zhq;H?QODnZQ69g+CC|YKFG}NW?>4IOv2Iu4C3G&ACKZ(%i=KSa+ zwpIiyKhf}_;>8|Ao7UG9j7O1n)NSpyxi_821#QDL^qFg@hW8WmyC=N*vd()hKKFP| zJ!&!Ovq^0}x)eyKo@CBuH_Z7rD}g3XB-Qq_^DTckPu5~hp6CL1%)?Hr)YdtEB>l@7 z8f}A?!+WgVswW^GDJTrYGfCSz%2dmrcj45x?dTdQz!un;;ym!qyF33$9fz05?{kz# z@^YfrlySe*H|C&KLsU{;t%x#Lz;g$kU*0D@ZYjUdPFFUnTAXj?@teoz;UVP1+8WgI5N*>rQ1<3FspXa1Q`!h<PZ@wNQaz)WPksB3O{2>*{ zL5{wb*Am>#HXAiC#c@&Rkh}tXaXl+g_6p@QZ8A12#J{1>Z~d0pJHqS1OG63b;bCI~ z>%&>b6(uyuCDDhQoaReBHODy%=YtE*D}FF#H`?rpM+!9pn_CR_W~@w>X>JoQkjaX* z=AFO9tY!k_!wF{_n4Rp-sQ!$7tf>muvjV}b``6dTF5XY1zY33z{5~+*B>PrTjM<0ll z{a`>UQ?H-*{C1OZ?5XDn%kbH(6?{KWxg`4 z`##1kCYh(>_8T{+bU1R5GeF8|DW!uxF>`|IJ$a*Txl!S38Rt^H_}tHziPZOkv#VH` zNve%eR3^^B3Apj%eV`^%J>63XC3ICMkB3EPgLqgtctb(GzxVLVlwncPHjJO;lND)D zp|Rt=QKEfNs`p(L{(+j$@l({I>lbTz<5u137yrzV2rbczi)Zp2SE%90O`U(gl6>IZ zd?PlTaPpIAd{NCJB2S9*CoQok*x>*+R-X$Yx_%~g$rVFw3@LGH5RU{f2lyN3#rwX{pLh2?#h5H-V*imSd{g7=&D?gHf|`iei`!sL zoK#M>@>*EvIQ^Y7?deHqwmO#-lNZ3>h>IHDN9M&WXaR*n9$1Kx{JNOV*N(5;7H#(L zxSKKn(VL! za85!AMO8sVqBI?$R;QNzFZ+}RDKVOo4tBJSZW(A{-)SfG4OfSK z&@|9}^b47BxkJrtW5pk*jn2j6i@yOqPQcz|k0S9_rDyl>-N{m7(ycNc;yDr!4-YHo zU&52(1(VyD_<;%|VOU(6N^EYr)%QQgTTdp3g3NB}&-+b8CqGVVN{vw?6Gi+Qo5rl} zeOQ+woc9wh#$5sv_x0m=naqIjg%2+QeCt*x?tph7=(5f)2U_7jtvU9(n|iZ)ah0Vk zRhOmJzXWiLhsYT#MemMr!e3NBvL9%U4I&-Xwfqe7tifuj0{Ry}u@P^cvn8uWdjEMi z;o_~=z11geeUEAK!ipB#zi5!SnFYn;YEuH|Aw+GnsOC)NimZ4e8I!p(vk+=fE)FhB zd*C?;8a^pxz+n5@GxbmGchv5|81G%B=S_f-4chuhsOr5Xp<|+=j)z^3d?T)oqc2|) zkFtx4OQ!Jq5Cz+wZt`6UN`ySM@yCzmKe9PQYVb=(+Xn9YRv$|jy*!Hwa0XLm@kK9$ z1XoDgqcCRE8Cv<%q*n8%Au7L;d(@L*g4dU?mR1qOBMkKc<_RVUBmRd%>F^Jl+IO@& zE;1-T`j`7-U7^ls;oX*$NK8(XiHp42dz)p`S8N<~H^gn>tg;jqWtfoRjT#*hY>;E- z`9-CN_jsgkfiL%xw6UCvxVWy_-!1?wg*y&ISKji!^h)fxx@Ce=Y-dU5vDBJ~L?dHH z-^U}hyk)fb}3GawiQdQ0w=zSQ;tU1DG&GU3A$Wv2ABAup(dNah} z^~^6n;X;J?`AFnL-<^5+NL}kkj_ALI9M}71H=JzlCR_m`8xG(2unR(b?W|DJ=7$+K zW3=SuM{5q7&|M+!>v96ivcC%%Z}7+{aR=6FN?1|0>HYC=OE{UcUq( zDhjPiaDA(b&PD6%I)QSrKnI*gfH+oBfv>23HG0u9{;Wws&#-%rbYI9S{>OZuC_C$T z22ItSUy~B5YhJE;Ob;KWmRMyGX;VSzZR;F-*SK1u%csx8zJ!D=z_`u$RNlr!J5iSt z6DjZ?8HL)~uf;S;a$I3%eTW#CGz$&}%2JT|p3h#0h~_b%ze@5?kwWg62nWyRf_S(& zm+=74Ml{N5p`*8@b!5H`T#{U{)fVUY%)6cJ^ASgGZtX0xGU3VPdWSL2cRJy2^Au$q z+)HgbA_OKOxMJs}ys^yx+NiJAP7j}Kl){65h1vX;40BB2X(grB2?0xnW5C11oi!)% zqBTV4#B9nY?OL&_|OA6KgE!2H*{OnKL)9|+%ECIc> zmp;PvSmyNFEwzD)p*p+{sE6As4qY6bSBqu2B~}vu>YaU;{3SR~5cF{pZ%$nW*R-{NjP@lEn?v_FTb^ zN7lcycC9SzClDUH+O{H6zCTt-wFg`bhc7Nf8B zTfU5n5WFJnOA+%tB_D`KlGAPwtiw}3kZ0L0evY9Zl(o~tz&s6as*^Lr53CRU#0B{5 zU~eOT%hXU)XfresnEfQwv?SAK8(R859ET~oNHH=cX?lc-C*U!_K>|Q`6D#m~x4mw) zdrN$?f$G*InOT?c=@h1^;e&K>y3^@V!#uZJXIm(q-7>xdwI{D@`4gF{6ii7A^i8Zo zw^-N=fjVkZTpBg0v7nTQ!M^S{D9TFzu15gZa;uD~r^d?#RB#B;(SwyR{z)N&kiGWd z$}cF3pG*r2U$Ph|7Z)e&Z(c@ph!)ha$r)j`oyvuysuMqSAGBUg>WovAt)fDvC%)Wb zGD2!mMa~aVcfeP*cA7crmQo?3D4CIHPiCk3pd!I7T)WC}qf&`6Z)Sat-s)wOya#Zd zV4z4!0nC|*pxR!$@1W9}$2!cRY!!rBzV2nOP@ax7pj<_`7OJ<<$GFY@&C9?X63Abo z+SO!Yypd_5&;Jn;sDkU}MAN!)K4g@qE2Wh;NVFW)62qi>!KMc~##S)iIu-=N1xy5e z5?d)!^HRXX8I+5N**FlK{}?3z?S119fPkKzH?v*RIZsh|1d>g*8OEPI$isjkMZA3K zSuZ_YgqcRGLt=erZc*vm@6ch-B`plbeRB(4pUNRDkKh@r=jFi6rXF=nC@{H zX#vKI3-<&{g7ch15r?Z+7U%_iMmSib%MG0rCo(rK8|0?4k*Lo=|Mr!l9LouR&t(`q z`3=<~EvN^qQq~T+p^^+I%VL+}XE#ktZEfY>WLrAwdR-r3$1E12^+ro;YlA&&t%K$# zWa_U|Xo&mN`bxq}{@NAx%foFGoMQa)? z_1S8o6;?g1??3)n&`cnlFa7+=qIr9_P*FTs90?pop>f^CKqW*B=nLp8IR?@vxZ!XJ zLGftHMl$szhp5K6Qhq?hD{6^lSLkfVDzN0ezol91RnD$^BnGjf|bLL z!|x5bT{rJWTI*_aSakMHO3(GIliOf}d6j=;%j5NdfbUn#6K9HqUq#o@jb9TMfAliA@YULa~E>?KDHn zHZ2C27EJ_k`{~=qS&UgUe=bnDLr*kErDWH@gQ>bcb4}BJh?apBj=NN?b_GU8oh3-e zF6P{gtEALnZ^zwvar+4q=(A|I6e4e_CYQ zZxJT#zl7c*5Voa+6HEIEgoUJsjPNgxq6xtN{&{Am3H0x*Luw2R?~;n6q#z0N)e#Tm zfGxs_rX*LZTh{+R(3X@%{f7lUM-=#i22S(>0{IX3{&(Bpa~#3vfM?|Y-HrcY4++!x i`}Ggczgz$M7U3;$Kn@+<3 diff --git a/tests/data/find/.hidden/.gitignore b/tests/data/find/.hidden/.gitignore deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/data/find/baz/file b/tests/data/find/baz/file deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/data/find/foo/bar/file b/tests/data/find/foo/bar/file deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/data/find/foo/file b/tests/data/find/foo/file deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/data/find2/bar b/tests/data/find2/bar deleted file mode 120000 index 19102815..00000000 --- a/tests/data/find2/bar +++ /dev/null @@ -1 +0,0 @@ -foo \ No newline at end of file diff --git a/tests/data/find2/foo b/tests/data/find2/foo deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/data/find3/loop b/tests/data/find3/loop deleted file mode 120000 index ba817196..00000000 --- a/tests/data/find3/loop +++ /dev/null @@ -1 +0,0 @@ -../find3 \ No newline at end of file diff --git a/tests/data/symlink-loop b/tests/data/symlink-loop deleted file mode 120000 index c83815d9..00000000 --- a/tests/data/symlink-loop +++ /dev/null @@ -1 +0,0 @@ -symlink-loop \ No newline at end of file diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 91a1345b..3dcc8ac8 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -214,73 +214,132 @@ class ExpandPathTest(unittest.TestCase): class FindMTimesTest(unittest.TestCase): maxDiff = None - # TODO: Consider if more of these directory structures should be created by - # the test. This would make it more obvious what our expected result is. + def setUp(self): + self.tmpdir = tempfile.mkdtemp(b'.mopidy-tests') - DOES_NOT_EXIST = tests.path_to_data_dir('does-no-exist') - SINGLE_FILE = tests.path_to_data_dir('blank.mp3') - SINGLE_SYMLINK = tests.path_to_data_dir('find2/bar') - DATA_DIR = tests.path_to_data_dir('') - FIND_DIR = tests.path_to_data_dir('find') - FIND2_DIR = tests.path_to_data_dir('find2') - NO_PERMISSION_DIR = tests.path_to_data_dir('no-permission') - SYMLINK_LOOP = tests.path_to_data_dir('symlink-loop') + def tearDown(self): + shutil.rmtree(self.tmpdir, ignore_errors=True) - def test_basic_dir(self): - result, errors = path.find_mtimes(self.FIND_DIR) - self.assert_(result) - self.assertEqual(errors, {}) + def mkdir(self, *args): + name = os.path.join(self.tmpdir, *args) + os.mkdir(name) + return name - def test_nonexistant_dir(self): - result, errors = path.find_mtimes(self.DOES_NOT_EXIST) - self.assertEqual(result, {}) - self.assertEqual(errors, {self.DOES_NOT_EXIST: tests.IsA(OSError)}) - - def test_file(self): - result, errors = path.find_mtimes(self.SINGLE_FILE) - self.assertEqual(result, {self.SINGLE_FILE: tests.any_int}) - self.assertEqual(errors, {}) - - def test_files(self): - result, errors = path.find_mtimes(self.FIND_DIR) - expected = { - tests.path_to_data_dir(b'find/foo/bar/file'): tests.any_int, - tests.path_to_data_dir(b'find/foo/file'): tests.any_int, - tests.path_to_data_dir(b'find/baz/file'): tests.any_int} - self.assertEqual(expected, result) - self.assertEqual(errors, {}) + def touch(self, *args): + name = os.path.join(self.tmpdir, *args) + open(name, 'w').close() + return name def test_names_are_bytestrings(self): - for name in path.find_mtimes(self.DATA_DIR)[0]: + """We shouldn't be mixing in unicode for paths.""" + result, errors = path.find_mtimes(tests.path_to_data_dir('')) + for name in result.keys() + errors.keys(): self.assertEqual(name, tests.IsA(bytes)) - def test_symlinks_are_ignored(self): - result, errors = path.find_mtimes(self.SINGLE_SYMLINK) - self.assertEqual({}, result) - self.assertEqual({self.SINGLE_SYMLINK: tests.IsA(Exception)}, errors) + def test_nonexistent_dir(self): + """Non existent search roots are an error""" + missing = os.path.join(self.tmpdir, 'does-not-exist') + result, errors = path.find_mtimes(missing) + self.assertEqual(result, {}) + self.assertEqual(errors, {missing: tests.IsA(OSError)}) + + def test_empty_dir(self): + """Empty directories should not show up in results""" + self.mkdir('empty') + + result, errors = path.find_mtimes(self.tmpdir) + self.assertEqual(result, {}) + self.assertEqual(errors, {}) + + def test_file_as_the_root(self): + """Specifying a file as the root should just return the file""" + single = self.touch('single') + + result, errors = path.find_mtimes(single) + self.assertEqual(result, {single: tests.any_int}) + self.assertEqual(errors, {}) + + def test_hidden_directories_are_skipped(self): + pass + + def test_hidden_files_are_skipped(self): + pass + + def test_nested_directories(self): + """Searching nested directories should find all files""" + + # Setup foo/bar and baz directories + self.mkdir('foo') + self.mkdir('foo', 'bar') + self.mkdir('baz') + + # Touch foo/file foo/bar/file and baz/file + foo_file = self.touch('foo', 'file') + foo_bar_file = self.touch('foo', 'bar', 'file') + baz_file = self.touch('baz', 'file') + + result, errors = path.find_mtimes(self.tmpdir) + self.assertEqual(result, {foo_file: tests.any_int, + foo_bar_file: tests.any_int, + baz_file: tests.any_int}) + self.assertEqual(errors, {}) def test_missing_permission_to_file(self): - # Note that we cannot know if we have access, but the stat will succeed - with tempfile.NamedTemporaryFile() as tmp: - os.chmod(tmp.name, 0) - result, errors = path.find_mtimes(tmp.name) - self.assertEqual({tmp.name: tests.any_int}, result) - self.assertEqual({}, errors) + """Missing permissions to a file is not a search error""" + target = self.touch('no-permission') + os.chmod(target, 0) - def test_missing_permission_to_directory(self): - result, errors = path.find_mtimes(self.NO_PERMISSION_DIR) - self.assertEqual({}, result) - self.assertEqual({self.NO_PERMISSION_DIR: tests.IsA(OSError)}, errors) - - def test_basic_symlink(self): - result, errors = path.find_mtimes(self.SINGLE_SYMLINK, follow=True) - self.assertEqual({self.SINGLE_SYMLINK: tests.any_int}, result) + result, errors = path.find_mtimes(self.tmpdir) + self.assertEqual({target: tests.any_int}, result) self.assertEqual({}, errors) - def test_direct_symlink_loop(self): - result, errors = path.find_mtimes(self.SYMLINK_LOOP, follow=True) + def test_missing_permission_to_directory(self): + """Missing permissions to a directory is an error""" + directory = self.mkdir('no-permission') + os.chmod(directory, 0) + + result, errors = path.find_mtimes(self.tmpdir) self.assertEqual({}, result) - self.assertEqual({self.SYMLINK_LOOP: tests.IsA(OSError)}, errors) + self.assertEqual({directory: tests.IsA(OSError)}, errors) + + def test_symlinks_are_ignored(self): + """By default symlinks should be treated as an error""" + target = self.touch('target') + link = os.path.join(self.tmpdir, 'link') + os.symlink(target, link) + + result, errors = path.find_mtimes(self.tmpdir) + self.assertEqual(result, {target: tests.any_int}) + self.assertEqual(errors, {link: tests.IsA(Exception)}) + + def test_symlink_to_file_as_root_is_followed(self): + """Passing a symlink as the root should be followed when follow=True""" + target = self.touch('target') + link = os.path.join(self.tmpdir, 'link') + os.symlink(target, link) + + result, errors = path.find_mtimes(link, follow=True) + self.assertEqual({link: tests.any_int}, result) + self.assertEqual({}, errors) + + def test_symlink_to_directory_is_followed(self): + pass + + def test_symlink_pointing_at_itself_fails(self): + link = os.path.join(self.tmpdir, 'link') + os.symlink(link, link) + + result, errors = path.find_mtimes(link, follow=True) + self.assertEqual({}, result) + self.assertEqual({link: tests.IsA(OSError)}, errors) + + def test_symlink_pointing_at_parent_fails(self): + os.symlink(self.tmpdir, os.path.join(self.tmpdir, 'link')) + + result, errors = path.find_mtimes(self.tmpdir, follow=True) + self.assertEqual({}, result) + self.assertEqual(1, len(errors)) + self.assertEqual(tests.IsA(OSError), errors.values()[0]) # TODO: kill this in favour of just os.path.getmtime + mocks From 54a89038d3e596025d5b361234cbb228127000d1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 23:00:12 +0200 Subject: [PATCH 12/19] utils/path: Don't skip hidden files and folders in generic find code Updates the local scan code to do this instead. --- mopidy/local/commands.py | 6 ++++++ mopidy/utils/path.py | 13 +++++-------- tests/utils/test_path.py | 6 ------ 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index a182aa25..098bd9a9 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -75,6 +75,12 @@ class ScanCommand(commands.Command): uris_to_remove = set() file_mtimes, file_errors = path.find_mtimes(media_dir) + + # TODO: Not sure if we want to keep this, but for now lets filter these + for name in file_mtimes.keys(): + if name.startswith('.'): + del file_mtimes[name] + logger.info('Found %d files in media_dir.', len(file_mtimes)) # TODO: log file errors diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index 02152fd4..34a7f75e 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -110,11 +110,10 @@ def expand_path(path): return path -def _find_worker(relative, hidden, follow, done, work, results, errors): +def _find_worker(relative, follow, done, work, results, errors): """Worker thread for collecting stat() results. :param str relative: directory to make results relative to - :param bool hidden: whether to include files and dirs starting with '.' :param bool follow: if symlinks should be followed :param threading.Event done: event indicating that all work has been done :param queue.Queue work: queue of paths to process @@ -140,8 +139,7 @@ def _find_worker(relative, hidden, follow, done, work, results, errors): if stat.S_ISDIR(st.st_mode): for e in os.listdir(entry): - if hidden or not e.startswith(b'.'): - work.put(os.path.join(entry, e)) + work.put(os.path.join(entry, e)) elif stat.S_ISREG(st.st_mode): results[path] = st else: @@ -152,7 +150,7 @@ def _find_worker(relative, hidden, follow, done, work, results, errors): work.task_done() -def _find(root, thread_count=10, hidden=True, relative=False, follow=False): +def _find(root, thread_count=10, relative=False, follow=False): """Threaded find implementation that provides stat results for files. Note that we do _not_ handle loops from bad sym/hardlinks in any way. @@ -160,7 +158,6 @@ def _find(root, thread_count=10, hidden=True, relative=False, follow=False): :param str root: root directory to search from, may not be a file :param int thread_count: number of workers to use, mainly useful to mitigate network lag when scanning on NFS etc. - :param bool hidden: whether to include files and dirs starting with '.' :param bool relative: if results should be relative to root or absolute :param bool follow: if symlinks should be followed """ @@ -174,7 +171,7 @@ def _find(root, thread_count=10, hidden=True, relative=False, follow=False): if not relative: root = None - args = (root, hidden, follow, done, work, results, errors) + args = (root, follow, done, work, results, errors) for i in range(thread_count): t = threading.Thread(target=_find_worker, args=args) t.daemon = True @@ -189,7 +186,7 @@ def _find(root, thread_count=10, hidden=True, relative=False, follow=False): def find_mtimes(root, follow=False): - results, errors = _find(root, hidden=False, relative=False, follow=follow) + results, errors = _find(root, relative=False, follow=follow) mtimes = dict((f, int(st.st_mtime)) for f, st in results.iteritems()) return mtimes, errors diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 3dcc8ac8..0e91e008 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -259,12 +259,6 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual(result, {single: tests.any_int}) self.assertEqual(errors, {}) - def test_hidden_directories_are_skipped(self): - pass - - def test_hidden_files_are_skipped(self): - pass - def test_nested_directories(self): """Searching nested directories should find all files""" From 063c757570f0c11678e9ade47811c3daeba26233 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 23:21:11 +0200 Subject: [PATCH 13/19] utils/path: Add support for handling sym/hardlink loops --- mopidy/utils/path.py | 15 +++++++++++---- tests/utils/test_path.py | 39 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index 34a7f75e..01b97a50 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -122,7 +122,7 @@ def _find_worker(relative, follow, done, work, results, errors): """ while not done.is_set(): try: - entry = work.get(block=False) + entry, parents = work.get(block=False) except queue.Empty: continue @@ -137,13 +137,19 @@ def _find_worker(relative, follow, done, work, results, errors): else: st = os.lstat(entry) + if (st.st_dev, st.st_ino) in parents: + errors[path] = Exception('Sym/hardlink loop found.') + continue + + parents = parents + [(st.st_dev, st.st_ino)] if stat.S_ISDIR(st.st_mode): for e in os.listdir(entry): - work.put(os.path.join(entry, e)) + work.put((os.path.join(entry, e), parents)) elif stat.S_ISREG(st.st_mode): results[path] = st else: errors[path] = Exception('Not a file or directory') + except os.error as e: errors[path] = e finally: @@ -153,7 +159,8 @@ def _find_worker(relative, follow, done, work, results, errors): def _find(root, thread_count=10, relative=False, follow=False): """Threaded find implementation that provides stat results for files. - Note that we do _not_ handle loops from bad sym/hardlinks in any way. + Tries to protect against sym/hardlink loops by keeping an eye on parent + (st_dev, st_ino) pairs. :param str root: root directory to search from, may not be a file :param int thread_count: number of workers to use, mainly useful to @@ -166,7 +173,7 @@ def _find(root, thread_count=10, relative=False, follow=False): errors = {} done = threading.Event() work = queue.Queue() - work.put(os.path.abspath(root)) + work.put((os.path.abspath(root), [])) if not relative: root = None diff --git a/tests/utils/test_path.py b/tests/utils/test_path.py index 0e91e008..8dafd951 100644 --- a/tests/utils/test_path.py +++ b/tests/utils/test_path.py @@ -221,12 +221,12 @@ class FindMTimesTest(unittest.TestCase): shutil.rmtree(self.tmpdir, ignore_errors=True) def mkdir(self, *args): - name = os.path.join(self.tmpdir, *args) + name = os.path.join(self.tmpdir, *[bytes(a) for a in args]) os.mkdir(name) return name def touch(self, *args): - name = os.path.join(self.tmpdir, *args) + name = os.path.join(self.tmpdir, *[bytes(a) for a in args]) open(name, 'w').close() return name @@ -320,6 +320,7 @@ class FindMTimesTest(unittest.TestCase): pass def test_symlink_pointing_at_itself_fails(self): + """Symlink pointing at itself should give as an OS error""" link = os.path.join(self.tmpdir, 'link') os.symlink(link, link) @@ -328,12 +329,44 @@ class FindMTimesTest(unittest.TestCase): self.assertEqual({link: tests.IsA(OSError)}, errors) def test_symlink_pointing_at_parent_fails(self): + """We should detect a loop via the parent and give up on the branch""" os.symlink(self.tmpdir, os.path.join(self.tmpdir, 'link')) result, errors = path.find_mtimes(self.tmpdir, follow=True) self.assertEqual({}, result) self.assertEqual(1, len(errors)) - self.assertEqual(tests.IsA(OSError), errors.values()[0]) + self.assertEqual(tests.IsA(Exception), errors.values()[0]) + + def test_indirect_symlink_loop(self): + """More indirect loops should also be detected""" + # Setup tmpdir/directory/loop where loop points to tmpdir + directory = os.path.join(self.tmpdir, b'directory') + loop = os.path.join(directory, b'loop') + + os.mkdir(directory) + os.symlink(self.tmpdir, loop) + + result, errors = path.find_mtimes(self.tmpdir, follow=True) + self.assertEqual({}, result) + self.assertEqual({loop: tests.IsA(Exception)}, errors) + + def test_symlink_branches_are_not_excluded(self): + """Using symlinks to make a file show up multiple times should work""" + self.mkdir('directory') + target = self.touch('directory', 'target') + link1 = os.path.join(self.tmpdir, b'link1') + link2 = os.path.join(self.tmpdir, b'link2') + + os.symlink(target, link1) + os.symlink(target, link2) + + expected = {target: tests.any_int, + link1: tests.any_int, + link2: tests.any_int} + + result, errors = path.find_mtimes(self.tmpdir, follow=True) + self.assertEqual(expected, result) + self.assertEqual({}, errors) # TODO: kill this in favour of just os.path.getmtime + mocks From 3dc0a06ffefcbdcc61f6608253ae4925c4788f99 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 23:53:56 +0200 Subject: [PATCH 14/19] local: Fix skipping of hidden file/directories --- mopidy/local/commands.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index 098bd9a9..5cf9f64f 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -77,8 +77,10 @@ class ScanCommand(commands.Command): file_mtimes, file_errors = path.find_mtimes(media_dir) # TODO: Not sure if we want to keep this, but for now lets filter these + # Could be replaced with passing in a predicate to the find function? for name in file_mtimes.keys(): - if name.startswith('.'): + if b'/.' in name: + logger.debug('Skipping hidden file/directory: %r', name) del file_mtimes[name] logger.info('Found %d files in media_dir.', len(file_mtimes)) From 5bf6b779acb6afcf47f6cc3ddca5d8c308bba728 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Oct 2014 23:56:59 +0200 Subject: [PATCH 15/19] local: Add basic logging of scan errors --- mopidy/local/commands.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index 5cf9f64f..d72a4477 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -85,7 +85,11 @@ class ScanCommand(commands.Command): logger.info('Found %d files in media_dir.', len(file_mtimes)) - # TODO: log file errors + if file_errors: + logger.warning('Encountered %d errors while scanning media_dir.', + len(file_errors)) + for name in file_errors: + logger.debug('Scan error %r for %r', file_errors[name], name) num_tracks = library.load() logger.info('Checking %d tracks from library.', num_tracks) From 43d8062094a4ae3ffe500a308495ace0180b594a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Oct 2014 16:46:33 +0200 Subject: [PATCH 16/19] util/path: s/os.error/OSError/ --- mopidy/utils/path.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index 01b97a50..f60eff59 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -150,7 +150,7 @@ def _find_worker(relative, follow, done, work, results, errors): else: errors[path] = Exception('Not a file or directory') - except os.error as e: + except OSError as e: errors[path] = e finally: work.task_done() From d4f47a34c2a3563a0abbe101c3e099fb59c37d2e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Oct 2014 16:52:01 +0200 Subject: [PATCH 17/19] local: Move Hidden file/directory check to excluded extensions check --- mopidy/local/commands.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index d72a4477..04eff795 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -76,13 +76,6 @@ class ScanCommand(commands.Command): file_mtimes, file_errors = path.find_mtimes(media_dir) - # TODO: Not sure if we want to keep this, but for now lets filter these - # Could be replaced with passing in a predicate to the find function? - for name in file_mtimes.keys(): - if b'/.' in name: - logger.debug('Skipping hidden file/directory: %r', name) - del file_mtimes[name] - logger.info('Found %d files in media_dir.', len(file_mtimes)) if file_errors: @@ -111,11 +104,13 @@ class ScanCommand(commands.Command): relpath = os.path.relpath(abspath, media_dir) uri = translator.path_to_local_track_uri(relpath) - if relpath.lower().endswith(excluded_file_extensions): + # TODO: move these to a "predicate" check in the finder? + if b'/.' in relpath: + logger.debug('Skipped %s: Hidden directory/file.', uri) + elif relpath.lower().endswith(excluded_file_extensions): logger.debug('Skipped %s: File extension excluded.', uri) - continue - - uris_to_update.add(uri) + else: + uris_to_update.add(uri) logger.info( 'Found %d tracks which need to be updated.', len(uris_to_update)) From 369edab76d884f3a1abae22ac16977a762ffe106 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Oct 2014 20:08:12 +0200 Subject: [PATCH 18/19] utils/path: Make it more clear that we are not following symlinks --- mopidy/utils/path.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mopidy/utils/path.py b/mopidy/utils/path.py index f60eff59..be735ae5 100644 --- a/mopidy/utils/path.py +++ b/mopidy/utils/path.py @@ -147,8 +147,10 @@ def _find_worker(relative, follow, done, work, results, errors): work.put((os.path.join(entry, e), parents)) elif stat.S_ISREG(st.st_mode): results[path] = st + elif stat.S_ISLNK(st.st_mode): + errors[path] = Exception('Not following symlinks.') else: - errors[path] = Exception('Not a file or directory') + errors[path] = Exception('Not a file or directory.') except OSError as e: errors[path] = e From b9a7a9d2b6844e5204cacfbb645755a01a84b20a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 17 Oct 2014 20:08:33 +0200 Subject: [PATCH 19/19] local: Add follow symlinks setting --- docs/ext/local.rst | 4 ++++ mopidy/local/__init__.py | 1 + mopidy/local/commands.py | 3 ++- mopidy/local/ext.conf | 1 + 4 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/ext/local.rst b/docs/ext/local.rst index 31d00d66..18f66adc 100644 --- a/docs/ext/local.rst +++ b/docs/ext/local.rst @@ -86,6 +86,10 @@ See :ref:`config` for general help on configuring Mopidy. Number of milliseconds before giving up scanning a file and moving on to the next file. +.. confval:: local/scan_follow_symlinks + + If we should follow symlinks found in :confval:`local/media_dir` + .. confval:: local/scan_flush_threshold Number of tracks to wait before telling library it should try and store diff --git a/mopidy/local/__init__.py b/mopidy/local/__init__.py index fc1035fe..a0bb7bc4 100644 --- a/mopidy/local/__init__.py +++ b/mopidy/local/__init__.py @@ -29,6 +29,7 @@ class Extension(ext.Extension): schema['scan_timeout'] = config.Integer( minimum=1000, maximum=1000*60*60) schema['scan_flush_threshold'] = config.Integer(minimum=0) + schema['scan_follow_symlinks'] = config.Boolean() schema['excluded_file_extensions'] = config.List(optional=True) return schema diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index 04eff795..1b8981df 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -74,7 +74,8 @@ class ScanCommand(commands.Command): uris_to_update = set() uris_to_remove = set() - file_mtimes, file_errors = path.find_mtimes(media_dir) + file_mtimes, file_errors = path.find_mtimes( + media_dir, follow=config['local']['scan_follow_symlinks']) logger.info('Found %d files in media_dir.', len(file_mtimes)) diff --git a/mopidy/local/ext.conf b/mopidy/local/ext.conf index 9a0f19f1..535f4806 100644 --- a/mopidy/local/ext.conf +++ b/mopidy/local/ext.conf @@ -6,6 +6,7 @@ data_dir = $XDG_DATA_DIR/mopidy/local playlists_dir = $XDG_DATA_DIR/mopidy/local/playlists scan_timeout = 1000 scan_flush_threshold = 1000 +scan_follow_symlinks = false excluded_file_extensions = .directory .html