Merge branch 'pylint3k'

7 files changed, 164 insertions, 16 deletions

diff --git a/ranger/ext/img_display.py b/ranger/ext/img_display.py
index e85c4f84..569d03a9 100644
--- a/ranger/ext/img_display.py
+++ b/ranger/ext/img_display.py
@@ -32,6 +32,7 @@ from tempfile import NamedTemporaryFile
 
 from ranger import PY3
 from ranger.core.shared import FileManagerAware, SettingsAware
+from ranger.ext.popen23 import Popen23
 
 W3MIMGDISPLAY_ENV = "W3MIMGDISPLAY_PATH"
 W3MIMGDISPLAY_OPTIONS = []
@@ -167,7 +168,7 @@ class W3MImageDisplayer(ImageDisplayer, FileManagerAware):
         fretint = fcntl.ioctl(fd_stdout, termios.TIOCGWINSZ, farg)
         rows, cols, xpixels, ypixels = struct.unpack("HHHH", fretint)
         if xpixels == 0 and ypixels == 0:
-            with Popen(
+            with Popen23(
                 [self.binary_path, "-test"],
                 stdout=PIPE,
                 universal_newlines=True,
diff --git a/ranger/ext/popen23.py b/ranger/ext/popen23.py
new file mode 100644
index 00000000..944ed3dd
--- /dev/null
+++ b/ranger/ext/popen23.py
@@ -0,0 +1,42 @@
+# This file is part of ranger, the console file manager.
+# License: GNU GPL version 3, see the file "AUTHORS" for details.
+
+from __future__ import absolute_import
+
+from contextlib import contextmanager
+
+from subprocess import Popen
+
+try:
+    from ranger import PY3
+except ImportError:
+    from sys import version_info
+    PY3 = version_info[0] >= 3
+
+
+# COMPAT: Python 2 (and Python <=3.2) subprocess.Popen objects aren't
+#         context managers. We don't care about early Python 3 but we do want
+#         to wrap Python 2's Popen. There's no harm in always using this Popen
+#         but it is only necessary when used with with-statements. This can be
+#         removed once we ditch Python 2 support.
+@contextmanager
+def Popen23(*args, **kwargs):  # pylint: disable=invalid-name
+    if PY3:
+        yield Popen(*args, **kwargs)
+        return
+    else:
+        popen2 = Popen(*args, **kwargs)
+    try:
+        yield popen2
+    finally:
+        # From Lib/subprocess.py Popen.__exit__:
+        if popen2.stdout:
+            popen2.stdout.close()
+        if popen2.stderr:
+            popen2.stderr.close()
+        try:  # Flushing a BufferedWriter may raise an error
+            if popen2.stdin:
+                popen2.stdin.close()
+        finally:
+            # Wait for the process to terminate, to avoid zombies.
+            popen2.wait()
diff --git a/ranger/ext/popen_forked.py b/ranger/ext/popen_forked.py
index 85c8448b..4b171286 100644
--- a/ranger/ext/popen_forked.py
+++ b/ranger/ext/popen_forked.py
@@ -4,7 +4,7 @@
 from __future__ import (absolute_import, division, print_function)
 
 import os
-import subprocess
+from subprocess import Popen
 
 
 def Popen_forked(*args, **kwargs):  # pylint: disable=invalid-name
@@ -21,9 +21,7 @@ def Popen_forked(*args, **kwargs):  # pylint: disable=invalid-name
         with open(os.devnull, 'r') as null_r, open(os.devnull, 'w') as null_w:
             kwargs['stdin'] = null_r
             kwargs['stdout'] = kwargs['stderr'] = null_w
-            with subprocess.Popen(*args, **kwargs):
-                # Just to enable using with
-                pass
+            Popen(*args, **kwargs)  # pylint: disable=consider-using-with
         os._exit(0)  # pylint: disable=protected-access
     else:
         os.wait()
diff --git a/ranger/ext/rifle.py b/ranger/ext/rifle.py
index bb84b295..0d7b5650 100755
--- a/ranger/ext/rifle.py
+++ b/ranger/ext/rifle.py
@@ -18,9 +18,10 @@ from __future__ import (absolute_import, division, print_function)
 
 import os.path
 import re
-from subprocess import Popen, PIPE
+from subprocess import PIPE
 import sys
 
+
 __version__ = 'rifle 1.9.3'
 
 # Options and constants that a user might want to change:
@@ -70,6 +71,66 @@ except ImportError:
 
 
 try:
+    from ranger.ext.popen23 import Popen23
+except ImportError:
+    # COMPAT: Python 2 (and Python <=3.2) subprocess.Popen objects aren't
+    #         context managers. We don't care about early Python 3 but we do
+    #         want to wrap Python 2's Popen. There's no harm in always using
+    #         this Popen but it is only necessary when used with
+    #         with-statements. This can be removed once we ditch Python 2
+    #         support.
+    from contextlib import contextmanager
+    # pylint: disable=ungrouped-imports
+    from subprocess import Popen, TimeoutExpired
+
+    try:
+        from ranger import PY3
+    except ImportError:
+        from sys import version_info
+        PY3 = version_info[0] >= 3
+
+    @contextmanager
+    def Popen23(*args, **kwargs):  # pylint: disable=invalid-name
+        if PY3:
+            yield Popen(*args, **kwargs)
+            return
+        else:
+            popen2 = Popen(*args, **kwargs)
+        try:
+            yield popen2
+        finally:
+            # From Lib/subprocess.py Popen.__exit__:
+            if popen2.stdout:
+                popen2.stdout.close()
+            if popen2.stderr:
+                popen2.stderr.close()
+            try:  # Flushing a BufferedWriter may raise an error
+                if popen2.stdin:
+                    popen2.stdin.close()
+            except KeyboardInterrupt:
+                # https://bugs.python.org/issue25942
+                # In the case of a KeyboardInterrupt we assume the SIGINT
+                # was also already sent to our child processes.  We can't
+                # block indefinitely as that is not user friendly.
+                # If we have not already waited a brief amount of time in
+                # an interrupted .wait() or .communicate() call, do so here
+                # for consistency.
+                # pylint: disable=protected-access
+                if popen2._sigint_wait_secs > 0:
+                    try:
+                        # pylint: disable=no-member
+                        popen2._wait(timeout=popen2._sigint_wait_secs)
+                    except TimeoutExpired:
+                        pass
+                popen2._sigint_wait_secs = 0  # Note that this's been done.
+                # pylint: disable=lost-exception
+                return  # resume the KeyboardInterrupt
+            finally:
+                # Wait for the process to terminate, to avoid zombies.
+                popen2.wait()
+
+
+try:
     from ranger.ext.popen_forked import Popen_forked
 except ImportError:
     def Popen_forked(*args, **kwargs):  # pylint: disable=invalid-name
@@ -85,8 +146,7 @@ except ImportError:
             ) as null_w:
                 kwargs["stdin"] = null_r
                 kwargs["stdout"] = kwargs["stderr"] = null_w
-                with Popen(*args, **kwargs):
-                    pass
+                Popen(*args, **kwargs)  # pylint: disable=consider-using-with
             os._exit(0)  # pylint: disable=protected-access
         return True
 
@@ -259,14 +319,14 @@ class Rifle(object):  # pylint: disable=too-many-instance-attributes
         self._mimetype, _ = mimetypes.guess_type(fname)
 
         if not self._mimetype:
-            with Popen(
+            with Popen23(
                 ["file", "--mime-type", "-Lb", fname], stdout=PIPE, stderr=PIPE
             ) as process:
                 mimetype, _ = process.communicate()
             self._mimetype = mimetype.decode(ENCODING).strip()
             if self._mimetype == 'application/octet-stream':
                 try:
-                    with Popen(
+                    with Popen23(
                         ["mimetype", "--output-format", "%m", fname],
                         stdout=PIPE,
                         stderr=PIPE,
@@ -445,7 +505,7 @@ class Rifle(object):  # pylint: disable=too-many-instance-attributes
                 if 'f' in flags or 't' in flags:
                     Popen_forked(cmd, env=self.hook_environment(os.environ))
                 else:
-                    with Popen(
+                    with Popen23(
                         cmd, env=self.hook_environment(os.environ)
                     ) as process:
                         process.wait()
@@ -528,7 +588,7 @@ def main():  # pylint: disable=too-many-locals
         label = options.p
 
     if options.w is not None and not options.l:
-        with Popen([options.w] + list(positional)) as process:
+        with Popen23([options.w] + list(positional)) as process:
             process.wait()
     else:
         # Start up rifle
diff --git a/ranger/ext/spawn.py b/ranger/ext/spawn.py
index d4590c48..c237d88a 100644
--- a/ranger/ext/spawn.py
+++ b/ranger/ext/spawn.py
@@ -4,7 +4,10 @@
 from __future__ import (absolute_import, division, print_function)
 
 from os import devnull
-from subprocess import Popen, PIPE, CalledProcessError
+from subprocess import PIPE, CalledProcessError
+
+from ranger.ext.popen23 import Popen23
+
 ENCODING = 'utf-8'
 
 
@@ -28,11 +31,11 @@ def check_output(popenargs, **kwargs):
     kwargs.setdefault('shell', isinstance(popenargs, str))
 
     if 'stderr' in kwargs:
-        with Popen(popenargs, **kwargs) as process:
+        with Popen23(popenargs, **kwargs) as process:
             stdout, _ = process.communicate()
     else:
         with open(devnull, mode='w') as fd_devnull:
-            with Popen(popenargs, stderr=fd_devnull, **kwargs) as process:
+            with Popen23(popenargs, stderr=fd_devnull, **kwargs) as process:
                 stdout, _ = process.communicate()
 
     if process.returncode != 0:
diff --git a/tests/pylint/py2_compat.py b/tests/pylint/py2_compat.py
index f3c4398c..7e136148 100644
--- a/tests/pylint/py2_compat.py
+++ b/tests/pylint/py2_compat.py
@@ -45,7 +45,12 @@ class Py2CompatibilityChecker(BaseChecker):
         "E4220": ('Use explicit format spec numbering',
                   "implicit-format-spec",
                   'Python 2.6 does not support implicit format spec numbering'
-                  ' "{}", use explicit numbering "{0}" or keywords "{key}".')
+                  ' "{}", use explicit numbering "{0}" or keywords "{key}".'),
+        "E4230": ("Use popen23.Popen with with-statements",
+                  "with-popen23",
+                  "Python 2 subprocess.Popen objects were not contextmanagers,"
+                  "popen23.Popen wraps them to enable use with"
+                  "with-statements."),
     }
     # This class variable declares the options
     # that are configurable by the user.
@@ -116,6 +121,19 @@ class Py2CompatibilityChecker(BaseChecker):
                     self.add_message("implicit-format-spec", node=node,
                                      confidence=HIGH)
 
+    def visit_with(self, node):
+        """Make sure subprocess.Popen objects aren't used in with-statements"""
+        for (cm, _) in node.items:
+            if isinstance(cm, astroid.nodes.Call):
+                if ((isinstance(cm.func, astroid.nodes.Name)
+                    and cm.func.name.endswith("Popen")
+                    and (node.root().scope_lookup(node.root(), "Popen")[1][0]
+                         ).modname == "subprocess")
+                    or (isinstance(cm.func, astroid.nodes.Attribute)
+                        and cm.func.expr == "subprocess"
+                        and cm.func.attrname == "Popen")):
+                    self.add_message("with-popen23", node=node, confidence=HIGH)
+
 
 def register(linter):
     """This required method auto registers the checker.
diff --git a/tests/pylint/test_py2_compat.py b/tests/pylint/test_py2_compat.py
index bd1ace65..7156aba7 100644
--- a/tests/pylint/test_py2_compat.py
+++ b/tests/pylint/test_py2_compat.py
@@ -115,6 +115,32 @@ class TestPy2CompatibilityChecker(pylint.testutils.CheckerTestCase):
         ):
             self.checker.visit_call(implicit_format_spec)
 
+    def test_with_Popen(self):
+        with_subprocess_Popen, with_Popen, with_Popen23 = astroid.extract_node("""
+        import subprocess
+        with subprocess.Popen(): #@
+            pass
+
+        from subprocess import Popen
+        with Popen(): #@
+            pass
+
+        from ranger.ext.popen23 import Popen23
+        with Popen23(): #@
+            pass
+        """)
+
+        with self.assertAddsMessages(
+            pylint.testutils.Message(
+                msg_id='with-popen23',
+                node=with_Popen,
+            ),
+        ):
+            self.checker.visit_with(with_subprocess_Popen)
+            self.checker.visit_with(with_Popen)
+        with self.assertNoMessages():
+            self.checker.visit_with(with_Popen23)
+
     # # These checks still exist as old-division and no-absolute-import
     # def test_division_without_import(self):
     #     division = astroid.extract_node("""