[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Tom Rini]

The function subprocess.check_output() only exists in Python 2.7 and
later (and there are long term supported distributions that ship with
2.6.x).  Replace this with a call to subprocess.Popen() and then checking
output via communicate()

Signed-off-by: Tom Rini <trini@ti.com>
---
 tools/genboardscfg.py |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
index 805e4eb..6588392 100755
--- a/tools/genboardscfg.py
+++ b/tools/genboardscfg.py
@@ -83,7 +83,8 @@ def check_top_directory():
 def get_make_cmd():
     """Get the command name of GNU Make."""
     try:
-        make_cmd = subprocess.check_output([SHOW_GNU_MAKE])
+        process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
+        make_cmd, unused = process.communicate()
     except subprocess.CalledProcessError:
         print >> sys.stderr, 'GNU Make not found'
         sys.exit(1)
@@ -493,8 +494,10 @@ def main():
             sys.exit(1)
     else:
         try:
-            jobs = int(subprocess.check_output(['getconf',
-                                                '_NPROCESSORS_ONLN']))
+            process = subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
+                                       stdout=subprocess.PIPE)
+            jobstr, unused = process.communicate()
+            jobs = int(jobstr)
         except subprocess.CalledProcessError:
             print 'info: failed to get the number of CPUs. Set jobs to 1'
             jobs = 1
-- 
1.7.0.4

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Simon Glass]

Hi Tom,

On 30 July 2014 15:24, Tom Rini <trini@ti.com> wrote:
> The function subprocess.check_output() only exists in Python 2.7 and
> later (and there are long term supported distributions that ship with
> 2.6.x).  Replace this with a call to subprocess.Popen() and then checking
> output via communicate()
>
> Signed-off-by: Tom Rini <trini@ti.com>

Looks fine as it is - see a few optional comments below.

Acked-by: Simon Glass <sjg@chromium.org>

> ---
>  tools/genboardscfg.py |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> index 805e4eb..6588392 100755
> --- a/tools/genboardscfg.py
> +++ b/tools/genboardscfg.py
> @@ -83,7 +83,8 @@ def check_top_directory():
>  def get_make_cmd():
>      """Get the command name of GNU Make."""
>      try:
> -        make_cmd = subprocess.check_output([SHOW_GNU_MAKE])
> +        process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> +        make_cmd, unused = process.communicate()

Sometimes people use underscore for unused variables, like:

   make_cmd, _ = process.communicate()

but in this case you could also do:

   make_cmd = process.communicate()[0]

>      except subprocess.CalledProcessError:
>          print >> sys.stderr, 'GNU Make not found'
>          sys.exit(1)
> @@ -493,8 +494,10 @@ def main():
>              sys.exit(1)
>      else:
>          try:
> -            jobs = int(subprocess.check_output(['getconf',
> -                                                '_NPROCESSORS_ONLN']))
> +            process = subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> +                                       stdout=subprocess.PIPE)
> +            jobstr, unused = process.communicate()
> +            jobs = int(jobstr)
>          except subprocess.CalledProcessError:
>              print 'info: failed to get the number of CPUs. Set jobs to 1'
>              jobs = 1
> --
> 1.7.0.4

Regards,
Simon

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Tom Rini]

On Wed, Jul 30, 2014 at 04:46:32PM +0100, Simon Glass wrote:

