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.

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

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

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

Ok, I implemented your proposal. I ran the tests locally, seems fine.

Comment on attachment 8576850 [details] [diff] [review] mozprocess.ProcessHandler should have an option not to print Oups, I forgot to update the doc. :)

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!

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8466fbf47cc

Ok, try is green so everything must be fine. :)