include ps front-end to mozprocess

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jeff Hammel, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozbase])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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)

Updated

6 years ago
See Also: → bug 700722
Whiteboard: [mozbase]
(Reporter)

Comment 1

6 years ago
For an example implementation, see http://k0s.org/hg/config/file/tip/python/process.py
(Reporter)

Comment 2

6 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

6 years ago
Created attachment 573350 [details] [diff] [review]
make mozprocess.pid module more robust

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 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

6 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

6 years ago
pushed: https://github.com/mozilla/mozbase/commit/98ee03e8004c4a42baf686806ca59d97cac4d821
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.