[diffoscope] 02/05: {command, feeders, diff}: replaces subprocess.Popen by .run

Mattia Rizzolo mattia at debian.org
Wed Feb 28 21:07:51 CET 2018


This is an automated email from the git hooks/post-receive script.

mattia pushed a commit to branch master
in repository diffoscope.

commit f93fb4c182b150da4d2d05b869c86beb9f59af49
Author: Juliana Oliveira <juliana.orod at gmail.com>
Date:   Sat Feb 10 17:20:15 2018 -0200

    {command, feeders, diff}: replaces subprocess.Popen by .run
    
    Popen requires thread/IO micromanaging, and locks some interactions
    with multiprocess lib due to its combined behavior with subprocess.PIPE, being
    unusable with .map function. Subprocess.run simplifies command execution
    by managing threads/IO and is also recommended over Popen since Python
    3.5.
    
    As a result, stderr and stdout were also simplified.
    
    Signed-off-by: Juliana Oliveira <juliana.orod at gmail.com>
    Signed-off-by: Mattia Rizzolo <mattia at debian.org>
---
 diffoscope/comparators/utils/command.py | 68 +++++++++++++++------------------
 diffoscope/comparators/zip.py           |  2 -
 diffoscope/diff.py                      |  7 ++--
 diffoscope/feeders.py                   |  4 +-
 4 files changed, 34 insertions(+), 47 deletions(-)

diff --git a/diffoscope/comparators/utils/command.py b/diffoscope/comparators/utils/command.py
index 1d4cfde..0a1634c 100644
--- a/diffoscope/comparators/utils/command.py
+++ b/diffoscope/comparators/utils/command.py
@@ -28,11 +28,15 @@ logger = logging.getLogger(__name__)
 
 
 class Command(object, metaclass=abc.ABCMeta):
+
+    MAX_STDERR_LINES = 50
+
     def __init__(self, path):
         self._path = path
 
     def start(self):
         logger.debug("Executing %s", ' '.join([shlex.quote(x) for x in self.cmdline()]))
+
         self._stdin = self.stdin()
         # "stdin" used to be a feeder but we didn't need the functionality so
         # it was simplified into the current form. it can be recovered from git
@@ -40,17 +44,15 @@ class Command(object, metaclass=abc.ABCMeta):
         # consider using a shell pipeline ("sh -ec $script") to implement what
         # you need, because that involves much less code - like it or not (I
         # don't) shell is still the most readable option for composing processes
-        self._process = subprocess.Popen(self.cmdline(),
-                                         shell=False, close_fds=True,
-                                         env=self.env(),
-                                         stdin=self._stdin,
-                                         stdout=subprocess.PIPE,
-                                         stderr=subprocess.PIPE)
-        self._stderr = io.BytesIO()
-        self._stderr_line_count = 0
-        self._stderr_reader = threading.Thread(target=self._read_stderr)
-        self._stderr_reader.daemon = True
-        self._stderr_reader.start()
+        self._process = subprocess.run(self.cmdline(),
+                                       shell=False, close_fds=True,
+                                       env=self.env(),
+                                       stdin=self._stdin,
+                                       stdout=subprocess.PIPE,
+                                       stderr=subprocess.PIPE)
+
+
+        self.stderr = self._read_stderr()
 
     @property
     def path(self):
@@ -74,42 +76,32 @@ class Command(object, metaclass=abc.ABCMeta):
         return line
 
     def poll(self):
-        return self._process.poll()
+        pass
 
     def terminate(self):
-        return self._process.terminate()
+        pass
 
     def wait(self):
-        self._stderr_reader.join()
-        returncode = self._process.wait()
-        logger.debug(
-            "%s returned (exit code: %d)",
-            ' '.join([shlex.quote(x) for x in self.cmdline()]),
-            returncode,
-        )
-        if self._stdin:
-            self._stdin.close()
-        return returncode
-
-    MAX_STDERR_LINES = 50
+        return self._process.returncode
 
     def _read_stderr(self):
-        for line in iter(self._process.stderr.readline, b''):
-            self._stderr_line_count += 1
-            if self._stderr_line_count <= Command.MAX_STDERR_LINES:
-                self._stderr.write(line)
-        if self._stderr_line_count > Command.MAX_STDERR_LINES:
-            self._stderr.write('[ {} lines ignored ]\n'.format(self._stderr_line_count - Command.MAX_STDERR_LINES).encode('utf-8'))
-        self._process.stderr.close()
+        buf = ""
+        lines = self._process.stderr.splitlines(True)
+        for index, line in enumerate(lines):
+          if index >= Command.MAX_STDERR_LINES:
+              break
+          buf += line.decode('utf-8', errors='replace')
 
-    @property
-    def stderr_content(self):
-        return self._stderr.getvalue().decode('utf-8', errors='replace')
+        if len(lines) > Command.MAX_STDERR_LINES:
+          buf += '[ {} lines ignored ]\n'.format(
+            len(lines) - Command.MAX_STDERR_LINES)
+
+        return buf
 
     @property
-    def stderr(self):
-        return self._stderr
+    def stderr_content(self):
+        return self.stderr
 
     @property
     def stdout(self):
-        return self._process.stdout
+        return self._process.stdout.splitlines(True)
diff --git a/diffoscope/comparators/zip.py b/diffoscope/comparators/zip.py
index 60d41ac..e626138 100644
--- a/diffoscope/comparators/zip.py
+++ b/diffoscope/comparators/zip.py
@@ -123,10 +123,8 @@ class MozillaZipCommandMixin(object):
     def wait(self):
         # zipinfo emits an error when reading Mozilla-optimized ZIPs,
         # which is fine to ignore.
-        super(Zipinfo, self).wait()
         return 0
 
-
 class MozillaZipinfo(MozillaZipCommandMixin, Zipinfo):
     pass
 
diff --git a/diffoscope/diff.py b/diffoscope/diff.py
index bb6df57..5efab55 100644
--- a/diffoscope/diff.py
+++ b/diffoscope/diff.py
@@ -79,12 +79,11 @@ class DiffParser(object):
         return self._success
 
     def parse(self):
-        for line in self._output:
+        for line in self._output.splitlines(True):
             self._action = self._action(line.decode('utf-8', errors='replace'))
 
         self._action('')
         self._success = True
-        self._output.close()
 
     def read_headers(self, line):
         if not line:
@@ -177,15 +176,15 @@ def run_diff(fifo1, fifo2, end_nl_q1, end_nl_q2):
 
     logger.debug("Running %s", ' '.join(cmd))
 
-    p = subprocess.Popen(
+    p = subprocess.run(
         cmd,
         bufsize=1,
         stdout=subprocess.PIPE,
         stderr=subprocess.STDOUT,
     )
+
     parser = DiffParser(p.stdout, end_nl_q1, end_nl_q2)
     parser.parse()
-    p.wait()
 
     logger.debug(
         "%s: returncode %d, parsed %s",
diff --git a/diffoscope/feeders.py b/diffoscope/feeders.py
index 019ece5..f064214 100644
--- a/diffoscope/feeders.py
+++ b/diffoscope/feeders.py
@@ -86,14 +86,12 @@ def from_command(command):
                 command.filter,
             )
             end_nl = feeder(out_file)
-            if command.poll() is None:
-                command.terminate()
             returncode = command.wait()
         if returncode not in (0, -signal.SIGTERM):
             raise subprocess.CalledProcessError(
                 returncode,
                 command.cmdline(),
-                output=command.stderr.getvalue(),
+                output=command.stderr,
             )
         return end_nl
     return feeder

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/reproducible/diffoscope.git


More information about the diffoscope mailing list