Closed
Bug 700862
Opened 13 years ago
Closed 13 years ago
include ps front-end to mozprocess
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Unassigned)
References
Details
(Whiteboard: [mozbase])
Attachments
(1 file)
2.93 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
mozprocess should have a front-end to unix `ps` on linux and mac as well as a robust way of finding the actual name of the executable from the process. There should probably be a front-end to mozrunner that is implemented for all OSes as well as for remote devices.
Reporter | ||
Comment 1•13 years ago
|
||
For an example implementation, see http://k0s.org/hg/config/file/tip/python/process.py
Reporter | ||
Comment 2•13 years ago
|
||
There is https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/pid.py in mozprocess currently, but it is much worse than the code pointed to in comment 1
Reporter | ||
Comment 3•13 years ago
|
||
So this isn't actually used anywhere. OTOH, we probably want this functionality and might as well add it now that its fresh off the proverbial griddle
Attachment #573350 -
Flags: review?(jmaher)
Comment 4•13 years ago
|
||
Comment on attachment 573350 [details] [diff] [review] make mozprocess.pid module more robust Review of attachment 573350 [details] [diff] [review]: ----------------------------------------------------------------- just some questions, overall good, please address the 3 issues/questions below before checking in. ::: mozprocess/mozprocess/pid.py @@ +88,5 @@ > + command = shlex.split(command) > + if command[-1] == '<defunct>': > + command = command[:-1] > + if not command or not defunct: > + continue in the talos utils.py fix, you have: if 'STAT' in process and not defunct: if process['STAT'] == 'Z+': continue why is that missing here? @@ +96,5 @@ > + retval.append((int(process['PID']), command)) > + return retval > + > +def get_pids(name): > + """Get all the pids matching name, exclude any pids below minimum_pid.""" I don't understand what the minimum_pid is and where it is used. Could just be an out of date comment. @@ +101,5 @@ > + > + if mozinfo.isWin: > + # use the windows-specific implementation > + import wpk > + return wpk.get_pids(name) is this reliable? will this be identical to what we have in talos?
Attachment #573350 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 5•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #4) > Comment on attachment 573350 [details] [diff] [review] [diff] [details] [review] > make mozprocess.pid module more robust > > Review of attachment 573350 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > just some questions, overall good, please address the 3 issues/questions > below before checking in. > > ::: mozprocess/mozprocess/pid.py > in the talos utils.py fix, you have: > if 'STAT' in process and not defunct: > if process['STAT'] == 'Z+': > continue > > why is that missing here? Good catch. It should be. I'll add it in. > @@ +96,5 @@ > > + """Get all the pids matching name, exclude any pids below minimum_pid.""" > > I don't understand what the minimum_pid is and where it is used. Could just > be an out of date comment. Yeah, the comment is out of date. This was from old code that originally tried a hack that avoided killing low processes. > @@ +101,5 @@ > > + > > + if mozinfo.isWin: > > + # use the windows-specific implementation > > + import wpk > > + return wpk.get_pids(name) > > is this reliable? will this be identical to what we have in talos? Both of them use windows API calls to determine the IDs. Despite the first-glance at the diff, I did not actually change this code (deletions of the same code also appear in the patch), nor did I for Talos. This should probably be assessed but IMHO not for this bug
Reporter | ||
Comment 6•13 years ago
|
||
pushed: https://github.com/mozilla/mozbase/commit/98ee03e8004c4a42baf686806ca59d97cac4d821
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•