Signed-off-by: Olivier Brunel jjk@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
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 } 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: /* nothing */ break; }
On 10/11/13 04:49, Olivier Brunel wrote:
Signed-off-by: Olivier Brunel jjk@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
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.
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.
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.
Allan
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
} 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: /* nothing */ break;
On 11/15/13 12:28, Allan McRae wrote:
On 10/11/13 04:49, Olivier Brunel wrote:
Signed-off-by: Olivier Brunel jjk@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
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.
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.
-j
Allan
[1] https://mailman.archlinux.org/pipermail/pacman-dev/2013-November/018166.html
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
} 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: /* nothing */ break;
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@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
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;}
On 11/15/13 16:15, Andrew Gregory wrote:
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@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
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.
Oh, so the goal would be to have ALPM do the logging, but let the frontend print something to the user based on the event instead of just relaying whatever was logged.
How would that work? If ALPM both triggers the event and still writes to the log, and also triggers the log callback, the frontend has no way of knowing when to ignore certain log messages (because an event was/will be triggered for it) and when not to.
So it would then require to remove the log callback, leaving the log handled (quietly, I get it now ;)) by ALPM, but making sure everything that's logged has a corresponding event, specifically we'd need a SCRIPTLET_OUTOUT event of some kind, as well as events for warnings/errors/debug messages.
Would that be right?
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.
You mean change the event callback signature ? Well, that's a(nother) major API break then; And I guess it's not for the scope of this patch. This is about adding events for pac* files, there should be another patch changing the event callback signature & removing the log callback, as described above.
If that were to happen though, I'm not sure I see much improvement over the current solution in using an event_context_t struct, I mean there can't be one type that fits all events nor can everything to be needed in the future be thought of ahead of time, so it doesn't seem ideal to me.
A list allows more than 2 arguments, but really you just need one, e.g. an event_t union, to be a different struct based on the actual event. If you know a bit about GTK, similar to how they have GdkEvent an union of GkEventButton, GdkEventKey, etc
Wouldn't that be simpler? (Or what that what you meant already?)
-j
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;}
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
pacman-dev@lists.archlinux.org