> Hi Tom,
> 
> On 30 July 2014 15:24, Tom Rini <trini@ti.com> wrote:
> > The function subprocess.check_output() only exists in Python 2.7 and
> > later (and there are long term supported distributions that ship with
> > 2.6.x).  Replace this with a call to subprocess.Popen() and then checking
> > output via communicate()
> >
> > Signed-off-by: Tom Rini <trini@ti.com>
> 
> Looks fine as it is - see a few optional comments below.
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 
> > ---
> >  tools/genboardscfg.py |    9 ++++++---
> >  1 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> > index 805e4eb..6588392 100755
> > --- a/tools/genboardscfg.py
> > +++ b/tools/genboardscfg.py
> > @@ -83,7 +83,8 @@ def check_top_directory():
> >  def get_make_cmd():
> >      """Get the command name of GNU Make."""
> >      try:
> > -        make_cmd = subprocess.check_output([SHOW_GNU_MAKE])
> > +        process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> > +        make_cmd, unused = process.communicate()
> 
> Sometimes people use underscore for unused variables, like:
> 
>    make_cmd, _ = process.communicate()
> 
> but in this case you could also do:
> 
>    make_cmd = process.communicate()[0]
> 
> >      except subprocess.CalledProcessError:
> >          print >> sys.stderr, 'GNU Make not found'
> >          sys.exit(1)
> > @@ -493,8 +494,10 @@ def main():
> >              sys.exit(1)
> >      else:
> >          try:
> > -            jobs = int(subprocess.check_output(['getconf',
> > -                                                '_NPROCESSORS_ONLN']))
> > +            process = subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> > +                                       stdout=subprocess.PIPE)
> > +            jobstr, unused = process.communicate()
> > +            jobs = int(jobstr)
> >          except subprocess.CalledProcessError:
> >              print 'info: failed to get the number of CPUs. Set jobs to 1'
> >              jobs = 1

Ah, OK, thanks.  I've cleaned up both to just do communicate()[0] (and
condensed the jobs part back to a single line).  One last sanity check
build going on now.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140730/0a90a949/attachment.pgp>

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Masahiro YAMADA]

Hi Tom,


2014-07-30 23:24 GMT+09:00 Tom Rini <trini@ti.com>:
>
> The function subprocess.check_output() only exists in Python 2.7 and
> later (and there are long term supported distributions that ship with
> 2.6.x).  Replace this with a call to subprocess.Popen() and then checking
> output via communicate()

Unfortunately..  :-(
Anyway, thanks for catching this issue.


> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  tools/genboardscfg.py |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> index 805e4eb..6588392 100755
> --- a/tools/genboardscfg.py
> +++ b/tools/genboardscfg.py
> @@ -83,7 +83,8 @@ def check_top_directory():
>  def get_make_cmd():
>      """Get the command name of GNU Make."""
>      try:
> -        make_cmd = subprocess.check_output([SHOW_GNU_MAKE])
> +        process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> +        make_cmd, unused = process.communicate()
>      except subprocess.CalledProcessError:
>          print >> sys.stderr, 'GNU Make not found'
>          sys.exit(1)



No good.

Unlike check_output(),  the communicate() method never throws
CalledProcessError exception,
which means the lines in except are never run.

When scripts/show-gnu-make fails, the function will not error out, but
just return a null string.
To know if the subprocess succeeded or not, 'returncode' should be checked.


The correct code should be something like this:


def get_make_cmd():
    """Get the command name of GNU Make."""
    process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
    ret = process.communicate()
    if process.returncode:
        print >> sys.stderr, 'GNU Make not found'
        sys.exit(1)
    return ret[0].strip()



>
> @@ -493,8 +494,10 @@ def main():
>              sys.exit(1)
>      else:
>          try:
> -            jobs = int(subprocess.check_output(['getconf',
> -                                                '_NPROCESSORS_ONLN']))
> +            process = subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> +                                       stdout=subprocess.PIPE)
> +            jobstr, unused = process.communicate()
> +            jobs = int(jobstr)
>          except subprocess.CalledProcessError:
>              print 'info: failed to get the number of CPUs. Set jobs to 1'
>              jobs = 1
> --

Ditto.
'except subprocess.CalledProcessError:' is meaningless and never
catches an exception.

In this case, 'getconf' may not exist on some systems.

If the 'getconf' command is not found, Popen() will throw OSError exception.
If the command is found but fails by some reason, int() will throw ValueError.
We cannot handle the other exceptions.

So, we can write the code something like this:

 ...
    else:
        try:
            jobs = int(subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
                                     stdout=subprocess.PIPE).communicate()[0])
        except (OSError, ValueError):
            # getconf command not found or fails
            print 'info: failed to get the number of CPUs. Set jobs to 1'
            jobs = 1
    gen_boards_cfg(jobs)



Best Regards
Masahiro Yamada

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Tom Rini]

