[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 at 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 at 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