907433 - mozprocess docs could be improved

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

We finally have mozprocess docs in mozbase (https://mozbase.readthedocs.org/en/latest/mozprocess.html) but they could stand for some improvement:

1. We should explain that ProcessHandlerMixin is an advanced class meant to be used in advanced cases where ProcessHandler isn't up to the job. We should also document ProcessHandler first.
2. We should put in some examples of using mozprocess right up on top. https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/test_mozprocess.py
3. As :ahal pointed out, processOutputlines in ProcessHandlerMixin could potentially be a list of functions

mozhttpd docs provide a good example for what we're looking for in a module like this: https://mozbase.readthedocs.org/en/latest/mozhttpd.html

Comment 1

[William Lachance (:wlach)]

Oops, forgot the standard "good first bug" stuff. If you want to work on this, see:

https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Installing_Mozbase_for_Development
https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Working_on_Mozbase_and_Contributing_Patches

Specifically, you are going to want to change the files:

docs/mozprocess.rst (the actual documentation)
mozprocess/mozprocess/*.py (the python annotations which get pulled into the above)

If you run into trouble, contact :ahal, :jhammel, :jgriffin, or :wlach on irc.mozilla.org #ateam.

Comment 2

[dadel]

Hi there,
I would like to work on this for my first contribution to Mozilla :-)

Comment 3

[dadel]

Here is a first not complete version of the docs

Comment 4

[dadel]

Attachment: A First Incomplete Version Of Mozprocess Docs

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8380949 [details] [diff] [review]
A First Incomplete Version Of Mozprocess Docs

Review of attachment 8380949 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for all this work! I have a bunch of comments. I think there's a bit more back & forth that we'll need to do before this lands, so r- for now... but this is a great start.

::: testing/mozbase/docs/mozprocess.rst
@@ +11,5 @@
>    for each line of output produced by the process
>  * the ability to specify handlers that will be called on process timeout
>    and normal process termination
>  
> +Basic use

I'd probably saying "Running a process" as opposed to "Basic use" here.

@@ +14,5 @@
>  
> +Basic use
> +---------
> +
> +There are mostly two main classes for mozprocess users: ProcessHandler and ProcessHandlerMixin for advanced usage cases. The first heritates from the second.

"There are mostly two main classes for mozprocess users:" -> "mozprocess consists of two classes:"
"The first heritates from the second" -> "ProcessHandler inherits from ProcessHandlerMixin"

@@ +16,5 @@
> +---------
> +
> +There are mostly two main classes for mozprocess users: ProcessHandler and ProcessHandlerMixin for advanced usage cases. The first heritates from the second.
> +
> +Lets see how to run a process.

"Lets see" -> "Let's see"

@@ +34,5 @@
> +    p.wait()
> +
> +Note that using *ProcessHandler* instead of *ProcessHandlerMixin* will print the output of executed command. The attribute *commandline* provides the launched command.
> +
> +Basic process

Let's call this "Collecting process output" and incorporate the next section, since they form a logical group.

@@ +70,5 @@
> +        PING -n 2 127.0.0.1 > NUL
> +    )
> +
> +
> +Process output

As discussed above, let's remove this section and incorporate it into above.

@@ +73,5 @@
> +
> +Process output
> +--------------
> +
> +Mozprocess allows to use custom output handlers to trace a process output while running.

"allows to use" -> " allows the specification of"

"trace a process output" -> "gather process output"

@@ +125,5 @@
> +---------
> +
> +Mozprocess provides also facilities concerning waiting, timeout, children managing, killing and ending a process.
> +
> +- Running

I think you can remove this.

@@ +127,5 @@
> +Mozprocess provides also facilities concerning waiting, timeout, children managing, killing and ending a process.
> +
> +- Running
> +
> +  status:

I think we can replace this with 

Status
``````

@@ +129,5 @@
> +- Running
> +
> +  status:
> +
> +It is possible to know the status of the process. The *poll()* method will return None if the process is still running, 0 it ended without failures and a negative value if it was killed by a signal (only Unix like OS).

"It is possible to know the status of the process" -> "It is possible to query the status of the process via the *poll()* method."
"The poll() method" -> "poll()"
"(only Unix like OS)" -> "(Unix-only)"

@@ +135,5 @@
> +.. code-block:: python
> +    import time
> +    from mozprocess import processhandler
> +
> +    #TODO: to enhance!

You probably want to remove this line. :)

@@ +140,5 @@
> +    command = './proc_sleep_echo.sh'
> +    p = processhandler.ProcessHandler(command)
> +    p.run()
> +    time.sleep(2)
> +    print "poll status", p.poll()

print "poll status: %s" % p.poll()

@@ +142,5 @@
> +    p.run()
> +    time.sleep(2)
> +    print "poll status", p.poll()
> +    p.wait()
> +    print "poll status", p.poll()

As above:

print "poll status: %s" % p.poll()

@@ +144,5 @@
> +    print "poll status", p.poll()
> +    p.wait()
> +    print "poll status", p.poll()
> +
> +- Timeout

Timeout
```````

@@ +146,5 @@
> +    print "poll status", p.poll()
> +
> +- Timeout
> +
> +A timeout can be provided in the *run()* method. If the process last more than timeout seconds, it will be stoped.

stoped -> stopped

@@ +164,5 @@
> +    functions = [ontimeout]
> +    p = processhandler.ProcessHandler(command, onTimeout=functions)
> +    p.run(timeout=2)
> +    p.wait()
> +    print "timedOut = %s" % str(p.timedOut)

I don't think the cast to str() is necessary here?

@@ +166,5 @@
> +    p.run(timeout=2)
> +    p.wait()
> +    print "timedOut = %s" % str(p.timedOut)
> +
> +By default the process will be killed on timeout but it is possibleprevent this by setting *kill_on_timeout* to *False*.

possibleprevent -> possible to prevent

@@ +173,5 @@
> +
> +    p = processhandler.ProcessHandler(command, onTimeout=functions, kill_on_timeout=False)
> +    p.run(timeout=2)
> +    p.wait()
> +    print "timedOut = %s" % str(p.timedOut)

Likewise, cast to str() not necessary.

@@ +175,5 @@
> +    p.run(timeout=2)
> +    p.wait()
> +    print "timedOut = %s" % str(p.timedOut)
> +
> +You can notice that the output is no more available after the timeout, but the process is still running.

Could probably be said more clearly as: "In this case, no output will be available after the timeout, but the process will still be running."

@@ +177,5 @@
> +    print "timedOut = %s" % str(p.timedOut)
> +
> +You can notice that the output is no more available after the timeout, but the process is still running.
> +
> +- Waiting

Waiting
````````

@@ +187,5 @@
> +    command = './proc_sleep_echo.sh' # Windows: 'proc_sleep_echo.bat'
> +    p = processhandler.ProcessHandler(command)
> +    p.run()
> +    p.wait(timeout=2)
> +    print "timedOut = %s" % str(p.timedOut)

Cast to str not necessary?

@@ +190,5 @@
> +    p.wait(timeout=2)
> +    print "timedOut = %s" % str(p.timedOut)
> +    p.wait()
> +
> +- Killing

Killing
```````

@@ +192,5 @@
> +    p.wait()
> +
> +- Killing
> +
> +It is possible to request to kill the process with the method *kill*. It is also possible to request to kill all its children by setting to *False* the parameter *ignore_children* of the initialisation.

It is also possible to request to kill all its children by setting to *False* the parameter *ignore_children* of the initialisation. -> If the parameter "ignore_children" is set to False when the process handler class is initialized, all the process's children will be killed as well.

@@ +194,5 @@
> +- Killing
> +
> +It is possible to request to kill the process with the method *kill*. It is also possible to request to kill all its children by setting to *False* the parameter *ignore_children* of the initialisation.
> +
> +On other systems than Windows, it is possible to specify to the kill method a signal (e.g.: *kill(signal.SIGKILL)*).

On other systems than Windows. -> Except on Windows, you can specify the signal with which to kill the process...

@@ +207,5 @@
> +    p.run()
> +    time.sleep(2)
> +    p.kill()
> +
> +- End of execution

End of execution
````````````````

@@ +209,5 @@
> +    p.kill()
> +
> +- End of execution
> +
> +It is possible to provide a function or a list of functions to call at the end of the process using the initilisation parameter *onFinish*.

It is possible to provide -> You can provide
initilisation -> initialization

@@ +224,5 @@
> +    p = processhandler.ProcessHandler(command, onFinish=finish)
> +    p.run()
> +    p.wait()
> +
> +Children

I like the idea of calling this "Child management"

@@ +227,5 @@
> +
> +Children
> +--------
> +
> +Lets consider the following scripts for illustrating further examples:

I would just say "Consider the following scripts:"

@@ +253,5 @@
> +        echo $i
> +        sleep 1
> +    done
> +
> +Don't forget to allow execution (*chmod +x *.sh*).

I don't think this advice is necessary for our target audience.

@@ +255,5 @@
> +    done
> +
> +Don't forget to allow execution (*chmod +x *.sh*).
> +
> +For windows users adapt the first batch script example to match these two scripts.

We should probably do this ourselves for the benefit of our readers. I know it's a bit more work. :)

@@ +259,5 @@
> +For windows users adapt the first batch script example to match these two scripts.
> +
> +It is possible to get a child running status, wait a child termination or kill children.
> +It is possible to manage groups tracking.
> +Note that under Windows, the child process management is done using the IO Completion ports.

the child process management -> child process management

@@ +261,5 @@
> +It is possible to get a child running status, wait a child termination or kill children.
> +It is possible to manage groups tracking.
> +Note that under Windows, the child process management is done using the IO Completion ports.
> +
> +* Ignoring children:

Ignoring children
`````````````````

@@ +302,1 @@
>  

Should probably have a section here called:

API Documentation
-----------------

(or something)

Comment 6

[dadel]

Attachment: Enhance Mozprocess Documentation

Enhance the mozprocess documentation with description and examples

Comment 7

[William Lachance (:wlach)]

Comment on attachment 8386405 [details] [diff] [review]
Enhance Mozprocess Documentation

Review of attachment 8386405 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good. There were a few more things that I noticed should be changed on a second pass. r+ with those changes applied. Could you make the modifications below (or discuss with me if you don't agree) and upload a new patch?

::: testing/mozbase/docs/mozprocess.rst
@@ +14,5 @@
>  
> +Running a process
> +-----------------
> +
> +mozprocess consists of two classes: ProcessHandler inheritates from ProcessHandlerMixin.

inheritates -> inherits.

@@ +46,5 @@
> +.. code-block:: sh
> +
> +    #!/bin/sh
> +
> +    for i in 1 2 3 4 5 

Extra whitespace at end of line.

@@ +68,5 @@
> +        PING -n 2 127.0.0.1 > NUL
> +    )
> +
> +
> +Mozprocess allows the specification of custom output handlers to gather process output while running.

Can you remove the newline here?

@@ +69,5 @@
> +    )
> +
> +
> +Mozprocess allows the specification of custom output handlers to gather process output while running.
> +ProcessHandler will by default write all outputs on stdout. But the user can provide (to ProcessHandler or ProcessHandlerMixin) a function or a list of functions that will be used as callbacks on each output line generated by the process.

"But the user can provide" -> "You can also provide"

@@ +133,5 @@
> +    command = './proc_sleep_echo.sh'
> +    p = processhandler.ProcessHandler(command)
> +    p.run()
> +    time.sleep(2)
> +    print ("poll status: %s" % p.poll())

Remove parans from print statement.

@@ +136,5 @@
> +    time.sleep(2)
> +    print ("poll status: %s" % p.poll())
> +    time.sleep(1)
> +    p.kill(signal.SIGKILL)
> +    print ("poll status: %s" % p.poll())

Remove parans

@@ +141,5 @@
> +
> +Timeout
> +```````
> +
> +A timeout can be provided in the *run()* method. If the process last more than timeout seconds, it will be stopped.

in the run() method -> to the run() method

@@ +143,5 @@
> +```````
> +
> +A timeout can be provided in the *run()* method. If the process last more than timeout seconds, it will be stopped.
> +
> +After execution, the boolean property *timedOut* will be set to True if a timeout was reached.

the boolean property -> the property

@@ +152,5 @@
> +
> +    from mozprocess import processhandler
> +
> +    def ontimeout():
> +        print ("REACHED TIMEOUT")

Remove parans from print statement

@@ +159,5 @@
> +    functions = [ontimeout]
> +    p = processhandler.ProcessHandler(command, onTimeout=functions)
> +    p.run(timeout=2)
> +    p.wait()
> +    print ("timedOut = %s" % p.timedOut)

Remove parans

@@ +168,5 @@
> +
> +    p = processhandler.ProcessHandler(command, onTimeout=functions, kill_on_timeout=False)
> +    p.run(timeout=2)
> +    p.wait()
> +    print ("timedOut = %s" % p.timedOut)

Remove parans

@@ +175,5 @@
> +
> +Waiting
> +```````
> +
> +It is possible to wait until the the process end of execution as already seen with the method *wait()*, or until the end of a timeout if given. Note that in last case the process is still alive after the timeout.

until the the process end of execution -> until the process exits

@@ +183,5 @@
> +    command = './proc_sleep_echo.sh' # Windows: 'proc_sleep_echo.bat'
> +    p = processhandler.ProcessHandler(command)
> +    p.run()
> +    p.wait(timeout=2)
> +    print ("timedOut = %s" % p.timedOut)

Remove parans

@@ +189,5 @@
> +
> +Killing
> +```````
> +
> +It is possible to request to kill the process with the method *kill*. f the parameter "ignore_children" is set to False when the process handler class is initialized, all the process's children will be killed as well.

It is possible to request to -> "You can"

@@ +226,5 @@
> +Child management
> +----------------
> +
> +Consider the following scripts:
> +

Remove one of these blank lines.

@@ +279,5 @@
> +    )
> +
> +
> +It is possible to get a child running status, wait a child termination or kill children.
> +It is possible to manage groups tracking.

I would move this paragraph up to the beginning of this section, and change it as follows:

"It is possible to get a child running status, wait a child termination or kill children. It is possible to manage groups tracking." -> "For processes that launch other processes, mozprocess allows you to get child running status, wait for child termination, and kill children."

@@ +280,5 @@
> +
> +
> +It is possible to get a child running status, wait a child termination or kill children.
> +It is possible to manage groups tracking.
> +Note that under Windows, the child process management is done using the IO Completion ports.

I think we can just remove this note, as it's basically just an implementation detail that doesn't really help explain things that well.

Comment 8

[dadel]

Attachment: Improve mozprocess Documentation

As discussed on IRC, I left the print parentheses as is for Python3 compatibility.

Comment 9

[William Lachance (:wlach)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/232ae3020845

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/232ae3020845