[pacman-dev] [PATCH] makepkg: Delete logpipe when cleaning up

[Austin Lund]

On Mon, 26 Aug 2019 at 23:56, Eli Schwartz <eschwartz at archlinux.org> wrote:
>
> On 8/26/19 7:29 AM, Austin Lund wrote:
> > There are a number of exit paths where the logpipe fifo remains in the
> > logging directory.  Remove it if it exists when cleaning up at exit.
>
> Sounds like https://patchwork.archlinux.org/patch/1171/ again.
>
> error_function() should always run on the errexit trap for
> run_function_safe, and inside run_function_safe, run_function will run
> to execute the function, log the logpipe, and rm the logpipe if
> everything succeeded. If for any reason a call in run_function fails
> (even the `rm "$logpipe"`) the error_function trap should definitely
> run, and delete the logpipe fifo.
>
> The only way it should be left behind is if e.g. this exits without
> running the final rm, but does so without errexit erroring -- how???:
>
>                 mkfifo "$logpipe"
>                 tee "$BUILDLOG" < "$logpipe" &
>                 local teepid=$!
>
>                 $pkgfunc &>"$logpipe"
>
>                 wait $teepid
>                 rm "$logpipe"
>
>
> Alternatively, if the rm itself errors -- that is what the other patch
> is about, but why would it error?
>
> Alternatively, makepkg is killed in such a way that it isn't allowed to
> try running cleanup traps. If the process is SIGKILLed, then
> error_function will not run, but neither will clean_up.
>
> I would really like to know in what situations this is supposed to fail.
> e.g. Is it only in devtools?

I can trigger it with just basic makepkg followed by Ctrl-C to trigger
SIGINT or something like makepkg & sleep 5; kill -USR1 $! .

>
> > Signed-off-by: Austin Lund <austin.lund at gmail.com>
> > ---
> >  scripts/makepkg.sh.in | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> > index 43484db3..0e0d3461 100644
> > --- a/scripts/makepkg.sh.in
> > +++ b/scripts/makepkg.sh.in
> > @@ -129,6 +129,8 @@ clean_up() {
> >               return 0
> >       fi
> >
> > +     [[ -p $logpipe ]] && rm "$logpipe"
> > +
>
> Why switch from
> if [[ -p $logpipe ]]; then rm "$logpipe"; fi
> to
> [[ -p $logpipe ]] && rm "$logpipe"
>
> Seems a bit silly to add new cases of `return 1` to our cleanup function.

Indeed.  I forgot the special return behaviour of 'if' constructs when
the conditions are false.  I will submit a revised patch.