[PATCH 0/2] V4 replace os.system and os.popen with subbprocess module

[PATCH 0/2] V4 replace os.system and os.popen with subbprocess module

[Robert Yang]

* Changes of V4:
  Fix use readline() for the return value of bb.process.run(), the
  return value is already a string, we can't (or don't have to) use
  readline() for it, the string is what we need.

* Changes of V3:
  Use bb.process.run() rather than bb.process.Popen() to replace os.popen()

* Changes of V2:

  - Remove the 2>/dev/null since we dont' need it.
  - Use the wrapped Popen from bb.process, which is simpler than
    subprocess.Popen(....).

* Original message of V1:
Replace os.popen and os.system with subprocess.Popen and
subprocess.call, since the older functions would fail (more or less) if
the executed program cannot be found, this would cause potential errors
since we don't know whether the problem executed well or not.

For the performance issue, I've done the testing before the patches and
after with the oe-core layer (also the oe-core have applied the similar
patches):

# The sources are on local disk

1) Before applied these pacthes to bitbake and similar patches to oe-core:
$ time bitbake core-image-sato
real    177m50.723s
user    436m1.551s
sys     71m29.588s

2) After applied the pathes:
$ time bitbake core-image-sato
real    176m26.194s
user    436m7.931s
sys     71m1.994s

After applied these patches, the time has reduced 84 seconds, this is
very slight, I think that we can assume this is just a deviation, it
doesn't cause performance problems.

// Robert

The following changes since commit b4c8c74a45e386f99344cf9799eb5294ad6c9e3e:

  hob: update required pygtk to 2.22.0 and gtk+ to 2.20.0 (2012-05-20 09:24:26 +0100)

are available in the git repository at:
  git://git.pokylinux.org/poky-contrib robert/subprocess
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=robert/subprocess

Robert Yang (2):
  replace os.system with subprocess.call
  replace os.popen with subprocess.Popen

 bitbake/lib/bb/fetch2/perforce.py            |   14 ++++++++------
 bitbake/lib/bb/fetch2/svk.py                 |    4 ++--
 bitbake/lib/bb/shell.py                      |    6 +++---
 bitbake/lib/bb/ui/crumbs/builddetailspage.py |    5 +++--
 bitbake/lib/bb/ui/crumbs/hig.py              |    7 ++++---
 bitbake/lib/bb/ui/crumbs/imagedetailspage.py |    3 ++-
 bitbake/lib/bb/ui/ncurses.py                 |    4 ++--
 7 files changed, 24 insertions(+), 19 deletions(-)

[PATCH 1/2] replace os.system with subprocess.call

[Robert Yang]

Replace os.system with subprocess.call since the older function would
fail (more or less) silently if the executed program cannot be found

More info:
http://docs.python.org/library/subprocess.html#subprocess-replacements

[YOCTO #2075]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 bitbake/lib/bb/fetch2/perforce.py            |    3 ++-
 bitbake/lib/bb/shell.py                      |    6 +++---
 bitbake/lib/bb/ui/crumbs/imagedetailspage.py |    3 ++-
 bitbake/lib/bb/ui/ncurses.py                 |    4 ++--
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/bitbake/lib/bb/fetch2/perforce.py b/bitbake/lib/bb/fetch2/perforce.py
index cbdc848..6abf15d 100644
--- a/bitbake/lib/bb/fetch2/perforce.py
+++ b/bitbake/lib/bb/fetch2/perforce.py
@@ -27,6 +27,7 @@ BitBake build tools.
 
 from future_builtins import zip
 import os
+import subprocess
 import logging
 import bb
 from   bb import data
@@ -184,7 +185,7 @@ class Perforce(FetchMethod):
             dest = list[0][len(path)+1:]
             where = dest.find("#")
 
-            os.system("%s%s print -o %s/%s %s" % (p4cmd, p4opt, module, dest[:where], list[0]))
+            subprocess.call("%s%s print -o %s/%s %s" % (p4cmd, p4opt, module, dest[:where], list[0]), shell=True)
             count = count + 1
 
         if count == 0:
diff --git a/bitbake/lib/bb/shell.py b/bitbake/lib/bb/shell.py
index 1dd8d54..a83dedd 100644
--- a/bitbake/lib/bb/shell.py
+++ b/bitbake/lib/bb/shell.py
@@ -214,7 +214,7 @@ class BitBakeShellCommands:
         name = params[0]
         bbfile = self._findProvider( name )
         if bbfile is not None:
-            os.system( "%s %s" % ( os.environ.get( "EDITOR", "vi" ), bbfile ) )
+            subprocess.call( "%s %s" % ( os.environ.get( "EDITOR", "vi" ), bbfile ) , shell=True)
         else:
             print("ERROR: Nothing provides '%s'" % name)
     edit.usage = "<providee>"
@@ -259,7 +259,7 @@ class BitBakeShellCommands:
     def fileEdit( self, params ):
         """Call $EDITOR on a .bb file"""
         name = params[0]
-        os.system( "%s %s" % ( os.environ.get( "EDITOR", "vi" ), completeFilePath( name ) ) )
+        subprocess.call( "%s %s" % ( os.environ.get( "EDITOR", "vi" ), completeFilePath( name ) ) , shell=True)
     fileEdit.usage = "<bbfile>"
 
     def fileRebuild( self, params ):
@@ -370,7 +370,7 @@ SRC_URI = ""
 #}
 """, file=newpackage)
             newpackage.close()
-            os.system( "%s %s/%s" % ( os.environ.get( "EDITOR" ), fulldirname, filename ) )
+            subprocess.call( "%s %s/%s" % ( os.environ.get( "EDITOR" ), fulldirname, filename ) , shell=True)
     new.usage = "<directory> <filename>"
 
     def package( self, params ):
diff --git a/bitbake/lib/bb/ui/crumbs/imagedetailspage.py b/bitbake/lib/bb/ui/crumbs/imagedetailspage.py
index 1cfef80..2453dbc 100755
--- a/bitbake/lib/bb/ui/crumbs/imagedetailspage.py
+++ b/bitbake/lib/bb/ui/crumbs/imagedetailspage.py
@@ -25,6 +25,7 @@ import gtk
 from bb.ui.crumbs.hobcolor import HobColors
 from bb.ui.crumbs.hobwidget import hic, HobViewTable, HobAltButton, HobButton
 from bb.ui.crumbs.hobpages import HobPage
+import subprocess
 
 #
 # ImageDetailsPage
@@ -299,7 +300,7 @@ class ImageDetailsPage (HobPage):
         self.show_all()
 
     def view_files_clicked_cb(self, button, image_addr):
-        os.system("xdg-open /%s" % image_addr)
+        subprocess.call("xdg-open /%s" % image_addr, shell=True)
 
     def refresh_package_detail_box(self, image_size):
         self.package_detail.update_line_widgets("Total image size: ", image_size)
diff --git a/bitbake/lib/bb/ui/ncurses.py b/bitbake/lib/bb/ui/ncurses.py
index 8524446..1425bbd 100644
--- a/bitbake/lib/bb/ui/ncurses.py
+++ b/bitbake/lib/bb/ui/ncurses.py
@@ -47,7 +47,7 @@
 
 from __future__ import division
 import logging
-import os, sys, curses, itertools, time
+import os, sys, curses, itertools, time, subprocess
 import bb
 import xmlrpclib
 from bb import ui
@@ -286,7 +286,7 @@ class NCursesUI:
 #                            bb.error("log data follows (%s)" % logfile)
 #                            number_of_lines = data.getVar("BBINCLUDELOGS_LINES", d)
 #                            if number_of_lines:
-#                                os.system('tail -n%s %s' % (number_of_lines, logfile))
+#                                subprocess.call('tail -n%s %s' % (number_of_lines, logfile), shell=True)
 #                            else:
 #                                f = open(logfile, "r")
 #                                while True:
-- 
1.7.1

[PATCH 2/2] replace os.popen with subprocess.Popen

[Robert Yang]

Replace os.popen with subprocess.Popen since the older function would
fail (more or less) silently if the executed program cannot be found

There is a bb.process.run() which will invoke the Popen to run command,
use it for simplify the code.

For the:
p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot))
...
for file in p4file:
    list = file.split()

in bitbake/lib/bb/fetch2/perforce.py, it should be an error in the past,
since it didn't use readline() to read the pipe, but directly used the
split() for the pipe. Use the bb.process.run would fix the problem since
bb.process.run will return strings.

More info:
http://docs.python.org/library/subprocess.html#subprocess-replacements

[YOCTO #2075]

Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
---
 bitbake/lib/bb/fetch2/perforce.py            |   11 ++++++-----
 bitbake/lib/bb/fetch2/svk.py                 |    4 ++--
 bitbake/lib/bb/ui/crumbs/builddetailspage.py |    5 +++--
 bitbake/lib/bb/ui/crumbs/hig.py              |    7 ++++---
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/bitbake/lib/bb/fetch2/perforce.py b/bitbake/lib/bb/fetch2/perforce.py
index 6abf15d..df3a3a3 100644
--- a/bitbake/lib/bb/fetch2/perforce.py
+++ b/bitbake/lib/bb/fetch2/perforce.py
@@ -91,8 +91,8 @@ class Perforce(FetchMethod):
 
         p4cmd = data.getVar('FETCHCOMMAND_p4', d, True)
         logger.debug(1, "Running %s%s changes -m 1 %s", p4cmd, p4opt, depot)
-        p4file = os.popen("%s%s changes -m 1 %s" % (p4cmd, p4opt, depot))
-        cset = p4file.readline().strip()
+        p4file, errors = bb.process.run("%s%s changes -m 1 %s" % (p4cmd, p4opt, depot))
+        cset = p4file.strip()
         logger.debug(1, "READ %s", cset)
         if not cset:
             return -1
@@ -155,8 +155,8 @@ class Perforce(FetchMethod):
         logger.debug(2, "Fetch: creating temporary directory")
         bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
         data.setVar('TMPBASE', data.expand('${WORKDIR}/oep4.XXXXXX', localdata), localdata)
-        tmppipe = os.popen(data.getVar('MKTEMPDIRCMD', localdata, True) or "false")
-        tmpfile = tmppipe.readline().strip()
+        tmpfile, errors = bb.process.run(data.getVar('MKTEMPDIRCMD', localdata, True) or "false")
+        tmpfile = tmpfile.strip()
         if not tmpfile:
             raise FetchError("Fetch: unable to create temporary directory.. make sure 'mktemp' is in the PATH.", loc)
 
@@ -169,7 +169,8 @@ class Perforce(FetchMethod):
         os.chdir(tmpfile)
         logger.info("Fetch " + loc)
         logger.info("%s%s files %s", p4cmd, p4opt, depot)
-        p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot))
+        p4file, errors = bb.process.run("%s%s files %s" % (p4cmd, p4opt, depot))
+        p4file = p4file.strip()
 
         if not p4file:
             raise FetchError("Fetch: unable to get the P4 files from %s" % depot, loc)
diff --git a/bitbake/lib/bb/fetch2/svk.py b/bitbake/lib/bb/fetch2/svk.py
index 9d34abf..ee3823f 100644
--- a/bitbake/lib/bb/fetch2/svk.py
+++ b/bitbake/lib/bb/fetch2/svk.py
@@ -77,8 +77,8 @@ class Svk(FetchMethod):
         logger.debug(2, "Fetch: creating temporary directory")
         bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
         data.setVar('TMPBASE', data.expand('${WORKDIR}/oesvk.XXXXXX', localdata), localdata)
-        tmppipe = os.popen(data.getVar('MKTEMPDIRCMD', localdata, True) or "false")
-        tmpfile = tmppipe.readline().strip()
+        tmpfile, errors = bb.process.run(data.getVar('MKTEMPDIRCMD', localdata, True) or "false")
+        tmpfile = tmpfile.strip()
         if not tmpfile:
             logger.error()
             raise FetchError("Fetch: unable to create temporary directory.. make sure 'mktemp' is in the PATH.", loc)
diff --git a/bitbake/lib/bb/ui/crumbs/builddetailspage.py b/bitbake/lib/bb/ui/crumbs/builddetailspage.py
index c2d5abc..3ec15d4 100755
--- a/bitbake/lib/bb/ui/crumbs/builddetailspage.py
+++ b/bitbake/lib/bb/ui/crumbs/builddetailspage.py
@@ -23,6 +23,7 @@
 import gtk
 import pango
 import gobject
+import bb.process
 from bb.ui.crumbs.progressbar import HobProgressBar
 from bb.ui.crumbs.hobwidget import hic, HobNotebook, HobAltButton, HobWarpCellRendererText
 from bb.ui.crumbs.runningbuild import RunningBuildTreeView
@@ -96,9 +97,9 @@ class BuildConfigurationTreeView(gtk.TreeView):
         for path in src_config_info.layers:
             import os, os.path
             if os.path.exists(path):
-                f = os.popen('cd %s; git branch 2>&1 | grep "^* " | tr -d "* "' % path)
+                f, errors = bb.process.run('cd %s; git branch 2>&1 | grep "^* " | tr -d "* "' % path)
                 if f:
-                    branch = f.readline().lstrip('\n').rstrip('\n')
+                    branch = f.strip('\n')
                     vars.append(self.set_vars("Branch:", branch))
                     f.close()
                 break
diff --git a/bitbake/lib/bb/ui/crumbs/hig.py b/bitbake/lib/bb/ui/crumbs/hig.py
index 721d145..b3936f8 100644
--- a/bitbake/lib/bb/ui/crumbs/hig.py
+++ b/bitbake/lib/bb/ui/crumbs/hig.py
@@ -25,12 +25,12 @@ import gobject
 import hashlib
 import os
 import re
-import subprocess
 import shlex
 from bb.ui.crumbs.hobcolor import HobColors
 from bb.ui.crumbs.hobwidget import hcc, hic, HobViewTable, HobInfoButton, HobButton, HobAltButton, HobIconChecker
 from bb.ui.crumbs.progressbar import HobProgressBar
 import bb.ui.crumbs.utils
+import bb.process
 
 """
 The following are convenience classes for implementing GNOME HIG compliant
@@ -726,7 +726,8 @@ class DeployImageDialog (CrumbsDialog):
         self.progress_bar.hide()
 
     def popen_read(self, cmd):
-        return os.popen("%s 2>/dev/null" % cmd).read().strip()
+        tmpout, errors = bb.process.run("%s" % cmd)
+        return tmpout.strip()
 
     def find_all_usb_devices(self):
         usb_devs = [ os.readlink(u)
@@ -755,7 +756,7 @@ class DeployImageDialog (CrumbsDialog):
                 cmdline = bb.ui.crumbs.utils.which_terminal()
                 if cmdline:
                     cmdline += "\"sudo dd if=" + self.image_path + " of=" + combo_item + "\""
-                    subprocess.Popen(args=shlex.split(cmdline))
+                    bb.process.run(shlex.split(cmdline))
 
     def update_progress_bar(self, title, fraction, status=None):
         self.progress_bar.update(fraction)
-- 
1.7.1

Re: [PATCH 0/2] V4 replace os.system and os.popen with subbprocess module

[Chris Larson]

On Sun, May 20, 2012 at 5:36 AM, Robert Yang <liezhi.yang@windriver.com> wrote:
> * Changes of V4:
>  Fix use readline() for the return value of bb.process.run(), the
>  return value is already a string, we can't (or don't have to) use
>  readline() for it, the string is what we need.
>
> * Changes of V3:
>  Use bb.process.run() rather than bb.process.Popen() to replace os.popen()
>
> * Changes of V2:
>
>  - Remove the 2>/dev/null since we dont' need it.
>  - Use the wrapped Popen from bb.process, which is simpler than
>    subprocess.Popen(....).
>
> * Original message of V1:
> Replace os.popen and os.system with subprocess.Popen and
> subprocess.call, since the older functions would fail (more or less) if
> the executed program cannot be found, this would cause potential errors
> since we don't know whether the problem executed well or not.
>
> For the performance issue, I've done the testing before the patches and
> after with the oe-core layer (also the oe-core have applied the similar
> patches):
>
> # The sources are on local disk
>
> 1) Before applied these pacthes to bitbake and similar patches to oe-core:
> $ time bitbake core-image-sato
> real    177m50.723s
> user    436m1.551s
> sys     71m29.588s
>
> 2) After applied the pathes:
> $ time bitbake core-image-sato
> real    176m26.194s
> user    436m7.931s
> sys     71m1.994s
>
> After applied these patches, the time has reduced 84 seconds, this is
> very slight, I think that we can assume this is just a deviation, it
> doesn't cause performance problems.

