Patchwork [V3] posix: update os.popen with subprocess (issue4746)

login
register
mail settings
Submitter Rishabh Madan
Date March 3, 2017, 9:32 p.m.
Message ID <96bd01e81f0c8402392d.1488576757@bunty>
Download mbox | patch
Permalink /patch/18906/
State Superseded
Headers show

Comments

Rishabh Madan - March 3, 2017, 9:32 p.m.
# HG changeset patch
# User Rishabh Madan <rishabhmadan96@gmail.com>
# Date 1488572534 -19800
#      Sat Mar 04 01:52:14 2017 +0530
# Node ID 96bd01e81f0c8402392daa9f41cac72e31cbdc32
# Parent  71f692f1f678d86ffb4f95a3621aacfdaeb96b05
posix: update os.popen with subprocess (issue4746)

While importing a patch, we use a patch tool, say 'patch --merge', and the
patch doesn't merge cleanly, 'patch --merge' will return with exit status 1,
but 'hg import' displays 'abort: patch command failed: exited with status 256'.
This patch replaces the deprecated os.popen with subprocess.Popen with
conditional statements for 'r', 'rb' and 'w' mode and raise ValueError otherwise.
Yuya Nishihara - March 4, 2017, 2:11 a.m.
On Sat, 04 Mar 2017 03:02:37 +0530, Rishabh Madan wrote:
> # HG changeset patch
> # User Rishabh Madan <rishabhmadan96@gmail.com>
> # Date 1488572534 -19800
> #      Sat Mar 04 01:52:14 2017 +0530
> # Node ID 96bd01e81f0c8402392daa9f41cac72e31cbdc32
> # Parent  71f692f1f678d86ffb4f95a3621aacfdaeb96b05
> posix: update os.popen with subprocess (issue4746)

Oops, I noticed this wasn't that simple. The issue 4746 says the exit code
is wrongly interpreted, which is a return value of fp.close(), where fp would
be closed by pclose(). However, a file created by subprocess isn't special, so
its close() would return no value.

https://docs.python.org/2/library/subprocess.html#replacing-os-popen-os-popen2-os-popen3

>  def popen(command, mode='r'):
> -    return os.popen(command, mode)
> +    if 'r' in mode:
> +        return subprocess.Popen(command, shell=True,
> +            stdout=subprocess.PIPE).stdout
> +    elif mode == 'w':
> +        return subprocess.Popen(command, shell=True,
> +            stdin=subprocess.PIPE).stdin

'w' could also have 'b' flag.
Yuya Nishihara - March 5, 2017, 3:43 a.m.
On Sat, 4 Mar 2017 12:40:54 +0530, Rishabh Madan wrote:
> > Oops, I noticed this wasn't that simple. The issue 4746 says the exit code
> > is wrongly interpreted, which is a return value of fp.close(), where fp
> > would
> > be closed by pclose(). However, a file created by subprocess isn't
> > special, so
> > its close() would return no value.
> 
> So is there any other way we could resolve this issue

Some ideas:

 a. reintroduce old version of explainexit() by different name
 b. stop using util.popen(), and add new utility function that constructs
    subprocess.Popen() in friendlier way
 c. add wrapper class to emulate behavior of popen

(a) is simple, though it might not be nice in API point of view. The simplicity
should sell. (b) seems also good as it would eventually get rid of util.popen*()
family, which appear to rely on GC to wait child processes. (c) would work but
seems cumbersome so I don't like it.

> or should we just
> stick with no return value?

No. It's a bug. We'll need a test to cover a failure of external patch command.

Patch

diff -r 71f692f1f678 -r 96bd01e81f0c mercurial/posix.py
--- a/mercurial/posix.py	Wed Mar 01 20:22:04 2017 +0100
+++ b/mercurial/posix.py	Sat Mar 04 01:52:14 2017 +0530
@@ -16,6 +16,7 @@ 
 import re
 import select
 import stat
+import subprocess
 import sys
 import tempfile
 import unicodedata
@@ -419,7 +420,14 @@ 
     return cmd
 
 def popen(command, mode='r'):
-    return os.popen(command, mode)
+    if 'r' in mode:
+        return subprocess.Popen(command, shell=True,
+            stdout=subprocess.PIPE).stdout
+    elif mode == 'w':
+        return subprocess.Popen(command, shell=True,
+            stdin=subprocess.PIPE).stdin
+    else:
+        raise ValueError
 
 def testpid(pid):
     '''return False if pid dead, True if running or not sure'''