From 46334a2b001878498f8b9d03b137bad3d2b13460 Mon Sep 17 00:00:00 2001 From: Takeshi KOMIYA Date: Sat, 12 Jan 2019 20:14:26 +0900 Subject: [PATCH] Use subprocess.run() instead of Popen() Since python3.5, subprocess.run() has been introduced. It works a wrapper of Popen, and it looks much simple and better. This uses it instead of Popen to make our code simple. --- sphinx/__init__.py | 12 ++++---- sphinx/ext/graphviz.py | 32 +++++++++------------ sphinx/ext/imgconverter.py | 39 ++++++++------------------ sphinx/ext/imgmath.py | 42 ++++++++++++---------------- sphinx/testing/fixtures.py | 6 ++-- tests/test_build_epub.py | 28 +++++++++---------- tests/test_build_gettext.py | 37 +++++++++++------------- tests/test_build_latex.py | 56 +++++++++++++++---------------------- tests/test_build_texinfo.py | 27 +++++++----------- 9 files changed, 113 insertions(+), 166 deletions(-) diff --git a/sphinx/__init__.py b/sphinx/__init__.py index cd41ea654..bfa5297d5 100644 --- a/sphinx/__init__.py +++ b/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 - p = subprocess.Popen(['git', 'show', '-s', '--pretty=format:%h'], - stdout=subprocess.PIPE, stderr=subprocess.PIPE) - out, err = p.communicate() - if out: - __display_version__ += '/' + out.decode().strip() + ret = subprocess.run(['git', 'show', '-s', '--pretty=format:%h'], + stdout=PIPE, stderr=PIPE, encoding='ascii') + if ret.stdout: + __display_version__ += '/' + ret.stdout.strip() except Exception: pass diff --git a/sphinx/ext/graphviz.py b/sphinx/ext/graphviz.py index f5ebbfae9..9827d8531 100644 --- a/sphinx/ext/graphviz.py +++ b/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) - except FileNotFoundError: + ret = subprocess.run(dot_args, input=code.encode(), stdout=PIPE, stderr=PIPE, + 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: - # Graphviz may close standard input when an error occurs, - # resulting in a broken pipe on communicate() - 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 + except CalledProcessError as exc: + raise GraphvizError(__('dot exited with error:\n[stderr]\n%r\n' + '[stdout]\n%r') % (exc.stderr, exc.stdout)) def render_dot_html(self, node, code, options, prefix='graphviz', diff --git a/sphinx/ext/imgconverter.py b/sphinx/ext/imgconverter.py index ac2e00967..f4ba5d001 100644 --- a/sphinx/ext/imgconverter.py +++ b/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 - - try: - stdout, stderr = p.communicate() - except BrokenPipeError: - stdout, stderr = p.stdout.read(), p.stderr.read() - p.wait() - if p.returncode != 0: - encoding = locale.getpreferredencoding() + except CalledProcessError as exc: logger.warning(__('convert exited with error:\n' - '[stderr]\n%s\n[stdout]\n%s'), - stderr.decode(encoding), stdout.decode(encoding)) + '[stderr]\n%r\n[stdout]\n%r'), + 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) - except FileNotFoundError: + 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 - - try: - stdout, stderr = p.communicate() - except BrokenPipeError: - stdout, stderr = p.stdout.read(), p.stderr.read() - p.wait() - if p.returncode != 0: + except CalledProcessError as exc: raise ExtensionError(__('convert exited with error:\n' - '[stderr]\n%s\n[stdout]\n%s') % - (stderr, stdout)) - - return True + '[stderr]\n%r\n[stdout]\n%r') % + (exc.stderr, exc.stdout)) def setup(app): diff --git a/sphinx/ext/imgmath.py b/sphinx/ext/imgmath.py index f7c9aa15e..00aae13f2 100644 --- a/sphinx/ext/imgmath.py +++ b/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: - p = Popen(command, stdout=PIPE, stderr=PIPE) - except FileNotFoundError: - logger.warning(__('LaTeX command %r cannot be run (needed for math ' - 'display), check the imgmath_latex setting'), - builder.config.imgmath_latex) - raise InvokeError - - stdout, stderr = p.communicate() - if p.returncode != 0: - raise MathExtError('latex exited with error', stderr, stdout) - - return path.join(tempdir, 'math.dvi') + try: + subprocess.run(command, stdout=PIPE, stderr=PIPE, cwd=tempdir, check=True) + return path.join(tempdir, 'math.dvi') + except OSError: + logger.warning(__('LaTeX command %r cannot be run (needed for math ' + 'display), check the imgmath_latex setting'), + builder.config.imgmath_latex) + raise InvokeError + except CalledProcessError as exc: + raise MathExtError('latex exited with error', exc.stderr, exc.stdout) 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) - except FileNotFoundError: + ret = subprocess.run(command, stdout=PIPE, stderr=PIPE, check=True) + 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 - - stdout, stderr = p.communicate() - if p.returncode != 0: - raise MathExtError('%s exited with error' % name, stderr, stdout) - - return stdout, stderr + except CalledProcessError as exc: + raise MathExtError('%s exited with error' % name, exc.stderr, exc.stdout) def convert_dvi_to_png(dvipath, builder): diff --git a/sphinx/testing/fixtures.py b/sphinx/testing/fixtures.py index 4ad4a1f32..ad5a9d1da 100644 --- a/sphinx/testing/fixtures.py +++ b/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'], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) # show version - dot.communicate() + subprocess.run([graphviz_dot, '-V'], stdout=PIPE, stderr=PIPE) # show version return except OSError: # No such file or directory pass diff --git a/tests/test_build_epub.py b/tests/test_build_epub.py index 66ff62768..86e40b65d 100644 --- a/tests/test_build_epub.py +++ b/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) - except OSError: - # command not found - return False - else: - p.communicate() - return p.returncode == 0 + subprocess.run(command, stdout=PIPE, stderr=PIPE, check=True) + return True + except (OSError, CalledProcessError): + return False # command not found or exit with non-zero 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'], - stdout=PIPE, stderr=PIPE) - stdout, stderr = p.communicate() - if p.returncode != 0: - print(stdout) - print(stderr) - assert False, 'epubcheck exited with return code %s' % p.returncode + try: + subprocess.run(['java', '-jar', epubcheck, app.outdir / 'SphinxTests.epub'], + stdout=PIPE, stderr=PIPE, check=True) + except CalledProcessError as exc: + print(exc.stdout) + print(exc.stderr) + assert False, 'epubcheck exited with return code %s' % exc.returncode diff --git a/tests/test_build_gettext.py b/tests/test_build_gettext.py index 793acd5c4..26c63771a 100644 --- a/tests/test_build_gettext.py +++ b/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', - '--locale', 'en_US'], - stdout=PIPE, stderr=PIPE) + args = ['msginit', '--no-translator', '-i', 'markup.pot', '--locale', 'en_US'] + subprocess.run(args, stdout=PIPE, stderr=PIPE, check=True) except OSError: pytest.skip() # most likely msginit was not found - else: - stdout, stderr = p.communicate() - if p.returncode != 0: - print(stdout) - print(stderr) - assert False, 'msginit exited with return code %s' % \ - p.returncode + except CalledProcessError as exc: + print(exc.stdout) + print(exc.stderr) + assert False, 'msginit exited with return code %s' % exc.returncode + assert (app.outdir / 'en_US.po').isfile(), 'msginit failed' try: - p = Popen(['msgfmt', 'en_US.po', '-o', - os.path.join('en', 'LC_MESSAGES', 'test_root.mo')], - stdout=PIPE, stderr=PIPE) + args = ['msgfmt', 'en_US.po', '-o', os.path.join('en', 'LC_MESSAGES', 'test_root.mo')] + subprocess.run(args, stdout=PIPE, stderr=PIPE, check=True) except OSError: pytest.skip() # most likely msgfmt was not found - else: - stdout, stderr = p.communicate() - if p.returncode != 0: - print(stdout) - print(stderr) - assert False, 'msgfmt exited with return code %s' % \ - p.returncode + except CalledProcessError as exc: + print(exc.stdout) + print(exc.stderr) + assert False, 'msgfmt exited with return code %s' % exc.returncode + mo = app.outdir / 'en' / 'LC_MESSAGES' / 'test_root.mo' assert mo.isfile(), 'msgfmt failed' diff --git a/tests/test_build_latex.py b/tests/test_build_latex.py index b6821f425..fa06aa721 100644 --- a/tests/test_build_latex.py +++ b/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) - 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 + subprocess.run(['kpsewhich'] + list(filenames), stdout=PIPE, stderr=PIPE, check=True) 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: - ensuredir(app.config.latex_engine) - # keep a copy of latex file for this engine in case test fails - copyfile(filename, app.config.latex_engine + '/' + filename) - p = Popen([app.config.latex_engine, - '--halt-on-error', - '--interaction=nonstopmode', - '-output-directory=%s' % app.config.latex_engine, - filename], - stdout=PIPE, stderr=PIPE) - except OSError: # most likely the latex executable was not found - raise pytest.skip.Exception - else: - stdout, stderr = p.communicate() - if p.returncode != 0: - print(stdout) - print(stderr) - assert False, '%s exited with return code %s' % ( - app.config.latex_engine, p.returncode) + try: + ensuredir(app.config.latex_engine) + # keep a copy of latex file for this engine in case test fails + copyfile(filename, app.config.latex_engine + '/' + filename) + args = [app.config.latex_engine, + '--halt-on-error', + '--interaction=nonstopmode', + '-output-directory=%s' % app.config.latex_engine, + filename] + subprocess.run(args, stdout=PIPE, stderr=PIPE, cwd=app.outdir, check=True) + except OSError: # most likely the latex executable was not found + raise pytest.skip.Exception + except CalledProcessError as exc: + print(exc.stdout) + print(exc.stderr) + assert False, '%s exited with return code %s' % (app.config.latex_engine, + exc.returncode) def skip_if_requested(testfunc): diff --git a/tests/test_build_texinfo.py b/tests/test_build_texinfo.py index e47042f9c..d6ce8c536 100644 --- a/tests/test_build_texinfo.py +++ b/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: - p = Popen(['makeinfo', '--no-split', 'sphinxtests.texi'], - stdout=PIPE, stderr=PIPE) - except OSError: - raise pytest.skip.Exception # most likely makeinfo was not found - else: - stdout, stderr = p.communicate() - retcode = p.returncode - if retcode != 0: - print(stdout) - print(stderr) - assert False, 'makeinfo exited with return code %s' % retcode - finally: - os.chdir(cwd) + args = ['makeinfo', '--no-split', 'sphinxtests.texi'] + subprocess.run(args, stdout=PIPE, stderr=PIPE, cwd=app.outdir, check=True) + except OSError: + raise pytest.skip.Exception # most likely makeinfo was not found + except CalledProcessError as exc: + print(exc.stdout) + print(exc.stderr) + assert False, 'makeinfo exited with return code %s' % exc.retcode @pytest.mark.sphinx('texinfo', testroot='markup-rubric')