diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 720ca311a..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] @@ -1092,21 +1087,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) diff --git a/beetsplug/replaygain.py b/beetsplug/replaygain.py index c228f74b3..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)) @@ -1085,7 +1086,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 +1339,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)) diff --git a/docs/changelog.rst b/docs/changelog.rst index bd4413f41..2bca039be 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -50,6 +50,13 @@ New features: 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/``) 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}') 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])