On Thu, Jul 31, 2014 at 02:54:43AM +0900, Masahiro YAMADA wrote:
> Hi Tom,
> 
> 
> 2014-07-30 23:24 GMT+09:00 Tom Rini <trini@ti.com>:
> >
> > The function subprocess.check_output() only exists in Python 2.7 and
> > later (and there are long term supported distributions that ship with
> > 2.6.x).  Replace this with a call to subprocess.Popen() and then checking
> > output via communicate()
> 
> Unfortunately..  :-(
> Anyway, thanks for catching this issue.
> 
> 
> > Signed-off-by: Tom Rini <trini@ti.com>
> > ---
> >  tools/genboardscfg.py |    9 ++++++---
> >  1 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> > index 805e4eb..6588392 100755
> > --- a/tools/genboardscfg.py
> > +++ b/tools/genboardscfg.py
> > @@ -83,7 +83,8 @@ def check_top_directory():
> >  def get_make_cmd():
> >      """Get the command name of GNU Make."""
> >      try:
> > -        make_cmd = subprocess.check_output([SHOW_GNU_MAKE])
> > +        process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> > +        make_cmd, unused = process.communicate()
> >      except subprocess.CalledProcessError:
> >          print >> sys.stderr, 'GNU Make not found'
> >          sys.exit(1)
> 
> 
> 
> No good.
> 
> Unlike check_output(),  the communicate() method never throws
> CalledProcessError exception,
> which means the lines in except are never run.
> 
> When scripts/show-gnu-make fails, the function will not error out, but
> just return a null string.
> To know if the subprocess succeeded or not, 'returncode' should be checked.
> 
> 
> The correct code should be something like this:
> 
> 
> def get_make_cmd():
>     """Get the command name of GNU Make."""
>     process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
>     ret = process.communicate()
>     if process.returncode:
>         print >> sys.stderr, 'GNU Make not found'
>         sys.exit(1)
>     return ret[0].strip()
> 
> 
> 
> >
> > @@ -493,8 +494,10 @@ def main():
> >              sys.exit(1)
> >      else:
> >          try:
> > -            jobs = int(subprocess.check_output(['getconf',
> > -                                                '_NPROCESSORS_ONLN']))
> > +            process = subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> > +                                       stdout=subprocess.PIPE)
> > +            jobstr, unused = process.communicate()
> > +            jobs = int(jobstr)
> >          except subprocess.CalledProcessError:
> >              print 'info: failed to get the number of CPUs. Set jobs to 1'
> >              jobs = 1
> > --
> 
> Ditto.
> 'except subprocess.CalledProcessError:' is meaningless and never
> catches an exception.
> 
> In this case, 'getconf' may not exist on some systems.
> 
> If the 'getconf' command is not found, Popen() will throw OSError exception.
> If the command is found but fails by some reason, int() will throw ValueError.
> We cannot handle the other exceptions.
> 
> So, we can write the code something like this:
> 
>  ...
>     else:
>         try:
>             jobs = int(subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
>                                      stdout=subprocess.PIPE).communicate()[0])
>         except (OSError, ValueError):
>             # getconf command not found or fails
>             print 'info: failed to get the number of CPUs. Set jobs to 1'
>             jobs = 1
>     gen_boards_cfg(jobs)

Arg.  Do you want me to fold / rework things like that or do you want to
post a v9 of just this patch, adapted as you've shown?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140730/62dea442/attachment.pgp>

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Tom Rini]

