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

[Chris Larson <clarson@kergoth.com>]

On Tue, May 15, 2012 at 10:55 PM, Robert Yang <liezhi.yang@windriver.com> wrote:
> 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.
>
> 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                 |    2 +-
>  bitbake/lib/bb/ui/crumbs/builddetailspage.py |    3 ++-
>  bitbake/lib/bb/ui/crumbs/hig.py              |    7 ++++---
>  4 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/bitbake/lib/bb/fetch2/perforce.py b/bitbake/lib/bb/fetch2/perforce.py
> index 6abf15d..e07afdd 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, errors) = bb.process.run("%s%s changes -m 1 %s" % (p4cmd, p4opt, depot))
>         cset = p4file.readline().strip()

This is wrong, and will actually fail badly if you attempt to use
this. run() returns the values from Popen.communicate(), which returns
the data as strings, not pipes, so read() and readline() will not be
available on that object, and aren't needed. Also, the parens aren't
needed. "foo, bar =" is just as valid as "(foo, bar) =" (minor). This
should do it:

    output, errors = bb.process.run("%s%s changes -m 1 %s" % (p4cmd,
p4opt, depot))
    cset = output.strip()

> @@ -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(args=shlex.split(cmdline))

This is wrong, and will fail. run() has no 'args' named argument. Try
not to get discouraged -- we do appreciate the work you're doing, but
checking this in would break bitbake for a great number of people. I'd
suggest actually testing bitbake with your changes before sending them
to the list. Thanks.
-- 
Christopher Larson