[pacman-dev] [PATCH 4/8] Update the event callback

[Olivier Brunel]

On 01/10/14 14:25, Allan McRae wrote:
> On 10/01/14 23:14, Olivier Brunel wrote:
>>
>> On 12/15/13 13:10, Allan McRae wrote:
>>> On 03/12/13 06:45, Olivier Brunel wrote:
>>>> Instead of using two void* arguments for all events, we now send one
>>>> pointer to an alpm_event_t struct. This contains the type of event that
>>>> was triggered.
>>>>
>>>> With this information, the pointer can then be typecasted to the
>>>> event-specific struct in order to get additional arguments.
>>>>
>>>> Signed-off-by: Olivier Brunel <jjk at jjacky.com>
>>>
>>> I have given couple of suggestions below for naming that I think makes
>>> things clearer.  Also a query regarding the delta event struct.
>>>
>>> Otherwise, this looks good.
>>
>> Alright, I've rebased & updated the patches regarding all the comments
>> made. A couple of things before I resend them though:
>>
>> - What's your opinion regarding what I mentioned before:
>> I don't think addressing FS#36504 was right. This was about moving the
>> log messages about a package being installed/upgraded/etc before the
>> scriptlet messages, and not after as they were. I think this might have
>> been a mistake:
>> I guess it all depends on when you consider the operation to be
>> completed, once pacman's extraction reached 100%, or after the scriptlet
>> have run. I feel the later is right, since they are part of the process
>> of installing/upgrading/etc a package; Of course, you may disagree.
>>
>> Either way, the current situation is wrong, because right now ALPM does:
>>   1. add log message about the operation being done
>>   2. run scriptlet
>>   3. signal frontend (via event) that operation is done
>>
>> IOW if a frontend was to actually use the event to present its output to
>> the user, it wouldn't match the order in the log, which is wrong. This
>> was done because pacman ignores that event, only showing the progress of
>> the extraction, and thus people feel that it goes 1. install done; then
>> 2. scriptlet.
>>
>> In fact, pacman doesn't really ignore the event, and uses it to show
>> (new) optdep. Should this be done before or after the scriptlet?
>> One could also ask, when using --noprogressbar pacman doesn't say
>> anything at the end of the operation (save for optdep), imagine it
>> would: should it be before or after the scriptlet?
>>
>> As I said, I think the logging should be moved after the scriptlet have
>> run, as this is when the operation is really done. If you were to
>> disagree though, then the event should be moved before the scriptlet
>> run, to keep consistency between the frontend and the log (which is
>> broken ATM), but this just feels wrong to me, since then a frontend
>> would consider a package upgrade/etc done, when in fact the scriptlet
>> have yet to run (and produce new output relating to the operation
>> supposedly done).
> 
> I'd like Andrew to comment here as I think that was his patch.  I think
> arguments can be make both ways here.  Maybe signal the frontend it is
> done installingg at the same time as the log and add signals that
> install scriptlets are running?  I think this is independent of your
> patchset, or am I missing something?

Yeah it is, I did keep things (in) the same (order) and the fix should
be a separate patch indeed, I just didn't want this to be forgotten is all.

> 
>> - Any comments on the last 3 patches (adding new events)?
> 
> On patchwork [1], I have flagged "Add events ALPM_EVENT_RETRIEVE_{DONE,
> FAILED}" as accepted.  I think "Add events _PKGDOWNLOAD_{START, DONE,
> FAILED}" looks OK too at first glance.  Can you put "Add events on
> pacnew/pacsave/pacorig file creation" at the end of your patch set?   It
> is probably OK, but I have not looked too hard yet and it will be easier
> to merge that last if it is at the end.

Will do.

> Cheers,
> Allan
> 
> [1] https://patchwork.archlinux.org/project/pacman/list/?state=*
>