On Wed, Jul 30, 2014 at 02:04:50PM -0400, Tom Rini wrote:
> On Thu, Jul 31, 2014 at 02:54:43AM +0900, Masahiro YAMADA wrote:
> > Hi Tom,
> > 
> > 
> > 2014-07-30 23:24 GMT+09:00 Tom Rini <trini@ti.com>:
> > >
> > > The function subprocess.check_output() only exists in Python 2.7 and
> > > later (and there are long term supported distributions that ship with
> > > 2.6.x).  Replace this with a call to subprocess.Popen() and then checking
> > > output via communicate()
> > 
> > Unfortunately..  :-(
> > Anyway, thanks for catching this issue.
> > 
> > 
> > > Signed-off-by: Tom Rini <trini@ti.com>
> > > ---
> > >  tools/genboardscfg.py |    9 ++++++---
> > >  1 files changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/genboardscfg.py b/tools/genboardscfg.py
> > > index 805e4eb..6588392 100755
> > > --- a/tools/genboardscfg.py
> > > +++ b/tools/genboardscfg.py
> > > @@ -83,7 +83,8 @@ def check_top_directory():
> > >  def get_make_cmd():
> > >      """Get the command name of GNU Make."""
> > >      try:
> > > -        make_cmd = subprocess.check_output([SHOW_GNU_MAKE])
> > > +        process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> > > +        make_cmd, unused = process.communicate()
> > >      except subprocess.CalledProcessError:
> > >          print >> sys.stderr, 'GNU Make not found'
> > >          sys.exit(1)
> > 
> > 
> > 
> > No good.
> > 
> > Unlike check_output(),  the communicate() method never throws
> > CalledProcessError exception,
> > which means the lines in except are never run.
> > 
> > When scripts/show-gnu-make fails, the function will not error out, but
> > just return a null string.
> > To know if the subprocess succeeded or not, 'returncode' should be checked.
> > 
> > 
> > The correct code should be something like this:
> > 
> > 
> > def get_make_cmd():
> >     """Get the command name of GNU Make."""
> >     process = subprocess.Popen([SHOW_GNU_MAKE], stdout=subprocess.PIPE)
> >     ret = process.communicate()
> >     if process.returncode:
> >         print >> sys.stderr, 'GNU Make not found'
> >         sys.exit(1)
> >     return ret[0].strip()
> > 
> > 
> > 
> > >
> > > @@ -493,8 +494,10 @@ def main():
> > >              sys.exit(1)
> > >      else:
> > >          try:
> > > -            jobs = int(subprocess.check_output(['getconf',
> > > -                                                '_NPROCESSORS_ONLN']))
> > > +            process = subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> > > +                                       stdout=subprocess.PIPE)
> > > +            jobstr, unused = process.communicate()
> > > +            jobs = int(jobstr)
> > >          except subprocess.CalledProcessError:
> > >              print 'info: failed to get the number of CPUs. Set jobs to 1'
> > >              jobs = 1
> > > --
> > 
> > Ditto.
> > 'except subprocess.CalledProcessError:' is meaningless and never
> > catches an exception.
> > 
> > In this case, 'getconf' may not exist on some systems.
> > 
> > If the 'getconf' command is not found, Popen() will throw OSError exception.
> > If the command is found but fails by some reason, int() will throw ValueError.
> > We cannot handle the other exceptions.
> > 
> > So, we can write the code something like this:
> > 
> >  ...
> >     else:
> >         try:
> >             jobs = int(subprocess.Popen(['getconf', '_NPROCESSORS_ONLN'],
> >                                      stdout=subprocess.PIPE).communicate()[0])
> >         except (OSError, ValueError):
> >             # getconf command not found or fails
> >             print 'info: failed to get the number of CPUs. Set jobs to 1'
> >             jobs = 1
> >     gen_boards_cfg(jobs)
> 
> Arg.  Do you want me to fold / rework things like that or do you want to
> post a v9 of just this patch, adapted as you've shown?

... doing it this way now, testing, will move from there :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140730/57eacf01/attachment.pgp>

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Masahiro YAMADA]

Hi Tom,


2014-07-31 3:04 GMT+09:00 Tom Rini <trini@ti.com>:

> Arg.  Do you want me to fold / rework things like that or do you want to
> post a v9 of just this patch, adapted as you've shown?


Either is fine for me.

If you were about to apply v8  (I saw you are doing one last build check now),
could you rework things, please?

No more updates from me.


Best Regards
Masahiro Yamada

[U-Boot] [PATCH] tools/genboardscfg.py: Don't rely on subprocess.check_output

[Masahiro YAMADA]

Tom,


>
> Arg.  Do you want me to fold / rework things like that or do you want to
> post a v9 of just this patch, adapted as you've shown?
>

Either is fine for me.

But if you were about to apply v8
(it looks like you are doing the final test),
could you rework things, please?

No more updates from me. Thanks!


Best Regards
Masahiro Yamada