[pacman-dev] [PATCH] alpm: Add events for creation of pacnew/pacorig files

[Olivier Brunel]

On 11/15/13 12:28, Allan McRae wrote:

<snip>

> 
> Now I am not sure about the oldpkg/newpkg in an array passing.  I think
> just passing pkgname is a reasonable compromise.  I guess the frontend
> then can grab the old package version easily enough.

As I said earlier, I'd like to have oldpkg for pacnew, but there's
another reason to send both oldpkg & newpkg (as well as filename)
to the callback: a pacnew file can also be created when a file is in
NoUpgrade, in which case it should be oldpkg == NULL that indicates why
the pacnew was created.


Now, about Andrew's suggestion to get rid of the (void*, void*)
callback, replacing it with a pointer to a struct, I've been thinking
about this, and of all the ideas mentionned here's the one I like best:
using an alpm_event_t struct that is passed to the callback as sole
argument. This struct contains only one member, giving the type of event
we're dealing with. Many events do not actually have any arguments, so
that's that for those.

For events where there are arguments, it is then possible to recast to
the "correct"/event-specific struct, where the corresponding arguments
can be found.

This is somewhat like what I suggested earlier, only without an union
(because I'm not sure we're gaining much using one). All that's needed
is that every event-specific struct has the type as its first member.
For example:

typedef struct {
    alpm_event_type_t type;
} alpm_event_t;

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t oldpkg;
    alpm_pkg_t newpkg;
} alpm_event_upgrade_t;

void callback (alpm_event_t *_event)
{
    if (event->type == ALPM_EVENT_UPGRADE_START)
    {
        alpm_event_upgrade_t *event = (alpm_event_upgrade_t *) _event;
        printf ("upgrading %s (%s -> %s)\n",
            alpm_pkg_get_name (event->newpkg),
            alpm_pkg_get_version (event->oldpkg),
            alpm_pkg_get_version (event->newpkg));
    }
}

This allows to have (as many as needed) even-specific arguments and give
everything proper names and types. I feel it's also simpler to use and
document than having only one struct with all possible fields, and
needing to know which ones are used/available on what event.

Now, a lot of events work in pair, with a START & a DONE one. I think it
would be fine to have one single struct for both of those, with the
(currently) one exception of ALPM_EVENT_DELTA_PATCH_START where START
has a couple of arguments (pkg_filename, patch_filename) whereas DONE
has none.
But in this case I think it's fine to simply document that the
alpm_event_delta_patch_t struct only applied to START, and DONE has only
the alpm_event_t struct.

However, for events that take the same arguments but are unrelated, e.g.
the pairs for UPGRADE, DOWNGRADE & REINSTALL (which all take oldpkg &
newpkg) I think we should have one typedef for each pair of events, as
it makes things clearer (the goal being that every struct is
event-specific, not based on what arguments it (currently) takes) and
will be much better in case one of those shall take more arguments in
the future.

Going over things, here's what I'd suggest:

typedef struct {
    alpm_event_type_t type;
} alpm_event_t;

    ALPM_EVENT_CHECKDEPS_START
    ALPM_EVENT_CHECKDEPS_DONE
    ALPM_EVENT_FILECONFLICTS_START
    ALPM_EVENT_FILECONFLICTS_DONE
    ALPM_EVENT_RESOLVEDEPS_START
    ALPM_EVENT_RESOLVEDEPS_DONE
    ALPM_EVENT_INTERCONFLICTS_START
    ALPM_EVENT_INTERCONFLICTS_DONE
    ALPM_EVENT_INTEGRITY_START
    ALPM_EVENT_INTEGRITY_DONE
    ALPM_EVENT_LOAD_START
    ALPM_EVENT_LOAD_DONE
    ALPM_EVENT_DELTA_INTEGRITY_START
    ALPM_EVENT_DELTA_INTEGRITY_DONE
    ALPM_EVENT_DELTA_PATCHES_START
    ALPM_EVENT_DELTA_PATCHES_DONE
    ALPM_EVENT_DELTA_PATCH_DONE
    ALPM_EVENT_DELTA_PATCH_FAILED
    ALPM_EVENT_RETRIEVE_START
    ALPM_EVENT_DISKSPACE_START
    ALPM_EVENT_DISKSPACE_DONE
    ALPM_EVENT_KEYRING_START
    ALPM_EVENT_KEYRING_DONE
    ALPM_EVENT_KEY_DOWNLOAD_START
    ALPM_EVENT_KEY_DOWNLOAD_DONE

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t newpkg;
} alpm_event_add_t;

    ALPM_EVENT_ADD_START
    ALPM_EVENT_ADD_DONE

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t oldpkg;
} alpm_event_remove_t;

    ALPM_EVENT_REMOVE_START
    ALPM_EVENT_REMOVE_DONE

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t oldpkg;
    alpm_pkg_t newpkg;
} alpm_event_upgrade_t;

    ALPM_EVENT_UPGRADE_START
    ALPM_EVENT_UPGRADE_DONE

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t oldpkg;
    alpm_pkg_t newpkg;
} alpm_event_downgrade_t;

    ALPM_EVENT_DOWNGRADE_START
    ALPM_EVENT_DOWNGRADE_DONE

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t oldpkg;
    alpm_pkg_t newpkg;
} alpm_event_reinstall_t;

    ALPM_EVENT_REINSTALL_START
    ALPM_EVENT_REINSTALL_DONE

