Patchwork [2,of,5] setup: update runcmd() to also return the exit status

login
register
mail settings
Submitter Adam Simpkins
Date June 26, 2017, 6:37 p.m.
Message ID <a687d5c7ac85481471d9.1498502261@devbig125.prn1.facebook.com>
Download mbox | patch
Permalink /patch/21740/
State Accepted
Headers show

Comments

Adam Simpkins - June 26, 2017, 6:37 p.m.
# HG changeset patch
# User Adam Simpkins <simpkins@fb.com>
# Date 1498501890 25200
#      Mon Jun 26 11:31:30 2017 -0700
# Node ID a687d5c7ac85481471d973aa06c6fd4b1dad91f0
# Parent  532781f3e93229a495ca33d7569787d4bab88756
setup: update runcmd() to also return the exit status

Update the runcmd() helper function so it also returns the process exit status.
This allows callers to more definitively determine if a command failed, rather
than testing only for the presence of data on stderr.

I don't expect this to have any behavioral changes for now: the commands
invoked by setup generally should print data on stderr if and only if they
failed.
Yuya Nishihara - June 28, 2017, 2:22 p.m.
On Mon, 26 Jun 2017 11:37:41 -0700, Adam Simpkins wrote:
> # HG changeset patch
> # User Adam Simpkins <simpkins@fb.com>
> # Date 1498501890 25200
> #      Mon Jun 26 11:31:30 2017 -0700
> # Node ID a687d5c7ac85481471d973aa06c6fd4b1dad91f0
> # Parent  532781f3e93229a495ca33d7569787d4bab88756
> setup: update runcmd() to also return the exit status
> 
> Update the runcmd() helper function so it also returns the process exit status.
> This allows callers to more definitively determine if a command failed, rather
> than testing only for the presence of data on stderr.
> 
> I don't expect this to have any behavioral changes for now: the commands
> invoked by setup generally should print data on stderr if and only if they
> failed.
> 
> diff --git a/setup.py b/setup.py
> --- a/setup.py
> +++ b/setup.py
> @@ -146,10 +146,10 @@
>      p = subprocess.Popen(cmd, stdout=subprocess.PIPE,
>                           stderr=subprocess.PIPE, env=env)
>      out, err = p.communicate()
> -    return out, err
> +    return p.returncode, out, err

if sys.platform == 'darwin' and os.path.exists('/usr/bin/xcodebuild'):
    version = runcmd(['/usr/bin/xcodebuild', '-version'], {})[0].splitlines()

Perhaps, this should be updated as well.
Adam Simpkins - June 28, 2017, 6:11 p.m.
On Jun 28, Yuya Nishihara wrote:
> On Mon, 26 Jun 2017 11:37:41 -0700, Adam Simpkins wrote:
> > # HG changeset patch
> > # User Adam Simpkins <simpkins@fb.com>
> > # Date 1498501890 25200
> > #      Mon Jun 26 11:31:30 2017 -0700
> > # Node ID a687d5c7ac85481471d973aa06c6fd4b1dad91f0
> > # Parent  532781f3e93229a495ca33d7569787d4bab88756
> > setup: update runcmd() to also return the exit status
> > 
> > Update the runcmd() helper function so it also returns the process exit status.
> > This allows callers to more definitively determine if a command failed, rather
> > than testing only for the presence of data on stderr.
> > 
> > I don't expect this to have any behavioral changes for now: the commands
> > invoked by setup generally should print data on stderr if and only if they
> > failed.
> > 
> > diff --git a/setup.py b/setup.py
> > --- a/setup.py
> > +++ b/setup.py
> > @@ -146,10 +146,10 @@
> >      p = subprocess.Popen(cmd, stdout=subprocess.PIPE,
> >                           stderr=subprocess.PIPE, env=env)
> >      out, err = p.communicate()
> > -    return out, err
> > +    return p.returncode, out, err
> 
> if sys.platform == 'darwin' and os.path.exists('/usr/bin/xcodebuild'):
>     version = runcmd(['/usr/bin/xcodebuild', '-version'], {})[0].splitlines()
> 
> Perhaps, this should be updated as well.

Whoops, I missed that this index needs to be updated.  Thanks for
catching it.  I'll send out a patch.

Patch

diff --git a/setup.py b/setup.py
--- a/setup.py
+++ b/setup.py
@@ -146,10 +146,10 @@ 
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                          stderr=subprocess.PIPE, env=env)
     out, err = p.communicate()
-    return out, err
+    return p.returncode, out, err
 
 def runhg(cmd, env):
-    out, err = runcmd(cmd, env)
+    returncode, out, err = runcmd(cmd, env)
     # If root is executing setup.py, but the repository is owned by
     # another user (as in "sudo python setup.py install") we will get
     # trust warnings since the .hg/hgrc file is untrusted. That is
@@ -159,7 +159,7 @@ 
            if not e.startswith(b'not trusting file') \
               and not e.startswith(b'warning: Not importing') \
               and not e.startswith(b'obsolete feature not enabled')]
-    if err:
+    if err or returncode != 0:
         printf("stderr from '%s':" % (' '.join(cmd)), file=sys.stderr)
         printf(b'\n'.join([b'  ' + e for e in err]), file=sys.stderr)
         return ''
@@ -404,8 +404,8 @@ 
         # here no extension enabled, disabled() lists up everything
         code = ('import pprint; from mercurial import extensions; '
                 'pprint.pprint(extensions.disabled())')
-        out, err = runcmd([sys.executable, '-c', code], env)
-        if err:
+        returncode, out, err = runcmd([sys.executable, '-c', code], env)
+        if err or returncode != 0:
             raise DistutilsExecError(err)
 
         with open(self._indexfilename, 'w') as f: