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

[Aleix Pol]

On Sat, Sep 1, 2012 at 12:02 PM, Andreas Pakulat <apaku at gmx.de> wrote:
> Hi,
>
> I finally was annoyed by constantly popping up message boxes in the
> custom buildsystem plugin when the build failed to look at what
> changed recently to cause this. It turns out the culprit is
> e8710e1e552499fd76dad7d35737993619d458bd, which changed
> CommandExecutor to send our the procError signal also when a normal
> execution exited with a non-zero exit code.
>
> 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.
>
> Thoughts? Opinions?
>
> Andreas
>
> PS: I reverted the kdevplatform commit for now to make the existing
> users of CommandExecutor less annoying again (for example running an
> app which terminates with non-zero always yielded 2 messages boxes
> too).
>
> --
> KDevelop-devel mailing list
> KDevelop-devel at kdevelop.org
> https://barney.cs.uni-potsdam.de/mailman/listinfo/kdevelop-devel

Well, I understand that a process that ends with a return code
different to 0 has to be treated as a failed run. I'm not sure how you
work with the custom build system there, so I'm unsure why this is a
problem.

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.

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

In any case, if it's really annoying, leave the commit reverted and
I'll look into this when I'm back home. If you can report a bug about
that, it will be helpful so that I won't forget.

Aleix