This version looks good to me.
-- 
Christopher Larson

Re: [PATCH 0/2] V4 replace os.system and os.popen with subbprocess module

[Richard Purdie]

On Sun, 2012-05-20 at 20:36 +0800, Robert Yang wrote:
> * Changes of V4:
>   Fix use readline() for the return value of bb.process.run(), the
>   return value is already a string, we can't (or don't have to) use
>   readline() for it, the string is what we need.
> 
> * Changes of V3:
>   Use bb.process.run() rather than bb.process.Popen() to replace os.popen()
> 
> * Changes of V2:
> 
>   - Remove the 2>/dev/null since we dont' need it.
>   - Use the wrapped Popen from bb.process, which is simpler than
>     subprocess.Popen(....).
> 
> * Original message of V1:
> Replace os.popen and os.system with subprocess.Popen and
> subprocess.call, since the older functions would fail (more or less) if
> the executed program cannot be found, this would cause potential errors
> since we don't know whether the problem executed well or not.
> 
> For the performance issue, I've done the testing before the patches and
> after with the oe-core layer (also the oe-core have applied the similar
> patches):
> 
> # The sources are on local disk
> 
> 1) Before applied these pacthes to bitbake and similar patches to oe-core:
> $ time bitbake core-image-sato
> real    177m50.723s
> user    436m1.551s
> sys     71m29.588s
> 
> 2) After applied the pathes:
> $ time bitbake core-image-sato
> real    176m26.194s
> user    436m7.931s
> sys     71m1.994s
> 
> After applied these patches, the time has reduced 84 seconds, this is
> very slight, I think that we can assume this is just a deviation, it
> doesn't cause performance problems.
> 
> // Robert
> 
> The following changes since commit b4c8c74a45e386f99344cf9799eb5294ad6c9e3e:
> 
>   hob: update required pygtk to 2.22.0 and gtk+ to 2.20.0 (2012-05-20 09:24:26 +0100)
> 
> are available in the git repository at:
>   git://git.pokylinux.org/poky-contrib robert/subprocess
>   http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=robert/subprocess
> 
> Robert Yang (2):
>   replace os.system with subprocess.call
>   replace os.popen with subprocess.Popen

