From 5ae1e0f3c8d3a450cb39f7933aa49bb78c2bc0d9 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 10:33:26 -0700 Subject: [PATCH 1/8] Adapt tests to latest Unidecode version Unidecode 1.3.5 (a yanked PyPI version) changed the behavior of Unidecode for some specific characters: > Remove trailing space in replacements for vulgar fractions. As luck would have it, our tests used the 1/2 character specifically to test the behavior when these characters decoded to contain slashes. We now pin a sufficiently recent version of Unidecode and adapt the tests to match the new behavior. --- setup.py | 2 +- test/test_library.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/setup.py b/setup.py index 4c4f7d629..d49ed65b2 100755 --- a/setup.py +++ b/setup.py @@ -85,7 +85,7 @@ setup( }, install_requires=[ - 'unidecode', + 'unidecode>=1.3.6', 'musicbrainzngs>=0.4', 'pyyaml', 'mediafile>=0.9.0', diff --git a/test/test_library.py b/test/test_library.py index 31ced7a2c..5e57afb17 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -441,7 +441,7 @@ class DestinationTest(_common.TestCase): self.lib.directory = b'lib' self.lib.path_formats = [('default', '$title')] self.i.title = 'ab\xa2\xbdd' - self.assertEqual(self.i.destination(), np('lib/abC_ 1_2 d')) + self.assertEqual(self.i.destination(), np('lib/abC_ 1_2d')) def test_destination_with_replacements(self): self.lib.directory = b'base' @@ -637,7 +637,7 @@ class DestinationFunctionTest(_common.TestCase, PathFormattingMixin): def test_asciify_variable(self): self._setf('%asciify{ab\xa2\xbdd}') - self._assert_dest(b'/base/abC_ 1_2 d') + self._assert_dest(b'/base/abC_ 1_2d') def test_left_variable(self): self._setf('%left{$title, 3}') From 9803939a1c3e0ca1811cce2dd763fe7dba52ef39 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 15:47:41 -0700 Subject: [PATCH 2/8] replaygain: Fix error handling for parallel runs The parallelism strategy in #3478, in retrospect, used a pretty funky way to deal with exceptions in the asynchronous work---since `apply_async` has an `error_callback` parameter that's meant for exactly this. The problem is that the wrapped function would correctly log the exception *and then return `None`*, confusing any downstream code. Instead of just adding `None`-awareness to the callback, let's just avoid running the callback altogether in the case of an error. --- beetsplug/replaygain.py | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index c228f74b3..63d927f61 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -1085,7 +1085,7 @@ class ExceptionWatcher(Thread): try: exc = self._queue.get_nowait() self._callback() - raise exc[1].with_traceback(exc[2]) + raise exc except queue.Empty: # No exceptions yet, loop back to check # whether `_stopevent` is set @@ -1338,23 +1338,16 @@ class ReplayGainPlugin(BeetsPlugin): def _apply(self, func, args, kwds, callback): if self._has_pool(): - def catch_exc(func, exc_queue, log): - """Wrapper to catch raised exceptions in threads + def handle_exc(exc): + """Handle exceptions in the async work. """ - def wfunc(*args, **kwargs): - try: - return func(*args, **kwargs) - except ReplayGainError as e: - log.info(e.args[0]) # log non-fatal exceptions - except Exception: - exc_queue.put(sys.exc_info()) - return wfunc + if isinstance(exc, ReplayGainError): + self._log.info(exc.args[0]) # Log non-fatal exceptions. + else: + self.exc_queue.put(exc) - # Wrap function and callback to catch exceptions - func = catch_exc(func, self.exc_queue, self._log) - callback = catch_exc(callback, self.exc_queue, self._log) - - self.pool.apply_async(func, args, kwds, callback) + self.pool.apply_async(func, args, kwds, callback, + error_callback=handle_exc) else: callback(func(*args, **kwds)) From eaabf291f7edf3defbf469e251789ca2b0842d7c Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 15:51:44 -0700 Subject: [PATCH 3/8] Changelog for #4506 --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 11f04dee4..9df77d5e7 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -48,6 +48,9 @@ New features: Bug fixes: +* :doc:`/plugins/replaygain`: Avoid a crash when errors occur in the analysis + backend. + :bug:`4506` * We now respect the Spotify API's rate limiting, which avoids crashing when the API reports code 429 (too many requests). :bug:`4370` * Fix implicit paths OR queries (e.g. ``beet list /path/ , /other-path/``) From 675dd7b9a9f6cf7023225fd578f4fdde3045cf2b Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 16:06:40 -0700 Subject: [PATCH 4/8] Add logging for command output on errors --- beetsplug/replaygain.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index 63d927f61..ab3db4575 100644 --- a/beetsplug/replaygain.py +++ b/beetsplug/replaygain.py @@ -49,13 +49,14 @@ class FatalGstreamerPluginReplayGainError(FatalReplayGainError): loading the required plugins.""" -def call(args, **kwargs): +def call(args, log, **kwargs): """Execute the command and return its output or raise a ReplayGainError on failure. """ try: return command_output(args, **kwargs) except subprocess.CalledProcessError as e: + log.debug(e.output.decode('utf8', 'ignore')) raise ReplayGainError( "{} exited with status {}".format(args[0], e.returncode) ) @@ -261,7 +262,7 @@ class FfmpegBackend(Backend): # check that ffmpeg is installed try: - ffmpeg_version_out = call([self._ffmpeg_path, "-version"]) + ffmpeg_version_out = call([self._ffmpeg_path, "-version"], log) except OSError: raise FatalReplayGainError( f"could not find ffmpeg at {self._ffmpeg_path}" @@ -394,7 +395,7 @@ class FfmpegBackend(Backend): self._log.debug( 'executing {0}', ' '.join(map(displayable_path, cmd)) ) - output = call(cmd).stderr.splitlines() + output = call(cmd, self._log).stderr.splitlines() # parse output @@ -525,7 +526,7 @@ class CommandBackend(Backend): # Check whether the program is in $PATH. for cmd in ('mp3gain', 'aacgain'): try: - call([cmd, '-v']) + call([cmd, '-v'], self._log) self.command = cmd except OSError: pass @@ -605,7 +606,7 @@ class CommandBackend(Backend): self._log.debug('analyzing {0} files', len(items)) self._log.debug("executing {0}", " ".join(map(displayable_path, cmd))) - output = call(cmd).stdout + output = call(cmd, self._log).stdout self._log.debug('analysis finished') return self.parse_tool_output(output, len(items) + (1 if is_album else 0)) From 82d41446a22f7e55259aab469f9e7b99c50cc70f Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 16:37:47 -0700 Subject: [PATCH 5/8] Use fsdecode for template substitution The idea in this PR is to converge on Python's `fsdecode` and `fsencode` for argument manipulation. This seems to match up with the Python standard library's assumptions: namely, on Windows, we use `fsdecode` to get back to Unicode strings: https://github.com/python/cpython/blob/54bbb5e3363ef3634f1fd7521ba3f3c42c81a331/Lib/subprocess.py#L561 So let's start by dropping this utility and going straight for `fsdecode` here to match. --- beets/util/__init__.py | 18 ------------------ beetsplug/convert.py | 6 +++--- beetsplug/duplicates.py | 5 +++-- 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 720ca311a..cb9c3f4ea 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -1092,21 +1092,3 @@ def lazy_property(func): return value return wrapper - - -def decode_commandline_path(path): - """Prepare a path for substitution into commandline template. - - On Python 3, we need to construct the subprocess commands to invoke as a - Unicode string. On Unix, this is a little unfortunate---the OS is - expecting bytes---so we use surrogate escaping and decode with the - argument encoding, which is the same encoding that will then be - *reversed* to recover the same bytes before invoking the OS. On - Windows, we want to preserve the Unicode filename "as is." - """ - # On Python 3, the template is a Unicode string, which only supports - # substitution of Unicode variables. - if platform.system() == 'Windows': - return path.decode(_fsencoding()) - else: - return path.decode(arg_encoding(), 'surrogateescape') diff --git a/beetsplug/convert.py b/beetsplug/convert.py index 95240dc39..17a18e358 100644 --- a/beetsplug/convert.py +++ b/beetsplug/convert.py @@ -14,7 +14,7 @@ """Converts tracks or albums to external directory """ -from beets.util import par_map, decode_commandline_path, arg_encoding +from beets.util import par_map, arg_encoding import os import threading @@ -214,8 +214,8 @@ class ConvertPlugin(BeetsPlugin): self._log.info('Encoding {0}', util.displayable_path(source)) command = command.decode(arg_encoding(), 'surrogateescape') - source = decode_commandline_path(source) - dest = decode_commandline_path(dest) + source = os.fsdecode(source) + dest = os.fsdecode(dest) # Substitute $source and $dest in the argument list. args = shlex.split(command) diff --git a/beetsplug/duplicates.py b/beetsplug/duplicates.py index fdd5c1750..f655c61f2 100644 --- a/beetsplug/duplicates.py +++ b/beetsplug/duplicates.py @@ -16,11 +16,12 @@ """ import shlex +import os from beets.plugins import BeetsPlugin from beets.ui import decargs, print_, Subcommand, UserError from beets.util import command_output, displayable_path, subprocess, \ - bytestring_path, MoveOperation, decode_commandline_path + bytestring_path, MoveOperation from beets.library import Item, Album @@ -200,7 +201,7 @@ class DuplicatesPlugin(BeetsPlugin): output as flexattr on a key that is the name of the program, and return the key, checksum tuple. """ - args = [p.format(file=decode_commandline_path(item.path)) + args = [p.format(file=os.fsdecode(item.path)) for p in shlex.split(prog)] key = args[0] checksum = getattr(item, key, False) From 086bab55b1b04eb091fad8194fd641df76bff47a Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 16:42:43 -0700 Subject: [PATCH 6/8] Standardize on Python's fsencode for arguments This can apparently never be `None`, as of Python 3.2. --- beets/util/__init__.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index cb9c3f4ea..06e02ee08 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -17,7 +17,6 @@ import os import sys import errno -import locale import re import tempfile import shutil @@ -332,12 +331,7 @@ def arg_encoding(): """Get the encoding for command-line arguments (and other OS locale-sensitive strings). """ - try: - return locale.getdefaultlocale()[1] or 'utf-8' - except ValueError: - # Invalid locale environment variable setting. To avoid - # failing entirely for no good reason, assume UTF-8. - return 'utf-8' + return sys.getfilesystemencoding() def _fsencoding(): @@ -837,13 +831,14 @@ def cpu_count(): def convert_command_args(args): - """Convert command arguments to bytestrings on Python 2 and - surrogate-escaped strings on Python 3.""" + """Convert command arguments, which may either be `bytes` or `str` + objects, to uniformly surrogate-escaped strings. + """ assert isinstance(args, list) def convert(arg): if isinstance(arg, bytes): - arg = arg.decode(arg_encoding(), 'surrogateescape') + return os.fsdecode(arg) return arg return [convert(a) for a in args] From 8baf3e302d7b2b9a4dfc0806cc112bb0ddb768cd Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 17:13:57 -0700 Subject: [PATCH 7/8] Skip an unhelpful test on Windows On Windows, converting command-line arguments (hopefully!!!) only needs to deal with valid strings from the OS. So it is not really relevant to test with non-UTF-8, non-surrogate bytes. --- test/test_util.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_util.py b/test/test_util.py index fcaf9f5ce..14ac7f2b2 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -101,6 +101,7 @@ class UtilTest(unittest.TestCase): ]) self.assertEqual(p, 'foo/_/bar') + @unittest.skipIf(sys.platform == 'win32', 'win32') def test_convert_command_args_keeps_undecodeable_bytes(self): arg = b'\x82' # non-ascii bytes cmd_args = util.convert_command_args([arg]) From 1a73a4550ae721391a55755ea9edf58f51c76de6 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sat, 1 Oct 2022 17:30:53 -0700 Subject: [PATCH 8/8] Changelog for #4507 --- docs/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 9df77d5e7..8ea1c0322 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -51,6 +51,10 @@ Bug fixes: * :doc:`/plugins/replaygain`: Avoid a crash when errors occur in the analysis backend. :bug:`4506` +* We now use Python's defaults for command-line argument encoding, which + should reduce the chance for errors and "file not found" failures when + invoking other command-line tools, especially on Windows. + :bug:`4507` * We now respect the Spotify API's rate limiting, which avoids crashing when the API reports code 429 (too many requests). :bug:`4370` * Fix implicit paths OR queries (e.g. ``beet list /path/ , /other-path/``)