Note that really the 3 typedefs above could resolve to the same
_alpm_event_update_t struct.

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t pkg;
    alpm_depend_t optdep;
} alpm_event_optdep_t;

    ALPM_EVENT_OPTDEP_REQUIRED

Might be me, but isn't that one "oddly" named. It is emitted when an
optdep is being removed, so wouldn't ALPM_EVENT_OPTDEP_REMOVED be more
appropriate? Since we're talking optdep here, there's really no
requirement. Just because a package that is optdep for another one is
removed doesn't mean it was even used as optdep, much less is required, no?

typedef struct {
    alpm_event_type_t type;
    const char *pkg_filename;
    const char *patch_filename;
} alpm_event_delta_patch_t;

    ALPM_EVENT_DELTA_PATCH_START

typedef struct {
    alpm_event_type_t type;
    const char *line;
} alpm_event_scriptlet_t;

    ALPM_EVENT_SCRIPTLET_INFO

typedef struct {
    alpm_event_type_t type;
    const char *dbname;
} alpm_event_database_missing_t;

    ALPM_EVENT_DATABASE_MISSING

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t *oldpkg; /* NULL when creation from NoUpgrade */
    alpm_pkg_t *newpkg;
    const char *oldfile;
} alpm_event_pacnew_t;

    ALPM_EVENT_PACNEW_CREATED

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t *newpkg;
    const char *oldfile;
} alpm_event_pacorig_t;

    ALPM_EVENT_PACORIG_CREATED

typedef struct {
    alpm_event_type_t type;
    alpm_pkg_t *oldpkg;
    const char *oldfile;
} alpm_event_pacsave_t;

    ALPM_EVENT_PACSAVE_CREATED

And new events would need to be created, to get rid of the log_cb:

typedef struct {
    alpm_event_type_t type;
    const char *message;
} alpm_event_warning_t;

    ALPM_EVENT_WARNING

typedef struct {
    alpm_event_type_t type;
    const char *message;
} alpm_event_error_t;

    ALPM_EVENT_ERROR

typedef struct {
    alpm_event_type_t type;
    const char *message;
} alpm_event_debug_t;

    ALPM_EVENT_DEBUG

typedef struct {
    alpm_event_type_t type;
    const char *message;
} alpm_event_function_t;

    ALPM_EVENT_FUNCTION

As before, all 4 typedef could resolve to the same _alpm_event_msg_t struct.
(Unless you'd rather have one single event, and an level to indicate
whether it is a warning/error/etc ?)

I think all debug messages & errors shall remain as such. For warnings
though, we might want to turn some into actual events. In addition to
the creation of pac* files, I'd suggest the following:

typedef struct {
    alpm_event_type_t type;
    const char *url;
} alpm_event_download_t;

    ALPM_EVENT_DOWNLOAD_DONE
    ALPM_EVENT_DOWNLOAD_FAILED

There's a warning in alpm_fetch_pkgurl() in case of failure; I think it
might be good to turn that into an event, but also add such events in
download_files() (in sync.c) instead of, also, just a generic warning on
failure ("failed to retrieve some files").

Also, ALPM_EVENT_RETRIEVE_START doesn't have a matching DONE and I think
it'd be a good idea to add one as well (so a frontend can know when the
downloading part is done) (Maybe a FAILED too?).

Thoughts?
-j