[mozprocess] mozprocess.ProcessHandler should have an option not to print

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: k0scist, Assigned: parkouss)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
From
https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L829 :


"""
class ProcessHandler(ProcessHandlerMixin):
...

    def __init__(self, cmd, logfile=None, storeOutput=True, **kwargs):
        kwargs.setdefault('processOutputLine', [])

        # Print to standard output only if no outputline provided
        if not kwargs['processOutputLine']:
            kwargs['processOutputLine'].append(print_output)
...
"""

Output is printed unless an outputhandler is passed...though there is
no way to turn off printing other than passing, say, a null output
handler, e.g.:

def nullhandler(line):
    return # or `lambda x: None`

Instead, we should just make this behaviour explicit and controllable
with an argument.
(Assignee)

Comment 1

4 years ago
Hey,

I propose that we make it controlable by passing False to processOutputLine or processStderrLine in the ProcessHandlerMixin class.

Current default for these arguments are None, and a function or a list of function is expected, so this won't break the api.

What do you think ?
I think ProcessHandlerMixin already does the right thing. If no handlers are passed into it, then no output will be printed, no?

I believe Jeff was talking about the ProcessHandler class. By default it adds a handler that prints to stdout if none was specified. Though honestly, I'm not sure I agree with a flag to turn this behaviour on or off. I'd rather implement check_output/check_call convenience functions like :gps mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=819547#c1

The problem with this approach is that we either need to break backwards compatibility or else have a franken-api, half mozprocess, half subprocess (though, tbh, we already do).
(Assignee)

Comment 3

4 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> I think ProcessHandlerMixin already does the right thing. If no handlers are
> passed into it, then no output will be printed, no?

No, by default ProcessHandlerMixin redirect output: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#678

> I believe Jeff was talking about the ProcessHandler class. By default it
> adds a handler that prints to stdout if none was specified. Though honestly,
> I'm not sure I agree with a flag to turn this behaviour on or off. I'd
> rather implement check_output/check_call convenience functions like :gps
> mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=819547#c1

Ok :)

> The problem with this approach is that we either need to break backwards
> compatibility or else have a franken-api, half mozprocess, half subprocess
> (though, tbh, we already do).

No, that won't break the API. it is just adding the possibility to pass False (or None, I just see that it would work too) instead of a callback or a list of callback into the arguments processOutputLine or processStderrLine (currently if you give False or None, there is an exception raised) because of https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#625.

Or maybe we already broke the compatibility because patch in bug 794984 introduced this change.
(In reply to Julien Pagès from comment #3)
> (In reply to Andrew Halberstadt [:ahal] from comment #2)
> > I think ProcessHandlerMixin already does the right thing. If no handlers are
> > passed into it, then no output will be printed, no?
> 
> No, by default ProcessHandlerMixin redirect output:
> https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/
> mozprocess/processhandler.py#678

Right, but processOutputLine and processStderrLine are both () by default. In that case the reader won't do anything with the output (ie nothing will be printed):
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#811

> > The problem with this approach is that we either need to break backwards
> > compatibility or else have a franken-api, half mozprocess, half subprocess
> > (though, tbh, we already do).
> 
> No, that won't break the API. it is just adding the possibility to pass
> False 

Sorry, by "this" approach I meant the one I was suggesting.
Actually I think one easy way to fix this would be to make 'stream' default to sys.stdout and remove this if statement:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/processhandler.py#975

Then if no output were desired, consumers would just need to pass in stream=None.
(Assignee)

Comment 6

4 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #4)

> Right, but processOutputLine and processStderrLine are both () by default.
> In that case the reader won't do anything with the output (ie nothing will
> be printed):
> https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/
> mozprocess/processhandler.py#811

Oh, I see that now! You're right ; sorry for the noise. :)

Ok I see the point of your comment 5. It may just break is there are mozrunner consummers that already pass None for the stream value, expecting the current behaviour (ie print something).

I think it is the best choice if we can accept that, and I can work on this if you like.
(Assignee)

Comment 7

4 years ago
Created attachment 8576850 [details] [diff] [review]
mozprocess.ProcessHandler should have an option not to print

Ok, I implemented your proposal. I ran the tests locally, seems fine.
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8576850 - Flags: review?(ahalberstadt)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8576850 [details] [diff] [review]
mozprocess.ProcessHandler should have an option not to print

Oups, I forgot to update the doc. :)
Attachment #8576850 - Flags: review?(ahalberstadt)
(Assignee)

Comment 9

4 years ago
Created attachment 8576877 [details] [diff] [review]
mozprocess.ProcessHandler should have an option not to print
Attachment #8576850 - Attachment is obsolete: true
Attachment #8576877 - Flags: review?(ahalberstadt)
Comment on attachment 8576877 [details] [diff] [review]
mozprocess.ProcessHandler should have an option not to print

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

Thanks, lgtm!
Attachment #8576877 - Flags: review?(ahalberstadt) → review+
Depends on: 1142229
No longer depends on: 1142229
(Assignee)

Comment 12

4 years ago
Ok, try is green so everything must be fine. :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8255f2dde778
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.