Re: [PATCH 2/2] job.c: implementing child_execute_job() using posix_spawn(), and use it if present

[Barath Aron]

On 07/22/2018 09:58 PM, Paul Smith wrote:
> On Mon, 2018-07-09 at 09:05 +0200, Aron Barath wrote:
> > ---
> Thanks for the work you put into this!
You're welcome! :)
> Unfortunately this change reveals some deeper problems that I will need
> to address.  Basically, the old code never really expected fork() to
> fail: if it did we intended to just give up completely.  Thus, it
> expected child_execute_job() to always succeed except in exceptional
> situations, where make itself was not able to continue.
That's good news and bad news.
> Now that we use posix_spawn() it's quite possible that
> child_execute_job() can fail for simple reasons, such as the program we
> attempt to invoke does not exist or similar.
>
> This reveals that the error handling around child_execute_job() is just
> not right and needs to be reworked... this is some hairy code however.
>
> As an example of the kind of problem we get, consider a make recipe
> that is marked to be ignored, and the command does not exist:
>
>    all:
>            -barbbler
>            echo hi
>
> With current make we get correct behavior in that the non-existent
> command failure is ignored, and the "hi" is still echo'd:
>
>    barbbler
>    /tmp/x1.mk:2: recipe for target 'all' failed
>    make: [all] Error 127 (ignored)
>    echo hi
>    hi
>
> while after the posix_spawn change the failure is no longer ignored as
> it should be:
>
>    barbbler
>    make: barbbler: Command not found
>    make: fork: No such file or directory
>
> We get a nicer error message here (except for the extraneous fork
> error), but note how the "echo hi" is no longer run: the failure is not
> ignored as it should be.
Didn't we have a test case for that?
> I'll look into this today but it could take some effort to clean all
> this up.
If there is anything I can help, let me know.
> Cheers!