[pacman-dev] Pacman Hooks

[Sascha Kruse]

2011/1/29 Dan McGee <dpmcgee at gmail.com>:
> On Sat, Jan 29, 2011 at 8:36 AM, Sascha Kruse <knopwob at googlemail.com> wrote:
>> Anyway, here's an example config file:
>>
>> /etc/pacman.d/hooks.conf:
>> [testhook]
>>
>>    Package = lynx nano
>>    Package = vim
>>
>>    When = PreRemove
>>
>> [fonts]
>>    File = usr/share/fonts/*
>>    When = PostUpgrade
>> ---------------------------------------------------
>> You can define more than one Package that triggers a hook, either
>> separated by spaces in the "Package ="line, or in a separate "Package
>> =" line
>> or a mix of both. The "File = " argument can be anything that get
>> matched by fnmatch(...). You can also use "Package" and "File" within
>> the same hook.
>> In this case either one of them will trigger the hook (in other words:
>> Package || File).
>> "When" can be one of the following: PreRemove, PostRemove, PreUpgrade,
>> PostUpgrade, PreInstall or PostInstall.
> So are these on a per-package or per-total-operation basis? A huge
> part of hooks is something like installing a load of font packages;
> any command that these would normally run is postponed until the end
> of the transaction and then only needs to run once. Same for
> icon-cache, info reindexing, etc. or whatever other things you see in
>5 install scripts of packages.

I think the Post* hooks should be a per-total-oparation basis. I'm not
sure about the
Pre* hooks, though. (I had this mixed up in the actual code, but i'll fix that).


>> Things i don't like, but i don't really now how to fix those:
>>  - The global variable HOOK_LIST
> It belongs on the handle object, more than likely- this is where
> everything global is captured.

Okay, i'll look into that.

>>  - The paths to hooks.d and hooks.conf are hardcoded
>>  - The parsing of the config file is really ugly
> And also, seeing that the patch contains zero changes in src/...we
> never ever parse config stuff in the backend, it needs to get passed
> through by the frontend.

I wasn't sure where to put this. I had in mind that pacman is just
a frontend to libalpm, but the hooks should be used by every package manager
that is build upon libalpm since packages will depend on this feature.
So basically, every package manager that uses libalpm but isn't a
wrapper around pacman has to parse the config files itself?

>>  - The "File" argument can't begin with a / because it is matched
>> against alpm_pkg_get_data and that
>>      file-strings don't have the leading /
> This is a must-have, your logic is backwards. We never assume the
> install root is /; file paths in all install scripts should be written
> this way too, not to mention everywhere in the library. So nothing
> wrong here, it would be wrong if you did any prefix whatsoever.

I get your point, but i think it would be confusing for the users.
I would intuitively write File = /usr/share/fonts/*
instead of File = usr/share/fonts/*.
So maybe we should accept both and in the latter case ignore
the leading / ?


>>  /** Add a file target to the transaction.
>>  * @param target the name of the file target to add
>> @@ -516,6 +517,7 @@ static int commit_single_pkg(pmpkg_t *newpkg,
>> size_t pkg_current,
>>                        _alpm_runscriptlet(handle->root, newpkg->origin_data.file,
>>                                        "pre_upgrade", newpkg->version, oldpkg->version, trans);
>>                }
>> +               _trigger_hooks(newpkg, "PreUpgrade");
> If the function is in another file, it should be named _alpm, which is
> the standard prefix for "private" alpm functions.

Okay, I'll change that.


>>        return(0);
>>  }
>>
>> diff --git a/lib/libalpm/hooks.c b/lib/libalpm/hooks.c
>> new file mode 100644
>> index 0000000..e1ecfab
>> --- /dev/null
>> +++ b/lib/libalpm/hooks.c
>> @@ -0,0 +1,217 @@
>> +/*
>> + *  package.h
>> + *
>> + *  Copyright (c) 2006-2011 Pacman Development Team <pacman-dev at archlinux.org>
>> + *  Copyright (c) 2002-2006 by Judd Vinet <jvinet at zeroflux.org>
> No need for the earlier copyright if this is a new file.

The copyright comment was just copy&paste from another file. I'll change that.


>> +#include "package.h"
>> +#include "util.h"
>> +#include "hooks.h"
>> +
>> +
>> +int _run_hook(hook_t *hook)
> static
>> +{
>> +       char *cmd;
>> +       int retval;
>> +
>> +       cmd = malloc(strlen("/etc/pacman.d/hooks.d/") + strlen(hook->name) + 1);
> Use MALLOC macros please.
>> +       sprintf(cmd, "/etc/pacman.d/hooks.d/%s", hook->name);
>> +
>> +       retval = system(cmd);
> Err...take a look at util.c:_alpm_run_chroot() and the places we use
> that. You have tremendously oversimplified the setup necessary to run
> external commands and have them operate on the correct root and paths.
>
>> +       if(retval == -1) {
>> +       } else if(retval != 0) {
>> +       }
> I'll consider this work in progress?

Oh, yes. I had the calling of the XferCommand in pacman.c
as an example for that. And i forgot to substitute the pm_fprintf that
was between the if else.
But I guess I can't just use printf but I have to figure out how error messages
get passed to the frontend instead.

>> +       pkgname = alpm_pkg_get_name(pkg);
>> +       pkgfiles = alpm_pkg_get_files(pkg);
>> +
>> +       for(i = HOOK_LIST; i; i = alpm_list_next(i)) {
>> +               hook = alpm_list_getdata(i);
>> +
>> +               /* trigger package based hooks */
>> +               if(strcmp(hook->when, when) == 0 ||
>> +                               strcmp(hook->when, "Transaction") == 0) {
> Not following this logic...you are comparing to the passed in value
> and to the static string "Transaction"? Where did that come from?

The "Transaction" is another "When". Hooks with "When = Transaction" will
be triggered every time, the package or file is touched. It's for hooks that
needs to be triggered by every Pre* and Post*. I forgot to mention that
in the description above, sorry for that. And to follow the point
mentioned above:
I think this should be a per-total-operations basis.

(... big snip ...)

>> @@ -454,6 +456,7 @@ int _alpm_remove_packages(pmtrans_t *trans, pmdb_t *db)
>>                        _alpm_runscriptlet(handle->root, scriptlet, "post_remove",
>>                                        alpm_pkg_get_version(info), NULL, trans);
>>                }
>> +               _trigger_hooks(info, "PostRemve");
> Typo is probably going to not work out well. We should probably be
> using some sort of enum so we can be type-safe anyway.

I'll put the enum in hooks.h and change the rest accordingly.

>>
>>                /* remove the package from the database */
>>                _alpm_log(PM_LOG_DEBUG, "updating database\n");
>> diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
>> index 804ab7a..64b6e7d 100644
>> --- a/lib/libalpm/trans.c
>> +++ b/lib/libalpm/trans.c
>> @@ -45,6 +45,7 @@
>>  #include "sync.h"
>>  #include "alpm.h"
>>  #include "deps.h"
>> +#include "hooks.h"
>>
>>  /** \addtogroup alpm_trans Transaction Functions
>>  * @brief Functions to manipulate libalpm transactions
>> @@ -209,6 +210,7 @@ int SYMEXPORT alpm_trans_commit(alpm_list_t **data)
>>                        return(-1);
>>                }
>>        }
>> +       _run_postponed_hooks();
> Is the ldconfig call around here? I would expect it to be as that is
> the closest thing we have to a delayed hook right now.
>

No, that is in lib/libalpm/add.c and remove.c


Thank you for your comments. I hope I can post a better version soon.
Greetings,

Sascha Kruse