Merge pull request #5930 from tk0miya/refactor_subprocess.run

Use subprocess.run() instead of Popen()

sphinx/__init__.py

@@ -12,8 +12,10 @@
 # (Otherwise getting the version out of it from setup.py is impossible.)
 
 import os
+import subprocess
 import warnings
 from os import path
+from subprocess import PIPE
 
 from .deprecation import RemovedInNextVersionWarning
 
@@ -53,11 +55,9 @@ if __version__.endswith('+'):
     __display_version__ = __version__
     __version__ = __version__[:-1]  # remove '+' for PEP-440 version spec.
     try:
-        import subprocess
+        ret = subprocess.run(['git', 'show', '-s', '--pretty=format:%h'],
-        p = subprocess.Popen(['git', 'show', '-s', '--pretty=format:%h'],
+                             stdout=PIPE, stderr=PIPE, encoding='ascii')
-                             stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        if ret.stdout:
-        out, err = p.communicate()
+            __display_version__ += '/' + ret.stdout.strip()
-        if out:
-            __display_version__ += '/' + out.decode().strip()
     except Exception:
         pass

sphinx/ext/graphviz.py

@@ -11,9 +11,10 @@
 
 import posixpath
 import re
+import subprocess
 from hashlib import sha1
 from os import path
-from subprocess import Popen, PIPE
+from subprocess import CalledProcessError, PIPE
 
 from docutils import nodes
 from docutils.parsers.rst import directives
@@ -243,31 +244,24 @@ def render_dot(self, code, options, format, prefix='graphviz'):
 
     if format == 'png':
         dot_args.extend(['-Tcmapx', '-o%s.map' % outfn])
+
     try:
-        p = Popen(dot_args, stdout=PIPE, stdin=PIPE, stderr=PIPE, cwd=cwd)
+        ret = subprocess.run(dot_args, input=code.encode(), stdout=PIPE, stderr=PIPE,
-    except FileNotFoundError:
+                             cwd=cwd, check=True)
+        if not path.isfile(outfn):
+            raise GraphvizError(__('dot did not produce an output file:\n[stderr]\n%r\n'
+                                   '[stdout]\n%r') % (ret.stderr, ret.stdout))
+        return relfn, outfn
+    except OSError:
         logger.warning(__('dot command %r cannot be run (needed for graphviz '
                           'output), check the graphviz_dot setting'), graphviz_dot)
         if not hasattr(self.builder, '_graphviz_warned_dot'):
             self.builder._graphviz_warned_dot = {}  # type: ignore
         self.builder._graphviz_warned_dot[graphviz_dot] = True  # type: ignore
         return None, None
-    try:
+    except CalledProcessError as exc:
-        # Graphviz may close standard input when an error occurs,
+        raise GraphvizError(__('dot exited with error:\n[stderr]\n%r\n'
-        # resulting in a broken pipe on communicate()
+                               '[stdout]\n%r') % (exc.stderr, exc.stdout))
-        stdout, stderr = p.communicate(code.encode())
-    except BrokenPipeError:
-        # in this case, read the standard output and standard error streams
-        # directly, to get the error message(s)
-        stdout, stderr = p.stdout.read(), p.stderr.read()
-        p.wait()
-    if p.returncode != 0:
-        raise GraphvizError(__('dot exited with error:\n[stderr]\n%s\n'
-                               '[stdout]\n%s') % (stderr, stdout))
-    if not path.isfile(outfn):
-        raise GraphvizError(__('dot did not produce an output file:\n[stderr]\n%s\n'
-                               '[stdout]\n%s') % (stderr, stdout))
-    return relfn, outfn
 
 
 def render_dot_html(self, node, code, options, prefix='graphviz',

sphinx/ext/imgconverter.py

@@ -7,8 +7,8 @@
     :copyright: Copyright 2007-2019 by the Sphinx team, see AUTHORS.
     :license: BSD, see LICENSE for details.
 """
-import locale
 import subprocess
+from subprocess import CalledProcessError, PIPE
 
 from sphinx.errors import ExtensionError
 from sphinx.locale import __
@@ -37,27 +37,19 @@ class ImagemagickConverter(ImageConverter):
         try:
             args = [self.config.image_converter, '-version']
             logger.debug('Invoking %r ...', args)
-            p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            subprocess.run(args, stdout=PIPE, stderr=PIPE, check=True)
+            return True
         except OSError:
             logger.warning(__('convert command %r cannot be run.'
                               'check the image_converter setting'),
                            self.config.image_converter)
             return False
-
+        except CalledProcessError as exc:
-        try:
-            stdout, stderr = p.communicate()
-        except BrokenPipeError:
-            stdout, stderr = p.stdout.read(), p.stderr.read()
-            p.wait()
-        if p.returncode != 0:
-            encoding = locale.getpreferredencoding()
             logger.warning(__('convert exited with error:\n'
-                              '[stderr]\n%s\n[stdout]\n%s'),
+                              '[stderr]\n%r\n[stdout]\n%r'),
-                           stderr.decode(encoding), stdout.decode(encoding))
+                           exc.stderr, exc.stdout)
             return False
 
-        return True
-
     def convert(self, _from, _to):
         # type: (str, str) -> bool
         """Converts the image to expected one."""
@@ -70,24 +62,17 @@ class ImagemagickConverter(ImageConverter):
                     self.config.image_converter_args +
                     [_from, _to])
             logger.debug('Invoking %r ...', args)
-            p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            subprocess.run(args, stdout=PIPE, stderr=PIPE, check=True)
-        except FileNotFoundError:
+            return True
+        except OSError:
             logger.warning(__('convert command %r cannot be run.'
                               'check the image_converter setting'),
                            self.config.image_converter)
             return False
-
+        except CalledProcessError as exc:
-        try:
-            stdout, stderr = p.communicate()
-        except BrokenPipeError:
-            stdout, stderr = p.stdout.read(), p.stderr.read()
-            p.wait()
-        if p.returncode != 0:
             raise ExtensionError(__('convert exited with error:\n'
-                                    '[stderr]\n%s\n[stdout]\n%s') %
+                                    '[stderr]\n%r\n[stdout]\n%r') %
-                                 (stderr, stdout))
+                                 (exc.stderr, exc.stdout))
-
-        return True
 
 
 def setup(app):

sphinx/ext/imgmath.py

@@ -11,10 +11,11 @@
 import posixpath
 import re
 import shutil
+import subprocess
 import tempfile
 from hashlib import sha1
 from os import path
-from subprocess import Popen, PIPE
+from subprocess import CalledProcessError, PIPE
 
 from docutils import nodes
 
@@ -23,7 +24,7 @@ from sphinx.errors import SphinxError
 from sphinx.locale import _, __
 from sphinx.util import logging
 from sphinx.util.math import get_node_equation_number, wrap_displaymath
-from sphinx.util.osutil import ensuredir, cd
+from sphinx.util.osutil import ensuredir
 from sphinx.util.png import read_png_depth, write_png_depth
 from sphinx.util.pycompat import sys_encoding
 
@@ -132,38 +133,31 @@ def compile_math(latex, builder):
     command.extend(builder.config.imgmath_latex_args)
     command.append('math.tex')
 
-    with cd(tempdir):
+    try:
-        try:
+        subprocess.run(command, stdout=PIPE, stderr=PIPE, cwd=tempdir, check=True)
-            p = Popen(command, stdout=PIPE, stderr=PIPE)
+        return path.join(tempdir, 'math.dvi')
-        except FileNotFoundError:
+    except OSError:
-            logger.warning(__('LaTeX command %r cannot be run (needed for math '
+        logger.warning(__('LaTeX command %r cannot be run (needed for math '
-                              'display), check the imgmath_latex setting'),
+                          'display), check the imgmath_latex setting'),
-                           builder.config.imgmath_latex)
+                       builder.config.imgmath_latex)
-            raise InvokeError
+        raise InvokeError
-
+    except CalledProcessError as exc:
-    stdout, stderr = p.communicate()
+        raise MathExtError('latex exited with error', exc.stderr, exc.stdout)
-    if p.returncode != 0:
-        raise MathExtError('latex exited with error', stderr, stdout)
-
-    return path.join(tempdir, 'math.dvi')
 
 
 def convert_dvi_to_image(command, name):
     # type: (List[str], str) -> Tuple[bytes, bytes]
     """Convert DVI file to specific image format."""
     try:
-        p = Popen(command, stdout=PIPE, stderr=PIPE)
+        ret = subprocess.run(command, stdout=PIPE, stderr=PIPE, check=True)
-    except FileNotFoundError:
+        return ret.stdout, ret.stderr
+    except OSError:
         logger.warning(__('%s command %r cannot be run (needed for math '
                           'display), check the imgmath_%s setting'),
                        name, command[0], name)
         raise InvokeError
-
+    except CalledProcessError as exc:
-    stdout, stderr = p.communicate()
+        raise MathExtError('%s exited with error' % name, exc.stderr, exc.stdout)
-    if p.returncode != 0:
-        raise MathExtError('%s exited with error' % name, stderr, stdout)
-
-    return stdout, stderr
 
 
 def convert_dvi_to_png(dvipath, builder):

sphinx/testing/fixtures.py

@@ -13,6 +13,7 @@ import subprocess
 import sys
 from collections import namedtuple
 from io import StringIO
+from subprocess import PIPE
 from tempfile import gettempdir
 
 import pytest
@@ -211,10 +212,7 @@ def if_graphviz_found(app):
     graphviz_dot = getattr(app.config, 'graphviz_dot', '')
     try:
         if graphviz_dot:
-            dot = subprocess.Popen([graphviz_dot, '-V'],
+            subprocess.run([graphviz_dot, '-V'], stdout=PIPE, stderr=PIPE)  # show version
-                                   stdout=subprocess.PIPE,
-                                   stderr=subprocess.PIPE)  # show version
-            dot.communicate()
             return
     except OSError:  # No such file or directory
         pass

tests/test_build_epub.py

@@ -9,7 +9,8 @@
 """
 
 import os
-from subprocess import Popen, PIPE
+import subprocess
+from subprocess import CalledProcessError, PIPE
 from xml.etree import ElementTree
 
 import pytest
@@ -18,13 +19,10 @@ import pytest
 # check given command is runnable
 def runnable(command):
     try:
-        p = Popen(command, stdout=PIPE, stderr=PIPE)
+        subprocess.run(command, stdout=PIPE, stderr=PIPE, check=True)
-    except OSError:
+        return True
-        # command not found
+    except (OSError, CalledProcessError):
-        return False
+        return False  # command not found or exit with non-zero
-    else:
-        p.communicate()
-        return p.returncode == 0
 
 
 class EPUBElementTree:
@@ -377,10 +375,10 @@ def test_run_epubcheck(app):
 
     epubcheck = os.environ.get('EPUBCHECK_PATH', '/usr/share/java/epubcheck.jar')
     if runnable(['java', '-version']) and os.path.exists(epubcheck):
-        p = Popen(['java', '-jar', epubcheck, app.outdir / 'SphinxTests.epub'],
+        try:
-                  stdout=PIPE, stderr=PIPE)
+            subprocess.run(['java', '-jar', epubcheck, app.outdir / 'SphinxTests.epub'],
-        stdout, stderr = p.communicate()
+                           stdout=PIPE, stderr=PIPE, check=True)
-        if p.returncode != 0:
+        except CalledProcessError as exc:
-            print(stdout)
+            print(exc.stdout)
-            print(stderr)
+            print(exc.stderr)
-            assert False, 'epubcheck exited with return code %s' % p.returncode
+            assert False, 'epubcheck exited with return code %s' % exc.returncode

tests/test_build_gettext.py

@@ -11,7 +11,8 @@
 import gettext
 import os
 import re
-from subprocess import Popen, PIPE
+import subprocess
+from subprocess import CalledProcessError, PIPE
 
 import pytest
 
@@ -40,32 +41,26 @@ def test_msgfmt(app):
     (app.outdir / 'en' / 'LC_MESSAGES').makedirs()
     with cd(app.outdir):
         try:
-            p = Popen(['msginit', '--no-translator', '-i', 'markup.pot',
+            args = ['msginit', '--no-translator', '-i', 'markup.pot', '--locale', 'en_US']
-                       '--locale', 'en_US'],
+            subprocess.run(args, stdout=PIPE, stderr=PIPE, check=True)
-                      stdout=PIPE, stderr=PIPE)
         except OSError:
             pytest.skip()  # most likely msginit was not found
-        else:
+        except CalledProcessError as exc:
-            stdout, stderr = p.communicate()
+            print(exc.stdout)
-            if p.returncode != 0:
+            print(exc.stderr)
-                print(stdout)
+            assert False, 'msginit exited with return code %s' % exc.returncode
-                print(stderr)
+
-                assert False, 'msginit exited with return code %s' % \
-                    p.returncode
         assert (app.outdir / 'en_US.po').isfile(), 'msginit failed'
         try:
-            p = Popen(['msgfmt', 'en_US.po', '-o',
+            args = ['msgfmt', 'en_US.po', '-o', os.path.join('en', 'LC_MESSAGES', 'test_root.mo')]
-                       os.path.join('en', 'LC_MESSAGES', 'test_root.mo')],
+            subprocess.run(args, stdout=PIPE, stderr=PIPE, check=True)
-                      stdout=PIPE, stderr=PIPE)
         except OSError:
             pytest.skip()  # most likely msgfmt was not found
-        else:
+        except CalledProcessError as exc:
-            stdout, stderr = p.communicate()
+            print(exc.stdout)
-            if p.returncode != 0:
+            print(exc.stderr)
-                print(stdout)
+            assert False, 'msgfmt exited with return code %s' % exc.returncode
-                print(stderr)
+
-                assert False, 'msgfmt exited with return code %s' % \
-                    p.returncode
         mo = app.outdir / 'en' / 'LC_MESSAGES' / 'test_root.mo'
         assert mo.isfile(), 'msgfmt failed'
 

tests/test_build_latex.py

@@ -10,9 +10,10 @@
 
 import os
 import re
+import subprocess
 from itertools import product
 from shutil import copyfile
-from subprocess import Popen, PIPE
+from subprocess import CalledProcessError, PIPE
 
 import pytest
 from test_build_html import ENV_WARNINGS
@@ -22,7 +23,7 @@ from sphinx.config import Config
 from sphinx.errors import SphinxError
 from sphinx.testing.util import strip_escseq
 from sphinx.util import docutils
-from sphinx.util.osutil import cd, ensuredir
+from sphinx.util.osutil import ensuredir
 from sphinx.writers.latex import LaTeXTranslator
 
 
@@ -43,43 +44,32 @@ LATEX_WARNINGS = ENV_WARNINGS + """\
 # only run latex if all needed packages are there
 def kpsetest(*filenames):
     try:
-        p = Popen(['kpsewhich'] + list(filenames), stdout=PIPE)
+        subprocess.run(['kpsewhich'] + list(filenames), stdout=PIPE, stderr=PIPE, check=True)
-    except OSError:
-        # no kpsewhich... either no tex distribution is installed or it is
-        # a "strange" one -- don't bother running latex
-        return False
-    else:
-        p.communicate()
-        if p.returncode != 0:
-            # not found
-            return False
-        # found
         return True
+    except (OSError, CalledProcessError):
+        return False  # command not found or exit with non-zero
 
 
 # compile latex document with app.config.latex_engine
 def compile_latex_document(app, filename='python.tex'):
     # now, try to run latex over it
-    with cd(app.outdir):
+    try:
-        try:
+        ensuredir(app.config.latex_engine)
-            ensuredir(app.config.latex_engine)
+        # keep a copy of latex file for this engine in case test fails
-            # keep a copy of latex file for this engine in case test fails
+        copyfile(filename, app.config.latex_engine + '/' + filename)
-            copyfile(filename, app.config.latex_engine + '/' + filename)
+        args = [app.config.latex_engine,
-            p = Popen([app.config.latex_engine,
+                '--halt-on-error',
-                       '--halt-on-error',
+                '--interaction=nonstopmode',
-                       '--interaction=nonstopmode',
+                '-output-directory=%s' % app.config.latex_engine,
-                       '-output-directory=%s' % app.config.latex_engine,
+                filename]
-                       filename],
+        subprocess.run(args, stdout=PIPE, stderr=PIPE, cwd=app.outdir, check=True)
-                      stdout=PIPE, stderr=PIPE)
+    except OSError:  # most likely the latex executable was not found
-        except OSError:  # most likely the latex executable was not found
+        raise pytest.skip.Exception
-            raise pytest.skip.Exception
+    except CalledProcessError as exc:
-        else:
+        print(exc.stdout)
-            stdout, stderr = p.communicate()
+        print(exc.stderr)
-            if p.returncode != 0:
+        assert False, '%s exited with return code %s' % (app.config.latex_engine,
-                print(stdout)
+                                                         exc.returncode)
-                print(stderr)
-                assert False, '%s exited with return code %s' % (
-                    app.config.latex_engine, p.returncode)
 
 
 def skip_if_requested(testfunc):

tests/test_build_texinfo.py

@@ -10,7 +10,8 @@
 
 import os
 import re
-from subprocess import Popen, PIPE
+import subprocess
+from subprocess import CalledProcessError, PIPE
 
 import pytest
 from test_build_html import ENV_WARNINGS
@@ -52,23 +53,15 @@ def test_texinfo(app, status, warning):
             '@anchor{markup testing-various-markup}@anchor{13}' in result)
     assert 'Footnotes' not in result
     # now, try to run makeinfo over it
-    cwd = os.getcwd()
-    os.chdir(app.outdir)
     try:
-        try:
+        args = ['makeinfo', '--no-split', 'sphinxtests.texi']
-            p = Popen(['makeinfo', '--no-split', 'sphinxtests.texi'],
+        subprocess.run(args, stdout=PIPE, stderr=PIPE, cwd=app.outdir, check=True)
-                      stdout=PIPE, stderr=PIPE)
+    except OSError:
-        except OSError:
+        raise pytest.skip.Exception  # most likely makeinfo was not found
-            raise pytest.skip.Exception  # most likely makeinfo was not found
+    except CalledProcessError as exc:
-        else:
+        print(exc.stdout)
-            stdout, stderr = p.communicate()
+        print(exc.stderr)
-            retcode = p.returncode
+        assert False, 'makeinfo exited with return code %s' % exc.retcode
-            if retcode != 0:
-                print(stdout)
-                print(stderr)
-                assert False, 'makeinfo exited with return code %s' % retcode
-    finally:
-        os.chdir(cwd)
 
 
 @pytest.mark.sphinx('texinfo', testroot='markup-rubric')