Merged to master, thanks.

Richard

Re: [PATCH 2/2] replace os.popen with subprocess.Popen

[Wang, Shane]

Robert, be careful. Your patch 094742bed2fc01d55f572da946fcfa7a48521401 has broke Hob.

I am going to assign the bug https://bugzilla.yoctoproject.org/show_bug.cgi?id=2511 to you and Kai.

The error is:
Traceback (most recent call last):
  File "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builder.py", line 821, in handler_build_started_cb
    self.build_details_page.show_configurations(self.configuration, self.parameters)
  File "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage.py", line 336, in show_configurations
    self.config_tv.show(configurations, params)
  File "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage.py", line 105, in show
    f.close()
AttributeError: 'str' object has no attribute 'close'

--
Shane

Robert Yang wrote on 2012-05-20:

> Replace os.popen with subprocess.Popen since the older function would
> fail (more or less) silently if the executed program cannot be found
> 
> There is a bb.process.run() which will invoke the Popen to run command,
> use it for simplify the code.
> 
> For the:
> p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot))
> ...
> for file in p4file:
>     list = file.split()
> in bitbake/lib/bb/fetch2/perforce.py, it should be an error in the past,
> since it didn't use readline() to read the pipe, but directly used the
> split() for the pipe. Use the bb.process.run would fix the problem since
> bb.process.run will return strings.
> 
> More info:
> http://docs.python.org/library/subprocess.html#subprocess-replacements
> 
> [YOCTO #2075]
> 
> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> ---
>  bitbake/lib/bb/fetch2/perforce.py            |   11 ++++++-----
>  bitbake/lib/bb/fetch2/svk.py                 |    4 ++--
>  bitbake/lib/bb/ui/crumbs/builddetailspage.py |    5 +++--
>  bitbake/lib/bb/ui/crumbs/hig.py              |    7 ++++---
>  4 files changed, 15 insertions(+), 12 deletions(-)
> diff --git a/bitbake/lib/bb/fetch2/perforce.py
> b/bitbake/lib/bb/fetch2/perforce.py index 6abf15d..df3a3a3 100644 ---
> a/bitbake/lib/bb/fetch2/perforce.py +++
> b/bitbake/lib/bb/fetch2/perforce.py @@ -91,8 +91,8 @@ class
> Perforce(FetchMethod):
> 
>          p4cmd = data.getVar('FETCHCOMMAND_p4', d, True)
>          logger.debug(1, "Running %s%s changes -m 1 %s", p4cmd, p4opt,
> depot) -        p4file = os.popen("%s%s changes -m 1 %s" % (p4cmd,
> p4opt, depot)) -        cset = p4file.readline().strip() +       
> p4file, errors = bb.process.run("%s%s changes -m 1 %s" % (p4cmd, p4opt,
> depot)) +        cset = p4file.strip()
>          logger.debug(1, "READ %s", cset)
>          if not cset:
>              return -1
> @@ -155,8 +155,8 @@ class Perforce(FetchMethod):
>          logger.debug(2, "Fetch: creating temporary directory")
>          bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
>          data.setVar('TMPBASE', data.expand('${WORKDIR}/oep4.XXXXXX',
> localdata), localdata)
> -        tmppipe = os.popen(data.getVar('MKTEMPDIRCMD', localdata,
> True) or "false")
> -        tmpfile = tmppipe.readline().strip()
> +        tmpfile, errors = bb.process.run(data.getVar('MKTEMPDIRCMD',
> localdata, True) or "false")
> +        tmpfile = tmpfile.strip()
>          if not tmpfile:
>              raise FetchError("Fetch: unable to create temporary
> directory.. make sure 'mktemp' is in the PATH.", loc)
> 
> @@ -169,7 +169,8 @@ class Perforce(FetchMethod):
>          os.chdir(tmpfile)
>          logger.info("Fetch " + loc)
>          logger.info("%s%s files %s", p4cmd, p4opt, depot)
> -        p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot)) +   
>     p4file, errors = bb.process.run("%s%s files %s" % (p4cmd, p4opt,
> depot)) +        p4file = p4file.strip()
> 
>          if not p4file:
>              raise FetchError("Fetch: unable to get the P4 files from %s" %
> depot, loc)
> diff --git a/bitbake/lib/bb/fetch2/svk.py b/bitbake/lib/bb/fetch2/svk.py
> index 9d34abf..ee3823f 100644
> --- a/bitbake/lib/bb/fetch2/svk.py
> +++ b/bitbake/lib/bb/fetch2/svk.py
> @@ -77,8 +77,8 @@ class Svk(FetchMethod):
>          logger.debug(2, "Fetch: creating temporary directory")
>          bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
>          data.setVar('TMPBASE', data.expand('${WORKDIR}/oesvk.XXXXXX',
> localdata), localdata)
> -        tmppipe = os.popen(data.getVar('MKTEMPDIRCMD', localdata,
> True) or "false")
> -        tmpfile = tmppipe.readline().strip()
> +        tmpfile, errors = bb.process.run(data.getVar('MKTEMPDIRCMD',
> localdata, True) or "false")
> +        tmpfile = tmpfile.strip()
>          if not tmpfile:
>              logger.error()
>              raise FetchError("Fetch: unable to create temporary
> directory.. make sure 'mktemp' is in the PATH.", loc) diff --git
> a/bitbake/lib/bb/ui/crumbs/builddetailspage.py
> b/bitbake/lib/bb/ui/crumbs/builddetailspage.py index c2d5abc..3ec15d4
> 100755 --- a/bitbake/lib/bb/ui/crumbs/builddetailspage.py +++
> b/bitbake/lib/bb/ui/crumbs/builddetailspage.py @@ -23,6 +23,7 @@
>  import gtk import pango import gobject +import bb.process from
>  bb.ui.crumbs.progressbar import HobProgressBar from
>  bb.ui.crumbs.hobwidget import hic, HobNotebook, HobAltButton,
>  HobWarpCellRendererText from bb.ui.crumbs.runningbuild import
>  RunningBuildTreeView
> @@ -96,9 +97,9 @@ class BuildConfigurationTreeView(gtk.TreeView):
>          for path in src_config_info.layers:
>              import os, os.path
>              if os.path.exists(path):
> -                f = os.popen('cd %s; git branch 2>&1 | grep "^* " | tr -d
> "* "' % path)
> +                f, errors = bb.process.run('cd %s; git branch 2>&1 | grep
> "^* " | tr -d "* "' % path)
>                  if f:
> -                    branch = f.readline().lstrip('\n').rstrip('\n')
> +                    branch = f.strip('\n')
>                      vars.append(self.set_vars("Branch:", branch))
>                      f.close()
>                  break
> diff --git a/bitbake/lib/bb/ui/crumbs/hig.py b/bitbake/lib/bb/ui/crumbs/hig.py
> index 721d145..b3936f8 100644
> --- a/bitbake/lib/bb/ui/crumbs/hig.py
> +++ b/bitbake/lib/bb/ui/crumbs/hig.py
> @@ -25,12 +25,12 @@ import gobject
>  import hashlib import os import re -import subprocess import shlex from
>  bb.ui.crumbs.hobcolor import HobColors from bb.ui.crumbs.hobwidget
>  import hcc, hic, HobViewTable, HobInfoButton, HobButton, HobAltButton,
>  HobIconChecker from bb.ui.crumbs.progressbar import HobProgressBar
>  import bb.ui.crumbs.utils
> +import bb.process
> 
>  """
>  The following are convenience classes for implementing GNOME HIG
> compliant
> @@ -726,7 +726,8 @@ class DeployImageDialog (CrumbsDialog):
>          self.progress_bar.hide()
>      def popen_read(self, cmd):
> -        return os.popen("%s 2>/dev/null" % cmd).read().strip()
> +        tmpout, errors = bb.process.run("%s" % cmd)
> +        return tmpout.strip()
> 
>      def find_all_usb_devices(self):
>          usb_devs = [ os.readlink(u)
> @@ -755,7 +756,7 @@ class DeployImageDialog (CrumbsDialog):
>                  cmdline = bb.ui.crumbs.utils.which_terminal()
>                  if cmdline:
>                      cmdline += "\"sudo dd if=" + self.image_path + "
> of=" + combo_item + "\""
> -                    subprocess.Popen(args=shlex.split(cmdline))
> +                    bb.process.run(shlex.split(cmdline))
> 
>      def update_progress_bar(self, title, fraction, status=None):
>          self.progress_bar.update(fraction)

Re: [PATCH 2/2] replace os.popen with subprocess.Popen

[Wang, Shane]

Again, be careful to check whether "deploy image" and "run qemu" work.
Because previously I used "os.popen" and found the frontend process (i.e. the Hob) hangs when the backend process (i.e. the deployment process and the qemu process) is running.

So, I chose "subprocess.Popen", so please have a test with "deploy image" and "run qemu" after you replaced it.

--
Shane

Wang, Shane wrote on 2012-05-29:

> Robert, be careful. Your patch 094742bed2fc01d55f572da946fcfa7a48521401
> has broke Hob.
> 
> I am going to assign the bug
> https://bugzilla.yoctoproject.org/show_bug.cgi?id=2511 to you and Kai.
> 
> The error is:
> Traceback (most recent call last):
>   File
> "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builder.py", line
> 821, in handler_build_started_cb
>     self.build_details_page.show_configurations(self.configuration,
> self.parameters)
>   File
> "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage
> .py", line 336, in show_configurations
>     self.config_tv.show(configurations, params)
>   File
> "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage
> .py", line 105, in show
>     f.close()
> AttributeError: 'str' object has no attribute 'close'
>

Re: [PATCH 2/2] replace os.popen with subprocess.Popen

[Robert Yang]

On 05/29/2012 10:58 PM, Wang, Shane wrote:
> Robert, be careful. Your patch 094742bed2fc01d55f572da946fcfa7a48521401 has broke Hob.
>

Sorry, in bitbake/lib/bb/ui/crumbs/builddetailspage.py:

103                     branch = f.strip('\n')
104                     vars.append(self.set_vars("Branch:", branch))
105                     f.close()
106                 break

Remove the f.close should be OK, I will check the patch again to see whether
there is such an error in other corner, and then send a pull request today.

// Robert

> I am going to assign the bug https://bugzilla.yoctoproject.org/show_bug.cgi?id=2511 to you and Kai.
>
> The error is:
> Traceback (most recent call last):
>    File "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builder.py", line 821, in handler_build_started_cb
>      self.build_details_page.show_configurations(self.configuration, self.parameters)
>    File "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage.py", line 336, in show_configurations
>      self.config_tv.show(configurations, params)
>    File "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage.py", line 105, in show
>      f.close()
> AttributeError: 'str' object has no attribute 'close'
>
> --
> Shane
>
> Robert Yang wrote on 2012-05-20:
>
>> Replace os.popen with subprocess.Popen since the older function would
>> fail (more or less) silently if the executed program cannot be found
>>
>> There is a bb.process.run() which will invoke the Popen to run command,
>> use it for simplify the code.
>>
>> For the:
>> p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot))
>> ...
>> for file in p4file:
>>      list = file.split()
>> in bitbake/lib/bb/fetch2/perforce.py, it should be an error in the past,
>> since it didn't use readline() to read the pipe, but directly used the
>> split() for the pipe. Use the bb.process.run would fix the problem since
>> bb.process.run will return strings.
>>
>> More info:
>> http://docs.python.org/library/subprocess.html#subprocess-replacements
>>
>> [YOCTO #2075]
>>
>> Signed-off-by: Robert Yang<liezhi.yang@windriver.com>
>> ---
>>   bitbake/lib/bb/fetch2/perforce.py            |   11 ++++++-----
>>   bitbake/lib/bb/fetch2/svk.py                 |    4 ++--
>>   bitbake/lib/bb/ui/crumbs/builddetailspage.py |    5 +++--
>>   bitbake/lib/bb/ui/crumbs/hig.py              |    7 ++++---
>>   4 files changed, 15 insertions(+), 12 deletions(-)
>> diff --git a/bitbake/lib/bb/fetch2/perforce.py
>> b/bitbake/lib/bb/fetch2/perforce.py index 6abf15d..df3a3a3 100644 ---
>> a/bitbake/lib/bb/fetch2/perforce.py +++
>> b/bitbake/lib/bb/fetch2/perforce.py @@ -91,8 +91,8 @@ class
>> Perforce(FetchMethod):
>>
>>           p4cmd = data.getVar('FETCHCOMMAND_p4', d, True)
>>           logger.debug(1, "Running %s%s changes -m 1 %s", p4cmd, p4opt,
>> depot) -        p4file = os.popen("%s%s changes -m 1 %s" % (p4cmd,
>> p4opt, depot)) -        cset = p4file.readline().strip() +
>> p4file, errors = bb.process.run("%s%s changes -m 1 %s" % (p4cmd, p4opt,
>> depot)) +        cset = p4file.strip()
>>           logger.debug(1, "READ %s", cset)
>>           if not cset:
>>               return -1
>> @@ -155,8 +155,8 @@ class Perforce(FetchMethod):
>>           logger.debug(2, "Fetch: creating temporary directory")
>>           bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
>>           data.setVar('TMPBASE', data.expand('${WORKDIR}/oep4.XXXXXX',
>> localdata), localdata)
>> -        tmppipe = os.popen(data.getVar('MKTEMPDIRCMD', localdata,
>> True) or "false")
>> -        tmpfile = tmppipe.readline().strip()
>> +        tmpfile, errors = bb.process.run(data.getVar('MKTEMPDIRCMD',
>> localdata, True) or "false")
>> +        tmpfile = tmpfile.strip()
>>           if not tmpfile:
>>               raise FetchError("Fetch: unable to create temporary
>> directory.. make sure 'mktemp' is in the PATH.", loc)
>>
>> @@ -169,7 +169,8 @@ class Perforce(FetchMethod):
>>           os.chdir(tmpfile)
>>           logger.info("Fetch " + loc)
>>           logger.info("%s%s files %s", p4cmd, p4opt, depot)
>> -        p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot)) +
>>      p4file, errors = bb.process.run("%s%s files %s" % (p4cmd, p4opt,
>> depot)) +        p4file = p4file.strip()
>>
>>           if not p4file:
>>               raise FetchError("Fetch: unable to get the P4 files from %s" %
>> depot, loc)
>> diff --git a/bitbake/lib/bb/fetch2/svk.py b/bitbake/lib/bb/fetch2/svk.py
>> index 9d34abf..ee3823f 100644
>> --- a/bitbake/lib/bb/fetch2/svk.py
>> +++ b/bitbake/lib/bb/fetch2/svk.py
>> @@ -77,8 +77,8 @@ class Svk(FetchMethod):
>>           logger.debug(2, "Fetch: creating temporary directory")
>>           bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
>>           data.setVar('TMPBASE', data.expand('${WORKDIR}/oesvk.XXXXXX',
>> localdata), localdata)
>> -        tmppipe = os.popen(data.getVar('MKTEMPDIRCMD', localdata,
>> True) or "false")
>> -        tmpfile = tmppipe.readline().strip()
>> +        tmpfile, errors = bb.process.run(data.getVar('MKTEMPDIRCMD',
>> localdata, True) or "false")
>> +        tmpfile = tmpfile.strip()
>>           if not tmpfile:
>>               logger.error()
>>               raise FetchError("Fetch: unable to create temporary
>> directory.. make sure 'mktemp' is in the PATH.", loc) diff --git
>> a/bitbake/lib/bb/ui/crumbs/builddetailspage.py
>> b/bitbake/lib/bb/ui/crumbs/builddetailspage.py index c2d5abc..3ec15d4
>> 100755 --- a/bitbake/lib/bb/ui/crumbs/builddetailspage.py +++
>> b/bitbake/lib/bb/ui/crumbs/builddetailspage.py @@ -23,6 +23,7 @@
>>   import gtk import pango import gobject +import bb.process from
>>   bb.ui.crumbs.progressbar import HobProgressBar from
>>   bb.ui.crumbs.hobwidget import hic, HobNotebook, HobAltButton,
>>   HobWarpCellRendererText from bb.ui.crumbs.runningbuild import
>>   RunningBuildTreeView
>> @@ -96,9 +97,9 @@ class BuildConfigurationTreeView(gtk.TreeView):
>>           for path in src_config_info.layers:
>>               import os, os.path
>>               if os.path.exists(path):
>> -                f = os.popen('cd %s; git branch 2>&1 | grep "^* " | tr -d
>> "* "' % path)
>> +                f, errors = bb.process.run('cd %s; git branch 2>&1 | grep
>> "^* " | tr -d "* "' % path)
>>                   if f:
>> -                    branch = f.readline().lstrip('\n').rstrip('\n')
>> +                    branch = f.strip('\n')
>>                       vars.append(self.set_vars("Branch:", branch))
>>                       f.close()
>>                   break
>> diff --git a/bitbake/lib/bb/ui/crumbs/hig.py b/bitbake/lib/bb/ui/crumbs/hig.py
>> index 721d145..b3936f8 100644
>> --- a/bitbake/lib/bb/ui/crumbs/hig.py
>> +++ b/bitbake/lib/bb/ui/crumbs/hig.py
>> @@ -25,12 +25,12 @@ import gobject
>>   import hashlib import os import re -import subprocess import shlex from
>>   bb.ui.crumbs.hobcolor import HobColors from bb.ui.crumbs.hobwidget
>>   import hcc, hic, HobViewTable, HobInfoButton, HobButton, HobAltButton,
>>   HobIconChecker from bb.ui.crumbs.progressbar import HobProgressBar
>>   import bb.ui.crumbs.utils
>> +import bb.process
>>
>>   """
>>   The following are convenience classes for implementing GNOME HIG
>> compliant
>> @@ -726,7 +726,8 @@ class DeployImageDialog (CrumbsDialog):
>>           self.progress_bar.hide()
>>       def popen_read(self, cmd):
>> -        return os.popen("%s 2>/dev/null" % cmd).read().strip()
>> +        tmpout, errors = bb.process.run("%s" % cmd)
>> +        return tmpout.strip()
>>
>>       def find_all_usb_devices(self):
>>           usb_devs = [ os.readlink(u)
>> @@ -755,7 +756,7 @@ class DeployImageDialog (CrumbsDialog):
>>                   cmdline = bb.ui.crumbs.utils.which_terminal()
>>                   if cmdline:
>>                       cmdline += "\"sudo dd if=" + self.image_path + "
>> of=" + combo_item + "\""
>> -                    subprocess.Popen(args=shlex.split(cmdline))
>> +                    bb.process.run(shlex.split(cmdline))
>>
>>       def update_progress_bar(self, title, fraction, status=None):
>>           self.progress_bar.update(fraction)
>
>
>
>

Re: [PATCH 2/2] replace os.popen with subprocess.Popen

[Wang, Shane]

Sounds good. Thanks.

--
Shane

Robert Yang wrote on 2012-05-30:

> 
> 
> On 05/29/2012 10:58 PM, Wang, Shane wrote:
>> Robert, be careful. Your patch 094742bed2fc01d55f572da946fcfa7a48521401
>> has broke Hob.
>> 
> 
> Sorry, in bitbake/lib/bb/ui/crumbs/builddetailspage.py:
> 
> 103                     branch = f.strip('\n')
> 104                     vars.append(self.set_vars("Branch:", branch))
> 105                     f.close()
> 106                 break
> 
> Remove the f.close should be OK, I will check the patch again to see whether
> there is such an error in other corner, and then send a pull request today.
> 
> // Robert
> 
>> I am going to assign the bug
>> https://bugzilla.yoctoproject.org/show_bug.cgi?id=2511 to you and Kai.
>> 
>> The error is:
>> Traceback (most recent call last):
>>    File
> "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builder.py", line
> 821, in handler_build_started_cb
>>      self.build_details_page.show_configurations(self.configuration,
> self.parameters)
>>    File
> "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage
> .py", line 336, in show_configurations
>>      self.config_tv.show(configurations, params)
>>    File
> "/home/yocto-build5/poky-contrib/bitbake/lib/bb/ui/crumbs/builddetailspage
> .py", line 105, in show
>>      f.close()
>> AttributeError: 'str' object has no attribute 'close'
>> 
>> --
>> Shane
>> 
>> Robert Yang wrote on 2012-05-20:
>> 
>>> Replace os.popen with subprocess.Popen since the older function would
>>> fail (more or less) silently if the executed program cannot be found
>>> 
>>> There is a bb.process.run() which will invoke the Popen to run command,
>>> use it for simplify the code.
>>> 
>>> For the:
>>> p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot))
>>> ...
>>> for file in p4file:
>>>      list = file.split()
>>> in bitbake/lib/bb/fetch2/perforce.py, it should be an error in the past,
>>> since it didn't use readline() to read the pipe, but directly used the
>>> split() for the pipe. Use the bb.process.run would fix the problem since
>>> bb.process.run will return strings.
>>> 
>>> More info:
>>> http://docs.python.org/library/subprocess.html#subprocess-replacements
>>> 
>>> [YOCTO #2075]
>>> 
>>> Signed-off-by: Robert Yang<liezhi.yang@windriver.com>
>>> ---
>>>   bitbake/lib/bb/fetch2/perforce.py            |   11 ++++++-----
>>>   bitbake/lib/bb/fetch2/svk.py                 |    4 ++--
>>>   bitbake/lib/bb/ui/crumbs/builddetailspage.py |    5 +++--
>>>   bitbake/lib/bb/ui/crumbs/hig.py              |    7 ++++---
>>>   4 files changed, 15 insertions(+), 12 deletions(-)
>>> diff --git a/bitbake/lib/bb/fetch2/perforce.py
>>> b/bitbake/lib/bb/fetch2/perforce.py index 6abf15d..df3a3a3 100644 ---
>>> a/bitbake/lib/bb/fetch2/perforce.py +++
>>> b/bitbake/lib/bb/fetch2/perforce.py @@ -91,8 +91,8 @@ class
>>> Perforce(FetchMethod):
>>> 
>>>           p4cmd = data.getVar('FETCHCOMMAND_p4', d, True)
>>>           logger.debug(1, "Running %s%s changes -m 1 %s", p4cmd,
> p4opt,
>>> depot) -        p4file = os.popen("%s%s changes -m 1 %s" % (p4cmd,
>>> p4opt, depot)) -        cset = p4file.readline().strip() +
>>> p4file, errors = bb.process.run("%s%s changes -m 1 %s" % (p4cmd, p4opt,
>>> depot)) +        cset = p4file.strip()
>>>           logger.debug(1, "READ %s", cset)
>>>           if not cset:
>>>               return -1
>>> @@ -155,8 +155,8 @@ class Perforce(FetchMethod):
>>>           logger.debug(2, "Fetch: creating temporary directory")
>>>           bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
>>>           data.setVar('TMPBASE',
> data.expand('${WORKDIR}/oep4.XXXXXX',
>>> localdata), localdata) -        tmppipe =
>>> os.popen(data.getVar('MKTEMPDIRCMD', localdata, True) or "false") -   
>>>     tmpfile = tmppipe.readline().strip() +        tmpfile, errors =
>>> bb.process.run(data.getVar('MKTEMPDIRCMD', localdata, True) or
>>> "false") +        tmpfile = tmpfile.strip()
>>>           if not tmpfile:
>>>               raise FetchError("Fetch: unable to create temporary
>>> directory.. make sure 'mktemp' is in the PATH.", loc)
>>> 
>>> @@ -169,7 +169,8 @@ class Perforce(FetchMethod):
>>>           os.chdir(tmpfile)
>>>           logger.info("Fetch " + loc)
>>>           logger.info("%s%s files %s", p4cmd, p4opt, depot)
>>> -        p4file = os.popen("%s%s files %s" % (p4cmd, p4opt, depot)) +
>>>      p4file, errors = bb.process.run("%s%s files %s" % (p4cmd, p4opt,
>>> depot)) +        p4file = p4file.strip()
>>> 
>>>           if not p4file:
>>>               raise FetchError("Fetch: unable to get the P4 files
> from %s" %
>>> depot, loc)
>>> diff --git a/bitbake/lib/bb/fetch2/svk.py b/bitbake/lib/bb/fetch2/svk.py
>>> index 9d34abf..ee3823f 100644
>>> --- a/bitbake/lib/bb/fetch2/svk.py
>>> +++ b/bitbake/lib/bb/fetch2/svk.py
>>> @@ -77,8 +77,8 @@ class Svk(FetchMethod):
>>>           logger.debug(2, "Fetch: creating temporary directory")
>>>           bb.utils.mkdirhier(data.expand('${WORKDIR}', localdata))
>>>           data.setVar('TMPBASE',
> data.expand('${WORKDIR}/oesvk.XXXXXX',
>>> localdata), localdata) -        tmppipe =
>>> os.popen(data.getVar('MKTEMPDIRCMD', localdata, True) or "false") -   
>>>     tmpfile = tmppipe.readline().strip() +        tmpfile, errors =
>>> bb.process.run(data.getVar('MKTEMPDIRCMD', localdata, True) or
>>> "false") +        tmpfile = tmpfile.strip()
>>>           if not tmpfile:
>>>               logger.error()
>>>               raise FetchError("Fetch: unable to create temporary
>>> directory.. make sure 'mktemp' is in the PATH.", loc) diff --git
>>> a/bitbake/lib/bb/ui/crumbs/builddetailspage.py
>>> b/bitbake/lib/bb/ui/crumbs/builddetailspage.py index c2d5abc..3ec15d4
>>> 100755 --- a/bitbake/lib/bb/ui/crumbs/builddetailspage.py +++
>>> b/bitbake/lib/bb/ui/crumbs/builddetailspage.py @@ -23,6 +23,7 @@
>>>   import gtk import pango import gobject +import bb.process from
>>>   bb.ui.crumbs.progressbar import HobProgressBar from
>>>   bb.ui.crumbs.hobwidget import hic, HobNotebook, HobAltButton,
>>>   HobWarpCellRendererText from bb.ui.crumbs.runningbuild import
>>>   RunningBuildTreeView
>>> @@ -96,9 +97,9 @@ class BuildConfigurationTreeView(gtk.TreeView):
>>>           for path in src_config_info.layers:
>>>               import os, os.path
>>>               if os.path.exists(path):
>>> -                f = os.popen('cd %s; git branch 2>&1 | grep "^* " |
>>> tr -d "* "' % path) +                f, errors = bb.process.run('cd
>>> %s; git branch 2>&1 | grep "^* " | tr -d "* "' % path)
>>>                   if f:
>>> -                    branch = f.readline().lstrip('\n').rstrip('\n')
>>> +                    branch = f.strip('\n')
>>>                       vars.append(self.set_vars("Branch:", branch))
>>>                       f.close()
>>>                   break
>>> diff --git a/bitbake/lib/bb/ui/crumbs/hig.py
>>> b/bitbake/lib/bb/ui/crumbs/hig.py index 721d145..b3936f8 100644 ---
>>> a/bitbake/lib/bb/ui/crumbs/hig.py +++
>>> b/bitbake/lib/bb/ui/crumbs/hig.py @@ -25,12 +25,12 @@ import gobject
>>>   import hashlib import os import re -import subprocess import shlex
>>>   from bb.ui.crumbs.hobcolor import HobColors from
>>>   bb.ui.crumbs.hobwidget import hcc, hic, HobViewTable, HobInfoButton,
>>>   HobButton, HobAltButton, HobIconChecker from
>>>   bb.ui.crumbs.progressbar import HobProgressBar import
>>>   bb.ui.crumbs.utils
>>> +import bb.process
>>> 
>>>   """
>>>   The following are convenience classes for implementing GNOME HIG
>>> compliant
>>> @@ -726,7 +726,8 @@ class DeployImageDialog (CrumbsDialog):
>>>           self.progress_bar.hide()
>>>       def popen_read(self, cmd):
>>> -        return os.popen("%s 2>/dev/null" % cmd).read().strip()
>>> +        tmpout, errors = bb.process.run("%s" % cmd)
>>> +        return tmpout.strip()
>>> 
>>>       def find_all_usb_devices(self):
>>>           usb_devs = [ os.readlink(u)
>>> @@ -755,7 +756,7 @@ class DeployImageDialog (CrumbsDialog):
>>>                   cmdline = bb.ui.crumbs.utils.which_terminal()
>>>                   if cmdline:
>>>                       cmdline += "\"sudo dd if=" + self.image_path +
> "
>>> of=" + combo_item + "\""
>>> -                    subprocess.Popen(args=shlex.split(cmdline))
>>> +                    bb.process.run(shlex.split(cmdline))
>>> 
>>>       def update_progress_bar(self, title, fraction, status=None):
>>>           self.progress_bar.update(fraction)
>> 
>> 
>> 
>>