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

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

[Robert Yang]

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 12b4543ac9f54c0db0ee4a7ee546a71946a051f8:

  classes/rootfs_*: fix splitting package dependency strings (2012-05-13 20:09:56 +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            |    9 +++++----
 bitbake/lib/bb/fetch2/svk.py                 |    3 ++-
 bitbake/lib/bb/shell.py                      |    6 +++---
 bitbake/lib/bb/ui/crumbs/builddetailspage.py |    3 ++-
 bitbake/lib/bb/ui/crumbs/hig.py              |    2 +-
 bitbake/lib/bb/ui/crumbs/imagedetailspage.py |    3 ++-
 bitbake/lib/bb/ui/ncurses.py                 |    4 ++--
 7 files changed, 17 insertions(+), 13 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

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            |    6 +++---
 bitbake/lib/bb/fetch2/svk.py                 |    3 ++-
 bitbake/lib/bb/ui/crumbs/builddetailspage.py |    3 ++-
 bitbake/lib/bb/ui/crumbs/hig.py              |    2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/bitbake/lib/bb/fetch2/perforce.py b/bitbake/lib/bb/fetch2/perforce.py
index 6abf15d..ad7df5f 100644
--- a/bitbake/lib/bb/fetch2/perforce.py
+++ b/bitbake/lib/bb/fetch2/perforce.py
@@ -91,7 +91,7 @@ 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))
+        p4file = subprocess.Popen("%s%s changes -m 1 %s" % (p4cmd, p4opt, depot), shell=True, stdout=subprocess.PIPE).stdout
         cset = p4file.readline().strip()
         logger.debug(1, "READ %s", cset)
         if not cset:
@@ -155,7 +155,7 @@ 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")
+        tmppipe = subprocess.Popen(data.getVar('MKTEMPDIRCMD', localdata, True) or "false", shell=True, stdout=subprocess.PIPE).stdout
         tmpfile = tmppipe.readline().strip()
         if not tmpfile:
             raise FetchError("Fetch: unable to create temporary directory.. make sure 'mktemp' is in the PATH.", loc)
@@ -169,7 +169,7 @@ 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 = subprocess.Popen("%s%s files %s" % (p4cmd, p4opt, depot), shell=True, stdout=subprocess.PIPE).stdout
 
         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..cfca713 100644
--- a/bitbake/lib/bb/fetch2/svk.py
+++ b/bitbake/lib/bb/fetch2/svk.py
@@ -26,6 +26,7 @@ This implementation is for svk. It is based on the svn implementation
 # Based on functions from the base bb module, Copyright 2003 Holger Schurig
 
 import os
+import subprocess
 import logging
 import bb
 from   bb import data
@@ -77,7 +78,7 @@ 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")
+        tmppipe = subprocess.Popen(data.getVar('MKTEMPDIRCMD', localdata, True) or "false", shell=True, stdout=subprocess.PIPE).stdout
         tmpfile = tmppipe.readline().strip()
         if not tmpfile:
             logger.error()
diff --git a/bitbake/lib/bb/ui/crumbs/builddetailspage.py b/bitbake/lib/bb/ui/crumbs/builddetailspage.py
index 51e6a4a..30f8249 100755
--- a/bitbake/lib/bb/ui/crumbs/builddetailspage.py
+++ b/bitbake/lib/bb/ui/crumbs/builddetailspage.py
@@ -28,6 +28,7 @@ from bb.ui.crumbs.hobwidget import hic, HobNotebook, HobAltButton, HobWarpCellRe
 from bb.ui.crumbs.runningbuild import RunningBuildTreeView
 from bb.ui.crumbs.runningbuild import BuildFailureTreeView
 from bb.ui.crumbs.hobpages import HobPage
+import subprocess
 
 class BuildConfigurationTreeView(gtk.TreeView):
     def __init__ (self):
@@ -96,7 +97,7 @@ 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 = subprocess.Popen('cd %s; git branch 2>&1 | grep "^* " | tr -d "* "' % path, shell=True, stdout=subprocess.PIPE).stdout
                 if f:
                     branch = f.readline().lstrip('\n').rstrip('\n')
                     vars.append(self.set_vars("Branch:", branch))
diff --git a/bitbake/lib/bb/ui/crumbs/hig.py b/bitbake/lib/bb/ui/crumbs/hig.py
index 4baf960..e7650f2 100644
--- a/bitbake/lib/bb/ui/crumbs/hig.py
+++ b/bitbake/lib/bb/ui/crumbs/hig.py
@@ -726,7 +726,7 @@ class DeployImageDialog (CrumbsDialog):
         self.progress_bar.hide()
 
     def popen_read(self, cmd):
-        return os.popen("%s 2>/dev/null" % cmd).read().strip()
+        return subprocess.Popen("%s 2>/dev/null" % cmd, shell=True, stdout=subprocess.PIPE).stdout.read().strip()
 
     def find_all_usb_devices(self):
         usb_devs = [ os.readlink(u)
-- 
1.7.1

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

[Chris Larson]

On Mon, May 14, 2012 at 1:07 AM, Robert Yang <liezhi.yang@windriver.com> wrote:
>     def popen_read(self, cmd):
> -        return os.popen("%s 2>/dev/null" % cmd).read().strip()
> +        return subprocess.Popen("%s 2>/dev/null" % cmd, shell=True, stdout=subprocess.PIPE).stdout.read().strip()

The 2>/dev/null is irrelevant when we're only inspecting the stdout
anyway. Also, we already have wrappers around Popen for convenience in
the bb.process python module.
-- 
Christopher Larson

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

[Robert Yang]

On 05/14/2012 09:59 PM, Chris Larson wrote:
> On Mon, May 14, 2012 at 1:07 AM, Robert Yang<liezhi.yang@windriver.com>  wrote:
>>      def popen_read(self, cmd):
>> -        return os.popen("%s 2>/dev/null" % cmd).read().strip()
>> +        return subprocess.Popen("%s 2>/dev/null" % cmd, shell=True, stdout=subprocess.PIPE).stdout.read().strip()
>
> The 2>/dev/null is irrelevant when we're only inspecting the stdout
> anyway. Also, we already have wrappers around Popen for convenience in
> the bb.process python module.

Thanks, I will remove the 2>/dev/null and use:

from bb.process import Popen

to simplify the code, I will send the V2 sooner.

// Robert