Patchwork py3: teach run-tests.py to handle exe with spaces when --local isn't specified

login
register
mail settings
Submitter Matt Harbison
Date Dec. 13, 2018, 5:24 a.m.
Message ID <bf9c96ebceeab2cbad25.1544678681@Envy>
Download mbox | patch
Permalink /patch/37129/
State Accepted
Headers show

Comments

Matt Harbison - Dec. 13, 2018, 5:24 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1544678327 18000
#      Thu Dec 13 00:18:47 2018 -0500
# Node ID bf9c96ebceeab2cbad25b2aac07c43eb48fbc664
# Parent  008f3491dc5377e9e6f210e0a3f161323049db5d
py3: teach run-tests.py to handle exe with spaces when --local isn't specified

This was the reason that no amount of quoting worked in test-hghave.t.

`os.popen()` needed to be swapped out because while the added quoting around
line 3124 worked on py3, it failed on py2.  See 38d51371792b.  The problem with
`os.system()` was wrongly splitting the command on the space in 'Program Files',
regardless of quoting.  It looks like there are a few other instances of
`os.system()` in core code, so presumably those should be replaced?
Yuya Nishihara - Dec. 13, 2018, 11:17 a.m.
On Thu, 13 Dec 2018 00:24:41 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1544678327 18000
> #      Thu Dec 13 00:18:47 2018 -0500
> # Node ID bf9c96ebceeab2cbad25b2aac07c43eb48fbc664
> # Parent  008f3491dc5377e9e6f210e0a3f161323049db5d
> py3: teach run-tests.py to handle exe with spaces when --local isn't specified

Queued, thanks.
Yuya Nishihara - Dec. 13, 2018, 11:34 a.m.
On Thu, 13 Dec 2018 00:24:41 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1544678327 18000
> #      Thu Dec 13 00:18:47 2018 -0500
> # Node ID bf9c96ebceeab2cbad25b2aac07c43eb48fbc664
> # Parent  008f3491dc5377e9e6f210e0a3f161323049db5d
> py3: teach run-tests.py to handle exe with spaces when --local isn't specified

> -        cmd = b'%s -c "import mercurial; print (mercurial.__path__[0])"'
> +        cmd = b'"%s" -c "import mercurial; print (mercurial.__path__[0])"'
>          cmd = cmd % PYTHON
>          if PYTHON3:
>              cmd = _strpath(cmd)
> -        pipe = os.popen(cmd)
> -        try:
> -            self._hgpath = _bytespath(pipe.read().strip())
> -        finally:
> -            pipe.close()
> +
> +        p = subprocess.Popen(cmd, stdout=subprocess.PIPE)

Added missing shell=True. Otherwise, the test runner would just fail.
Matt Harbison - Dec. 13, 2018, 1:04 p.m.
> On Dec 13, 2018, at 6:34 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Thu, 13 Dec 2018 00:24:41 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1544678327 18000
>> #      Thu Dec 13 00:18:47 2018 -0500
>> # Node ID bf9c96ebceeab2cbad25b2aac07c43eb48fbc664
>> # Parent  008f3491dc5377e9e6f210e0a3f161323049db5d
>> py3: teach run-tests.py to handle exe with spaces when --local isn't specified
> 
>> -        cmd = b'%s -c "import mercurial; print (mercurial.__path__[0])"'
>> +        cmd = b'"%s" -c "import mercurial; print (mercurial.__path__[0])"'
>>         cmd = cmd % PYTHON
>>         if PYTHON3:
>>             cmd = _strpath(cmd)
>> -        pipe = os.popen(cmd)
>> -        try:
>> -            self._hgpath = _bytespath(pipe.read().strip())
>> -        finally:
>> -            pipe.close()
>> +
>> +        p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
> 
> Added missing shell=True. Otherwise, the test runner would just fail.

Are we missing test coverage, or is that a Unix problem in this case?  This was a last minute change after running everything, but it was able to run test-hghave.t.
Yuya Nishihara - Dec. 13, 2018, 1:13 p.m.
On Thu, 13 Dec 2018 08:04:28 -0500, Matt Harbison wrote:
> 
> > On Dec 13, 2018, at 6:34 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >> On Thu, 13 Dec 2018 00:24:41 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1544678327 18000
> >> #      Thu Dec 13 00:18:47 2018 -0500
> >> # Node ID bf9c96ebceeab2cbad25b2aac07c43eb48fbc664
> >> # Parent  008f3491dc5377e9e6f210e0a3f161323049db5d
> >> py3: teach run-tests.py to handle exe with spaces when --local isn't specified
> > 
> >> -        cmd = b'%s -c "import mercurial; print (mercurial.__path__[0])"'
> >> +        cmd = b'"%s" -c "import mercurial; print (mercurial.__path__[0])"'
> >>         cmd = cmd % PYTHON
> >>         if PYTHON3:
> >>             cmd = _strpath(cmd)
> >> -        pipe = os.popen(cmd)
> >> -        try:
> >> -            self._hgpath = _bytespath(pipe.read().strip())
> >> -        finally:
> >> -            pipe.close()
> >> +
> >> +        p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
> > 
> > Added missing shell=True. Otherwise, the test runner would just fail.
> 
> Are we missing test coverage, or is that a Unix problem in this case?  This was a last minute change after running everything, but it was able to run test-hghave.t.
> 

It's platform difference. On Windows, a command string is parsed by an
application program. On Unix, it's parsed by a shell and passed in to
a program in a list form. So, subprocess.Popen(str, shell=False) is
pretty much wrong.

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -3019,7 +3019,7 @@  class TestRunner(object):
             # least on Windows for now, deal with .pydistutils.cfg bugs
             # when they happen.
             nohome = b''
-        cmd = (b'%(exe)s setup.py %(pure)s clean --all'
+        cmd = (b'"%(exe)s" setup.py %(pure)s clean --all'
                b' build %(compiler)s --build-base="%(base)s"'
                b' install --force --prefix="%(prefix)s"'
                b' --install-lib="%(libdir)s"'
@@ -3042,7 +3042,7 @@  class TestRunner(object):
         makedirs(self._bindir)
 
         vlog("# Running", cmd)
-        if os.system(_strpath(cmd)) == 0:
+        if subprocess.call(_strpath(cmd), shell=True) == 0:
             if not self.options.verbose:
                 try:
                     os.remove(installerrs)
@@ -3121,15 +3121,15 @@  class TestRunner(object):
         if self._hgpath is not None:
             return self._hgpath
 
-        cmd = b'%s -c "import mercurial; print (mercurial.__path__[0])"'
+        cmd = b'"%s" -c "import mercurial; print (mercurial.__path__[0])"'
         cmd = cmd % PYTHON
         if PYTHON3:
             cmd = _strpath(cmd)
-        pipe = os.popen(cmd)
-        try:
-            self._hgpath = _bytespath(pipe.read().strip())
-        finally:
-            pipe.close()
+
+        p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
+        out, err = p.communicate()
+
+        self._hgpath = out.strip()
 
         return self._hgpath