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

[Andrew Gregory]

On 11/15/13 at 02:44pm, Olivier Brunel wrote:
> On 11/15/13 12:28, Allan McRae wrote:
> > On 10/11/13 04:49, Olivier Brunel wrote:
> >> Signed-off-by: Olivier Brunel <jjk at jjacky.com>
> >> ---
> >> Hey,
> >>
> >> Some background regarding this: I've written a little tool (kalu, [1]) which is
> >> an upgrade notifier, and also features a GUI/frontend for the sysupgrade (-Syu)
> >> operation.
> >>
> >> Whenever there was a pacnew installed, I would open a terminal and run vimdiff
> >> on the files; I would also wonder if it wouldn't be nice to have a third file
> >> featured, the "original" one (i.e. from the previous package). And so I started
> >> wondering about kalu making a list of all pacnew files created during the
> >> upgrade, as well as the name & version of the old package, so that I could use a
> >> little script to extract the file and start the 3-way diff.
> >>
> >> (kalu allows to execute one or more command lines after the sysupgrade
> >> completed, so this list is just a variable parsed on said cmdlines.)
> >>
> >> This all works well, but to make that list, kalu would have to e.g. use a regex
> >> to extract the pacnew filenames from ALPM's warnings, and that's just not a very
> >> good or reliable thing, especially when you know such warnings are translated.
> >>
> >> Hence, why adding a couple events (there's pacorig as well) in ALPM makes
> >> everything much better & simpler; And knowing about such events could be useful
> >> to some frontends, even though pacman obviously has no use for it (atm).
> >>
> >> So, I'm hoping you'd be willing to merge this.
> >>
> >> Thanks,
> >> -j
> >>
> >> [1] http://jjacky.com/kalu
> > 
> > I agree the backend should report this to the frontend.   In fact, I
> > would like the backend to be completely quiet and report everything to
> > the frontend.
> 
> Not sure what you mean by "completely quiet" ? When I read that, I took
> it to mean as far as the log goes (since ALPM doesn't output anything),
> that you'd like for frontends to be doing all the logging...
> 
> But that goes against a recent patch[1] you committed, which does the
> very opposite, and moved logging from pacman into ALPM. So I'm not sure
> I understand what you mean here.

The problem is the use of _alpm_log.  Even though alpm doesn't
actually output the message, it's passing a translated string to the
frontend which can't decipher or act on it.  The frontend effectively
acts as a simple middle-man for alpm's output.  pacman should be
printing the output from the event callback, allowing it to
translate/colorize/etc the output.

> > Lets look at what your patch currently does:
> > 
> > alpm_pkg_t *arr[2] = { oldpkg, newpkg };
> > EVENT(handle, ALPM_EVENT_PACNEW, arr, newpath);
> > 
> > First the passed path.  I think passing filename (e.g. /etc/pacman.conf)
> > rather than newpath (e.g. /etc/pacman.conf.pacsave) is the better
> > approach for that.  It requires less to output a ".pacsave" than to
> > remove one.
> 
> Agreed.
> 
> > 
> > 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.
> 
> Well, the reason I did this was that - as explained - I wanted to have
> the old version, but it makes sense to also have the new one (plus, I
> felt it was also better for consistency w/ pacsave files).
> 
> This was a simple way to make sure the frontend has all correct info.
> Otherwise, the frontend needs to store that event somewhere, then wait
> for the next UPGRADE_DONE event to assign the version number(s) to all
> "unassigned/versionless" pacnew/pacsave files. It just doesn't feel as
> good/reliable.
> 
> I could have just put oldpkg, newpkg being easy enough to get if one
> needs to, since it should be the currently installed one, but then
> pacnew would have had oldpkg, and pacsave newpkg (since there's no
> oldpkg), which I thought might not be the best re: consistency.
> Although, since both don't have the same arguments either way, I'm not
> sure there's consistency if any case actually.
> 
> If you're against the array passing, I'd like to use oldpkg for pacnew &
> newpkg for pacsave better than just passing the pkgname.

At the time of extraction oldpkg has already been removed from the
local package cache and newpkg hasn't been added yet, so I don't think
there is any reliable way to get at either of them unless they are
passed directly to the callback.  

How about switching from a (void*, void*) callback to an
event_context_t struct?  We could pass as much information to the
callback as we need and give everything proper names and types.
Alternatively, we could pass an alpm_list_t so that we're at least not
arbitrarily limited to two arguments.  A list with mixed types is
ugly, but shouldn't be too horrible as long as the backend takes care
of freeing everything that needs it.

> >>
> >>  lib/libalpm/add.c     |  3 +++
> >>  lib/libalpm/alpm.h    | 10 +++++++++-
> >>  src/pacman/callback.c |  3 +++
> >>  3 files changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> >> index ac4e36a..e5eca97 100644
> >> --- a/lib/libalpm/add.c
> >> +++ b/lib/libalpm/add.c
> >> @@ -363,10 +363,12 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
> >>  				if(try_rename(handle, checkfile, newpath)) {
> >>  					errors++;
> >>  				} else {
> >> +					alpm_pkg_t *arr[2] = { oldpkg, newpkg };
> >>  					_alpm_log(handle, ALPM_LOG_WARNING, _("%s installed as %s\n"),
> >>  							filename, newpath);
> >>  					alpm_logaction(handle, ALPM_CALLER_PREFIX,
> >>  							"warning: %s installed as %s\n", filename, newpath);
> >> +					EVENT(handle, ALPM_EVENT_PACNEW, arr, newpath);
> >>  				}
> >>  
> >>  				free(newpath);
> >> @@ -395,6 +397,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
> >>  								_("%s saved as %s\n"), filename, newpath);
> >>  						alpm_logaction(handle, ALPM_CALLER_PREFIX,
> >>  								"warning: %s saved as %s\n", filename, newpath);
> >> +						EVENT(handle, ALPM_EVENT_PACORIG, newpkg, newpath);
> >>  					}
> >>  				}
> >>  
> >> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> >> index 2c8c1e6..c4f772d 100644
> >> --- a/lib/libalpm/alpm.h
> >> +++ b/lib/libalpm/alpm.h
> >> @@ -387,7 +387,15 @@ typedef enum _alpm_event_t {
> >>  	/** Downloading missing keys into keyring. */
> >>  	ALPM_EVENT_KEY_DOWNLOAD_START,
> >>  	/** Key downloading is finished. */
> >> -	ALPM_EVENT_KEY_DOWNLOAD_DONE
> >> +	ALPM_EVENT_KEY_DOWNLOAD_DONE,
> >> +	/** A .pacnew file was installed
> >> +	 * An array of packages (current/old one, and installed/new one) and the
> >> +	 * name of the pacnew file are passed to the callback */
> >> +	ALPM_EVENT_PACNEW,
> >> +	/** A .pacorig file was created
> >> +	 * The installed/new package and the name of the pacorig file are passed to
> >> +	 * the callback */
> >> +	ALPM_EVENT_PACORIG

Rename these ALPM_EVENT_*_CREATED or something similar so they match
up with the other event names a little better and to avoid confusion
if we ever add any other pacnew/pacorig related events.  We should
also add a similar event for pacsave files for consistency.

> >>  } alpm_event_t;
> >>  
> >>  /** Event callback */
> >> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> >> index 36531a2..0fc35f6 100644
> >> --- a/src/pacman/callback.c
> >> +++ b/src/pacman/callback.c
> >> @@ -297,6 +297,9 @@ void cb_event(alpm_event_t event, void *data1, void *data2)
> >>  		case ALPM_EVENT_DELTA_INTEGRITY_DONE:
> >>  		case ALPM_EVENT_DELTA_PATCHES_DONE:
> >>  		case ALPM_EVENT_DISKSPACE_DONE:
> >> +		/* events of creation of .pacnew/.pacorig files */
> >> +		case ALPM_EVENT_PACNEW:
> >> +		case ALPM_EVENT_PACORIG:

This is where "foo installed as bar" needs to be printed.  Then we can
easily implement FS#37711.

> >>  			/* nothing */
> >>  			break;
> >>  	}
> >>