API of CommandExecutor and its behaviour on non-zero exit codes

[Andreas Pakulat]

Hi,

On Sat, Sep 1, 2012 at 7:44 PM, Aleix Pol <aleixpol at kde.org> wrote:
> On Sat, Sep 1, 2012 at 12:02 PM, Andreas Pakulat <apaku at gmx.de> wrote:
>> The problem with that is that this changes the meaning of the API.
>> procError was so far meant to indicate a problem with the process
>> itself, it wasn't meant as a way to indicate that the result of the
>> process is not 'successful'. So things like failure to start, crashes
>> and other unusual process-execution problems are to be tracked. In
>> particular they're often causing popups to be shown to make the user
>> aware of that.
>>
>> IMHO if a process exits normally with a non-zero code, then the
>> command-execution was sucessfully done and hence there's no reason to
>> communicate an error to the caller from the API of CommandExecutor.
>>
>> According to the commit the ninja-job execution didn't notice that
>> building wasn't successful. I don't know that codebase, but this
>> sounds like instead of relying on procError it should handle the
>> exit-code of the process in its completed slot instead. This is
>> currently not exposed, but we can easily add the exit-code to the
>> completed signal.
>>
> Well, I understand that a process that ends with a return code
> different to 0 has to be treated as a failed run.

No, not necessarily. That might be true for cmake and many other
tools, but it may not be the case for all kinds of apps. So I think
its not something that CommandExecutor can decide, since it depends on
the actual tool the code that sets the command should decide this
since it knows which tools it executes.

> I'm not sure how you work with the custom build system there, so I'm unsure why this is a problem.

Well, the plugin shows a message box when there's an error about
failure-to-start or crashing of the build process, since such things
shouldn't be overlooked in tons of other build output. So the plugin
treats the process-errors as more severe than a non-zero return value.
And by changing the behaviour of CommandExecutor to emit procError
also with successful execution but non-zero return value these message
boxes where now shown on any error that was produced during a build
tool execution.

> The commit you're referring to, doesn't happen only with ninja, it's
> the same problem with CMake too. The thing is that the CMake configure
> process is run using the CommandExecutor, so if the cmake process
> errors, it still returns positively and it proceeds to run make/ninja.

The point is that we're talking about the CommandExecutor API, thats a
class that executes a command and tells you wether it was able to do
so or not using two signals. So having it say 'completed' even when
the process returns a non-zero code is just fine IMO, as the process
was executed completely.

> This problem, though, is worsened in ninja because NinjaJob uses the
> CommandExecutor to call ninja (MakeBuilder uses a custom KProcess and
> duplicates all the logic) but NinjaJob can't know if the build was
> successful or not, to print *Finished* or  *Failed* and to emit either
> the emitResult or emitFailed (unsure of the naming, I'm not on my
> laptop).

Sure, no problem, add the return code to the completed signal, then
the caller can decide based on the return-code. Again, my point is
that its not CommandExecutor's job to make this decision since it has
no idea about the the command being executed and hence it cannot even
decide wether there was an error or not based on the return code. As I
said further up, there may be processes which produce different kinds
of success messages via non-zero error codes.

I can add the return-code parameter to completed and adjust all uses
inside kdevelop/kdevplatform along with having the ninja and cmake
code use it.