[pacman-dev] [PATCH 1/3] Collect all triggered hooks before running them
Having a first pass that checks which hooks are triggered followed by a second pass of the triggered hooks allows up to only provide output when a hook is actually triggered. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/hook.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index f5627af..ca90c4d 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -610,9 +610,9 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook) int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) { - alpm_list_t *i, *hooks = NULL; + alpm_list_t *i, *hooks = NULL, *hooks_triggered = NULL; const char *suffix = ".hook"; - size_t suflen = strlen(suffix); + size_t suflen = strlen(suffix), triggered = 0; int ret = 0; for(i = alpm_list_last(handle->hookdirs); i; i = alpm_list_previous(i)) { @@ -712,13 +712,25 @@ int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) for(i = hooks; i; i = i->next) { struct _alpm_hook_t *hook = i->data; if(hook && hook->when == when && _alpm_hook_triggered(handle, hook)) { - _alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name); - if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) { - ret = -1; - } + hooks_triggered = alpm_list_add(hooks_triggered, hook); + triggered++; + } + } + + if(triggered == 0) { + goto cleanup; + } + + for(i = hooks_triggered; i; i = i->next) { + struct _alpm_hook_t *hook = i->data; + _alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name); + if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) { + ret = -1; } } + alpm_list_free(hooks_triggered); + cleanup: alpm_list_free_inner(hooks, (alpm_list_fn_free) _alpm_hook_free); alpm_list_free(hooks); -- 2.6.4
From: Olivier Brunel <jjk@jjacky.com> Add events to let frontends know when hooks are being processed (and when it's done), as that might be useful to update the UI. Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/alpm.h | 24 +++++++++++++++++++++++- lib/libalpm/hook.c | 11 +++++++++-- lib/libalpm/hook.h | 7 +------ src/pacman/callback.c | 6 ++++++ 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 6cbcd24..f8e1f25 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -337,6 +337,16 @@ typedef struct _alpm_siglist_t { alpm_sigresult_t *results; } alpm_siglist_t; + +/* + * Hooks + */ + +typedef enum _alpm_hook_when_t { + ALPM_HOOK_PRE_TRANSACTION = 1, + ALPM_HOOK_POST_TRANSACTION +} alpm_hook_when_t; + /* * Logging facilities */ @@ -443,7 +453,11 @@ typedef enum _alpm_event_type_t { ALPM_EVENT_PACNEW_CREATED, /** A .pacsave file was created; See alpm_event_pacsave_created_t for * arguments */ - ALPM_EVENT_PACSAVE_CREATED + ALPM_EVENT_PACSAVE_CREATED, + /** Processing hooks will be started. */ + ALPM_EVENT_HOOK_START, + /** Processing hooks is finished. */ + ALPM_EVENT_HOOK_DONE } alpm_event_type_t; typedef struct _alpm_event_any_t { @@ -534,6 +548,13 @@ typedef struct _alpm_event_pacsave_created_t { const char *file; } alpm_event_pacsave_created_t; +typedef struct _alpm_event_hook_t { + /** Type of event.*/ + alpm_event_type_t type; + /** Type of hooks. */ + alpm_hook_when_t when; +} alpm_event_hook_t; + /** Events. * This is an union passed to the callback, that allows the frontend to know * which type of event was triggered (via type). It is then possible to @@ -550,6 +571,7 @@ typedef union _alpm_event_t { alpm_event_pkgdownload_t pkgdownload; alpm_event_pacnew_created_t pacnew_created; alpm_event_pacsave_created_t pacsave_created; + alpm_event_hook_t hook; } alpm_event_t; /** Event callback. */ diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index ca90c4d..df41fce 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -52,7 +52,7 @@ struct _alpm_hook_t { alpm_list_t *depends; char **cmd; alpm_list_t *matches; - enum _alpm_hook_when_t when; + alpm_hook_when_t when; int abort_on_fail, needs_targets; }; @@ -608,8 +608,9 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook) } } -int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) +int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) { + alpm_event_hook_t event = { .when = when }; alpm_list_t *i, *hooks = NULL, *hooks_triggered = NULL; const char *suffix = ".hook"; size_t suflen = strlen(suffix), triggered = 0; @@ -721,6 +722,9 @@ int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) goto cleanup; } + event.type = ALPM_EVENT_HOOK_START; + EVENT(handle, &event); + for(i = hooks_triggered; i; i = i->next) { struct _alpm_hook_t *hook = i->data; _alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name); @@ -729,6 +733,9 @@ int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) } } + event.type = ALPM_EVENT_HOOK_DONE; + EVENT(handle, &event); + alpm_list_free(hooks_triggered); cleanup: diff --git a/lib/libalpm/hook.h b/lib/libalpm/hook.h index 4894a19..3f05b31 100644 --- a/lib/libalpm/hook.h +++ b/lib/libalpm/hook.h @@ -22,12 +22,7 @@ #include "alpm.h" -enum _alpm_hook_when_t { - ALPM_HOOK_PRE_TRANSACTION = 1, - ALPM_HOOK_POST_TRANSACTION -}; - -int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when); +int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when); #endif /* _ALPM_HOOK_H */ diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 7a21b22..37966ab 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -167,6 +167,11 @@ void cb_event(alpm_event_t *event) return; } switch(event->type) { + case ALPM_EVENT_HOOK_START: + colon_printf(_("Running %s hooks...\n"), + event->hook.when == ALPM_HOOK_PRE_TRANSACTION + ? _("pre-transaction") : _("post-transaction")); + break; case ALPM_EVENT_CHECKDEPS_START: printf(_("checking dependencies...\n")); break; @@ -329,6 +334,7 @@ void cb_event(alpm_event_t *event) case ALPM_EVENT_DISKSPACE_DONE: case ALPM_EVENT_RETRIEVE_DONE: case ALPM_EVENT_RETRIEVE_FAILED: + case ALPM_EVENT_HOOK_DONE: /* we can safely ignore those as well */ case ALPM_EVENT_PKGDOWNLOAD_START: case ALPM_EVENT_PKGDOWNLOAD_DONE: -- 2.6.4
On 12/14/15 at 01:45pm, Allan McRae wrote:
From: Olivier Brunel <jjk@jjacky.com>
Add events to let frontends know when hooks are being processed (and when it's done), as that might be useful to update the UI.
Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/alpm.h | 24 +++++++++++++++++++++++- lib/libalpm/hook.c | 11 +++++++++-- lib/libalpm/hook.h | 7 +------ src/pacman/callback.c | 6 ++++++ 4 files changed, 39 insertions(+), 9 deletions(-) ... diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 7a21b22..37966ab 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -167,6 +167,11 @@ void cb_event(alpm_event_t *event) return; } switch(event->type) { + case ALPM_EVENT_HOOK_START: + colon_printf(_("Running %s hooks...\n"), + event->hook.when == ALPM_HOOK_PRE_TRANSACTION + ? _("pre-transaction") : _("post-transaction"));
Is there any reason to break up this string like this? Splitting it and using substitution adds an extra string to be translated and I would think translators would benefit from the extra context of the full message.
+ break; case ALPM_EVENT_CHECKDEPS_START: printf(_("checking dependencies...\n")); break; @@ -329,6 +334,7 @@ void cb_event(alpm_event_t *event) case ALPM_EVENT_DISKSPACE_DONE: case ALPM_EVENT_RETRIEVE_DONE: case ALPM_EVENT_RETRIEVE_FAILED: + case ALPM_EVENT_HOOK_DONE: /* we can safely ignore those as well */ case ALPM_EVENT_PKGDOWNLOAD_START: case ALPM_EVENT_PKGDOWNLOAD_DONE: -- 2.6.4
On Mon, 14 Dec 2015 10:59:48 -0500 Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
On 12/14/15 at 01:45pm, Allan McRae wrote:
From: Olivier Brunel <jjk@jjacky.com>
Add events to let frontends know when hooks are being processed (and when it's done), as that might be useful to update the UI.
Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/alpm.h | 24 +++++++++++++++++++++++- lib/libalpm/hook.c | 11 +++++++++-- lib/libalpm/hook.h | 7 +------ src/pacman/callback.c | 6 ++++++ 4 files changed, 39 insertions(+), 9 deletions(-) ... diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 7a21b22..37966ab 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -167,6 +167,11 @@ void cb_event(alpm_event_t *event) return; } switch(event->type) { + case ALPM_EVENT_HOOK_START: + colon_printf(_("Running %s hooks...\n"), + event->hook.when == ALPM_HOOK_PRE_TRANSACTION + ? _("pre-transaction") : _("post-transaction"));
Is there any reason to break up this string like this? Splitting it and using substitution adds an extra string to be translated and I would think translators would benefit from the extra context of the full message.
Nope, you're right two full messages would be better. Allan: do you want me to send an updated version of this?
On 15/12/15 02:58, Olivier Brunel wrote:
On Mon, 14 Dec 2015 10:59:48 -0500 Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
On 12/14/15 at 01:45pm, Allan McRae wrote:
From: Olivier Brunel <jjk@jjacky.com>
Add events to let frontends know when hooks are being processed (and when it's done), as that might be useful to update the UI.
Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/alpm.h | 24 +++++++++++++++++++++++- lib/libalpm/hook.c | 11 +++++++++-- lib/libalpm/hook.h | 7 +------ src/pacman/callback.c | 6 ++++++ 4 files changed, 39 insertions(+), 9 deletions(-) ... diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 7a21b22..37966ab 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -167,6 +167,11 @@ void cb_event(alpm_event_t *event) return; } switch(event->type) { + case ALPM_EVENT_HOOK_START: + colon_printf(_("Running %s hooks...\n"), + event->hook.when == ALPM_HOOK_PRE_TRANSACTION + ? _("pre-transaction") : _("post-transaction"));
Is there any reason to break up this string like this? Splitting it and using substitution adds an extra string to be translated and I would think translators would benefit from the extra context of the full message.
Nope, you're right two full messages would be better.
Allan: do you want me to send an updated version of this?
I'll handle this while rebasing. Allan
This provides a way to detect when the processing of package changes starts, allowing pacman to deliminate hook output and package installation/removal output. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/alpm.h | 4 ++++ lib/libalpm/trans.c | 5 +++++ src/pacman/callback.c | 4 ++++ 3 files changed, 13 insertions(+) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index f8e1f25..8fcdfdf 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -384,6 +384,10 @@ typedef enum _alpm_event_type_t { ALPM_EVENT_INTERCONFLICTS_START, /** Inter-conflicts were checked for target package. */ ALPM_EVENT_INTERCONFLICTS_DONE, + /** Processing the package transaction is starting. */ + ALPM_EVENT_TRANSACTION_START, + /** Processing the package transaction is finished. */ + ALPM_EVENT_TRANSACTION_DONE, /** Package will be installed/upgraded/downgraded/re-installed/removed; See * alpm_event_package_operation_t for arguments. */ ALPM_EVENT_PACKAGE_OPERATION_START, diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 06997a0..0ead10d 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -160,6 +160,7 @@ int SYMEXPORT alpm_trans_prepare(alpm_handle_t *handle, alpm_list_t **data) int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) { alpm_trans_t *trans; + alpm_event_any_t event; /* Sanity checks */ CHECK_HANDLE(handle, return -1); @@ -197,6 +198,8 @@ int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) trans->state = STATE_COMMITING; alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction started\n"); + event.type = ALPM_EVENT_TRANSACTION_START; + EVENT(handle, &event); if(trans->add == NULL) { if(_alpm_remove_packages(handle, 1) == -1) { @@ -222,6 +225,8 @@ int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction completed\n"); _alpm_hook_run(handle, ALPM_HOOK_POST_TRANSACTION); } + event.type = ALPM_EVENT_TRANSACTION_DONE; + EVENT(handle, &event); trans->state = STATE_COMMITED; diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 37966ab..c33bd24 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -186,6 +186,9 @@ void cb_event(alpm_event_t *event) case ALPM_EVENT_INTERCONFLICTS_START: printf(_("looking for conflicting packages...\n")); break; + case ALPM_EVENT_TRANSACTION_START: + colon_printf(_("Processing package changes...\n")); + break; case ALPM_EVENT_PACKAGE_OPERATION_START: if(config->noprogressbar) { alpm_event_package_operation_t *e = &event->package_operation; @@ -325,6 +328,7 @@ void cb_event(alpm_event_t *event) case ALPM_EVENT_CHECKDEPS_DONE: case ALPM_EVENT_RESOLVEDEPS_DONE: case ALPM_EVENT_INTERCONFLICTS_DONE: + case ALPM_EVENT_TRANSACTION_DONE: case ALPM_EVENT_INTEGRITY_DONE: case ALPM_EVENT_KEYRING_DONE: case ALPM_EVENT_KEY_DOWNLOAD_DONE: -- 2.6.4
On 12/14/15 at 01:45pm, Allan McRae wrote:
This provides a way to detect when the processing of package changes starts, allowing pacman to deliminate hook output and package installation/removal
s/deliminate/delineate/?
output.
Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/alpm.h | 4 ++++ lib/libalpm/trans.c | 5 +++++ src/pacman/callback.c | 4 ++++ 3 files changed, 13 insertions(+) ... diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 06997a0..0ead10d 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -160,6 +160,7 @@ int SYMEXPORT alpm_trans_prepare(alpm_handle_t *handle, alpm_list_t **data) int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) { alpm_trans_t *trans; + alpm_event_any_t event;
/* Sanity checks */ CHECK_HANDLE(handle, return -1); @@ -197,6 +198,8 @@ int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) trans->state = STATE_COMMITING;
alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction started\n"); + event.type = ALPM_EVENT_TRANSACTION_START; + EVENT(handle, &event);
if(trans->add == NULL) { if(_alpm_remove_packages(handle, 1) == -1) { @@ -222,6 +225,8 @@ int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction completed\n"); _alpm_hook_run(handle, ALPM_HOOK_POST_TRANSACTION); } + event.type = ALPM_EVENT_TRANSACTION_DONE; + EVENT(handle, &event);
You have the pre-transaction hooks outside the transaction events but the post hooks inside them.
On 12/14/15 at 01:45pm, Allan McRae wrote:
Having a first pass that checks which hooks are triggered followed by a second pass of the triggered hooks allows up to only provide output when a hook is actually triggered.
Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/hook.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index f5627af..ca90c4d 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -610,9 +610,9 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook)
int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) { - alpm_list_t *i, *hooks = NULL; + alpm_list_t *i, *hooks = NULL, *hooks_triggered = NULL; const char *suffix = ".hook"; - size_t suflen = strlen(suffix); + size_t suflen = strlen(suffix), triggered = 0; int ret = 0;
for(i = alpm_list_last(handle->hookdirs); i; i = alpm_list_previous(i)) { @@ -712,13 +712,25 @@ int _alpm_hook_run(alpm_handle_t *handle, enum _alpm_hook_when_t when) for(i = hooks; i; i = i->next) { struct _alpm_hook_t *hook = i->data; if(hook && hook->when == when && _alpm_hook_triggered(handle, hook)) { - _alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name); - if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) { - ret = -1; - } + hooks_triggered = alpm_list_add(hooks_triggered, hook); + triggered++; + } + } + + if(triggered == 0) {
triggered is redundant in this patch. We can just check 'hooks_triggered == NULL' here which is safer since triggered could, at least theoretically, overflow and wrap back to 0.
+ goto cleanup; + } + + for(i = hooks_triggered; i; i = i->next) { + struct _alpm_hook_t *hook = i->data; + _alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name); + if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) { + ret = -1; } }
+ alpm_list_free(hooks_triggered);
Just a minor style suggestion: I would reverse the above conditional and move all of this inside it to avoid the unnecessary goto.
+ cleanup: alpm_list_free_inner(hooks, (alpm_list_fn_free) _alpm_hook_free); alpm_list_free(hooks); -- 2.6.4
-- apg
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Olivier Brunel