Review Request: (code deduplication) Add class OutputExecuteJob as a common base for all output jobs which start processes

[Ivan Shapovalov]

> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 141
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line141>
> >
> >     This class is probably going to be used for builders.
> >     
> >     Maybe we can change working directory to build directory here (and change the class name too, of course).

This also will be used for VCS jobs... And probably everything else, so "build directory" is too specific here.
Though I agree that current wording is a bit vague; any ideas? Maybe a property called "IsBuilder" which will change the wording to builder-specific?


> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 203
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line203>
> >
> >     If Kill is called, we probably want to kill it right away instead of waiting for 3s...

I don't like this either. But:
In Unices, QProcess::terminate() is better in terms of resource freeing. If a program needs to do some _really_ important cleanups, it cannot do it with SIGKILL. So attempt to do SIGTERM before.
OTOH, in Windows QProcess::terminate() is (according to docs) GUI-only, and QProcess::kill() is a usual "process termination" which replaces both SIGTERM and SIGKILL.


> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 206
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line206>
> >
> >     Maybe we want a timeout here?

Maybe...


> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 209
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line209>
> >
> >     Put it in the else?
> >     
> >     Otherwise we'll get both an Abort and a Warning

Ok.


> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 228
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line228>
> >
> >     had failed sounds very wrong.
> >     Maybe "Process has failed to start"
> >     
> >     Also it might be interesting to put more information, like having:
> >     "Make has failed to start".
> >

Ok, will do "$path_to_executable has failed to start".


> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 381
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line381>
> >
> >     use KShell::joinArgs for this.
> >     
> >     http://api.kde.org/4.9-api/kdelibs-apidocs/kdecore/html/namespaceKShell.html#ad92ed56d00a678554dd04b0b1429760e

Ok.


> On Sept. 17, 2012, 11:37 a.m., Aleix Pol Gonzalez wrote:
> > outputview/outputexecutejob.cpp, line 402
> > <http://git.reviewboard.kde.org/r/106463/diff/1/?file=85697#file85697line402>
> >
> >     This should be called when the arguments change as well, then.

Ok - but only if "AppendProcessString" is set.


- Ivan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106463/#review19055
-----------------------------------------------------------


On Sept. 17, 2012, 9:21 a.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106463/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2012, 9:21 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> This class is going to be used instead of OutputJob as a common base for all such jobs which start processes (esp., builder jobs).
> 
> There are separate patches for make builder, cmake job and ninja builder.
> 
> 
> Diffs
> -----
> 
>   outputview/CMakeLists.txt 3dced4c 
>   outputview/outputexecutejob.h PRE-CREATION 
>   outputview/outputexecutejob.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106463/diff/
> 
> 
> Testing
> -------
> 
> Existing unit-tests and manual testing (couple of days).
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120917/41fbdef3/attachment.html>