[pacman-dev] [PATCH] alpm: Add event ALPM_EVENT_HOOK_RUN_FAILED

[Olivier Brunel]

On Mon, 14 Dec 2015 12:47:26 -0500
Andrew Gregory <andrew.gregory.8 at gmail.com> wrote:

> On 12/14/15 at 05:55pm, Olivier Brunel wrote:
> > Also include the abort_on_fail flag so the frontend can let the
> > user know which hook(s) are responsible.
> > 
> > Signed-off-by: Olivier Brunel <jjk at jjacky.com>
> > ---  
> 
> This seems like overkill.  A failed AbortOnFail hook will already
> generate 3 error messages (assuming the hook itself prints one).  We
> could just label AbortOnFail hooks as such when we print the info
> during HOOK_RUN_START.  Alternatively, and this might be preferable
> behavior anyway, we could simply stop running PreTransaction hooks as
> soon as an AbortOnFail hook fails.

But there's no guarantee the hook will actually print anything, so it
is good for frontends to (be able to) do so. And this is the only way to
tell which hook(s) are causing the transaction to be aborted, so I
don't see this as overkill.

Now I assumed there was a reason for remaining hooks to still be ran,
but if the behavior is to be changed to stop processing hooks right
away then it might not be needed indeed, since the faulty hook is always
the last one...

> 
> >  lib/libalpm/alpm.h    | 6 +++++-
> >  lib/libalpm/hook.c    | 7 +++++--
> >  src/pacman/callback.c | 4 ++++
> >  3 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > index f6e6b86..52cbd51 100644
> > --- a/lib/libalpm/alpm.h
> > +++ b/lib/libalpm/alpm.h
> > @@ -465,7 +465,9 @@ typedef enum _alpm_event_type_t {
> >  	/** A hook is starting */
> >  	ALPM_EVENT_HOOK_RUN_START,
> >  	/** A hook has finnished runnning */
> > -	ALPM_EVENT_HOOK_RUN_DONE
> > +	ALPM_EVENT_HOOK_RUN_DONE,
> > +	/** A hook has failed to run. */
> > +	ALPM_EVENT_HOOK_RUN_FAILED
> >  } alpm_event_type_t;
> >  
> >  typedef struct _alpm_event_any_t {
> > @@ -568,6 +570,8 @@ typedef struct _alpm_event_hook_run_t {
> >  	alpm_event_type_t type;
> >  	/** Text to be outputted */
> >  	const char *desc;
> > +	/** Whether the hook has the AbortOnFail flag set
> > (pre-transaction only) */  
> 
> This is not strictly true.  Setting AbortOnFail on a PostTransaction
> hook generates a warning, but leaves the flag set.  In that case your
> callback will falsely claim that it aborted the transaction.

Oh, I missed that. Easy enough to set the flag here only for
pre-transaction hooks though. (Since it doesn't apply, why are
abort_on_fail flags set for post-transaction hooks though?)