From d407db725f1cd4136ebc3c925cd482ed52ed5045 Mon Sep 17 00:00:00 2001
From: Adrian Sampson <adrian@radbox.org>
Date: Thu, 3 Apr 2014 19:47:21 -0700
Subject: [PATCH] convert: catch OSErrors and display error message

This also adds close_fds (only available on Unixes) to the common subprocess
invocation utility.
---
 beets/util/__init__.py |  6 ++++--
 beetsplug/convert.py   | 28 ++++++++++------------------
 docs/changelog.rst     |  2 ++
 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/beets/util/__init__.py b/beets/util/__init__.py
index f5810ff35..99c39e74f 100644
--- a/beets/util/__init__.py
+++ b/beets/util/__init__.py
@@ -23,6 +23,7 @@ import fnmatch
 from collections import defaultdict
 import traceback
 import subprocess
+import platform
 
 MAX_FILENAME_LENGTH = 200
 WINDOWS_MAGIC_PREFIX = u'\\\\?\\'
@@ -594,8 +595,9 @@ def command_output(cmd):
     Python 2.6 and which can have problems if lots of output is sent to
     stderr.
     """
-    with open(os.devnull, 'w') as devnull:
-        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=devnull)
+    with open(os.devnull, 'wb') as devnull:
+        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=devnull,
+                                close_fds=platform.system() != 'Windows')
         stdout, _ = proc.communicate()
     if proc.returncode:
         raise subprocess.CalledProcessError(proc.returncode, cmd)
diff --git a/beetsplug/convert.py b/beetsplug/convert.py
index ac092f94f..c9645ae5a 100644
--- a/beetsplug/convert.py
+++ b/beetsplug/convert.py
@@ -17,11 +17,10 @@
 import logging
 import os
 import threading
-from subprocess import Popen
+import subprocess
 import tempfile
 from string import Template
 import pipes
-import platform
 
 from beets.plugins import BeetsPlugin
 from beets import ui, util
@@ -39,19 +38,6 @@ ALIASES = {
 }
 
 
-def _silent_popen(args):
-    """Invoke a command (like subprocess.Popen) while silencing its
-    error output. Return the Popen object.
-    """
-    # On Windows, close_fds doesn't work (i.e., raises an exception)
-    # when stderr is redirected.
-    return Popen(
-        args,
-        close_fds=platform.system() != 'Windows',
-        stderr=open(os.devnull, 'wb'),
-    )
-
-
 def _destination(dest_dir, item, keep_new, path_formats):
     """Return the path under `dest_dir` where the file should be placed
     (possibly after conversion).
@@ -113,10 +99,11 @@ def encode(source, dest):
     log.debug(u'convert: executing: {0}'.format(
         u' '.join(pipes.quote(o.decode('utf8', 'ignore')) for o in opts)
     ))
-    encode = _silent_popen(opts)
-    encode.wait()
 
-    if encode.returncode != 0:
+    try:
+        util.command_output(opts)
+
+    except subprocess.CalledProcessError:
         # Something went wrong (probably Ctrl+C), remove temporary files
         log.info(u'Encoding {0} failed. Cleaning up...'
                  .format(util.displayable_path(source)))
@@ -124,6 +111,11 @@ def encode(source, dest):
         util.prune_dirs(os.path.dirname(dest))
         return
 
+    except OSError as exc:
+        raise ui.UserError(
+            u'convert: could invoke ffmpeg: {0}'.format(exc)
+        )
+
     if not quiet:
         log.info(u'Finished encoding {0}'.format(
             util.displayable_path(source))
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 3c5efd7d2..7895872ad 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -69,6 +69,8 @@ Fixes:
 * :doc:`/plugins/importfeeds`: Fix crash when importing albums
   containing ``/`` with the ``m3u_multi`` format.
 * Avoid crashing on Mutagen bugs while writing files' tags.
+* :doc:`/plugins/convert`: Display a useful error message when the FFmpeg
+  executable can't be found.
 
 .. _requests: http://www.python-requests.org/