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.

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?