Bug 62179 - webkitpy: add a popen() call to executive
Summary: webkitpy: add a popen() call to executive
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-06 19:44 PDT by Dirk Pranke
Modified: 2011-06-07 16:58 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.66 KB, patch)
2011-06-06 19:44 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2011-06-06 19:46 PDT, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-06-06 19:44:17 PDT
webkitpy: add a popen() call to executive
Comment 1 Dirk Pranke 2011-06-06 19:44:45 PDT
Created attachment 96182 [details]
Patch
Comment 2 Dirk Pranke 2011-06-06 19:46:10 PDT
Created attachment 96183 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-06-07 16:29:16 PDT
Comment on attachment 96183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96183&action=review

I'm OK with this change.  It's more interesting when we see it used.

> Tools/Scripts/webkitpy/common/system/executive.py:107
> +    PIPE = subprocess.PIPE
> +    STDOUT = subprocess.STDOUT

I'm not sure this bit is super useful.  Especially since we don't ahve STDERR and STDIN there.
Comment 4 Eric Seidel (no email) 2011-06-07 16:29:44 PDT
Another quesiton to ask... why do some parts of the code need popen?  Should we be providing more limited APIs with Executive instead of adding popen?
Comment 5 Dirk Pranke 2011-06-07 16:34:29 PDT
(In reply to comment #3)
> (From update of attachment 96183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96183&action=review
> 
> I'm OK with this change.  It's more interesting when we see it used.
> 

Sure, that'll be showing up shortly in a different patch.

> > Tools/Scripts/webkitpy/common/system/executive.py:107
> > +    PIPE = subprocess.PIPE
> > +    STDOUT = subprocess.STDOUT
> 
> I'm not sure this bit is super useful.  Especially since we don't ahve STDERR and STDIN there.

These are useful insofar as they allow me to not have to import subprocess directly. STDERR and STDIN aren't there because my code didn't need them. We could certainly add them for completeness.

Note that I'm not usually a fan of shadowing variables like this, but avoiding the import seemed like a win.

(In reply to comment #4)
> Another quesiton to ask... why do some parts of the code need popen?  Should we be providing more limited APIs with Executive instead of adding popen?

The callers are using popen() to start (and subsequently stop) processes (apache, lighttpd, the python websocket server). We probably don't need the full generality of popen(), but, annoyingly, all three processes need different sets of arguments, so we end up needing at least some of popen.
Comment 6 Dirk Pranke 2011-06-07 16:57:47 PDT
(In reply to comment #5)
> > > Tools/Scripts/webkitpy/common/system/executive.py:107
> > > +    PIPE = subprocess.PIPE
> > > +    STDOUT = subprocess.STDOUT
> > 
> > I'm not sure this bit is super useful.  Especially since we don't ahve STDERR and STDIN there.
> 
> These are useful insofar as they allow me to not have to import subprocess directly. STDERR and STDIN aren't there because my code didn't need them. We could certainly add them for completeness.
> 

Oh, er, also because they don't exist :) PIPE and STDOUT are the only two constants in the subprocess interface.
Comment 7 Dirk Pranke 2011-06-07 16:58:31 PDT
Committed r88287: <http://trac.webkit.org/changeset/88287>