Xavier wrote:
On Wed, Dec 31, 2008 at 6:10 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
Thank you very much for your feedback, Xavier! Comments inline below ...
Maybe we could simply get totally rid of this prompt, and avoid this option altogether. I think you already suggested that somewhere, and it might be fine. We are usually against interaction.
(Xavier is referring here to the prompt where libalpm issues a query (via a callback) about whether or not the user wants to upgrade a package which was in IgnorePkg/IgnoreGroup should that package be needed to upgrade another package in the transaction. My changes add a config flag allowing this prompt to be skipped. I personally would like to see that prompt removed entirely, as you have suggested. My reasoning is that if the user has put a package in IgnorePkg/IgnoreGroup, it's because they really want it to be ignored, so asking them if they are sure is unnecessary. However, I don't feel that I can make a call like this - I think it's up to the main pacman devs, so I'd like to hear others' opinions on this. I can certainly remove that prompt entirely in a new patch, and would in fact like to do that, but I won't unless instructed to by the pacman dev team. One thing that I think could help make that question even more redundant is if the information given to the user in the new prompt I have created was more thorough. Right now it just tells the user what packages need to be removed from the transaction, but it doesn't say what packages were unresolvable or why. If the prompt looked something more like this: :: the following packages cannot be upgraded due to unresolvable dependencies: firefox 3.0.5, requires xulrunner 1.0.9 xulrunner 1.0.9, requires pango-1.6 pango-1.6, ignored by user configuration Then at least the user would always be aware of what effect their IgnorePkg/IgnoreGroup settings is having on the sync operation. I wanted to do something like this, but was not ambitious enough, as it would require quite a more sophisticated data structure being passed to the query callback, and for the query callback to be significantly more complicated.
I see that we need to keep tracks of all the parent dependencies, so that when we want to remove a package from the targets, we remove all the parent dependencies as well. We might want to re-use the existing pmgraph_t structure we have for that, instead of adding a new one.
Thanks for the suggestion, I will look into that. I was unaware of the existence of this structure; if it can replace pkginfo_t, I will do so.
As for the rest, we could adopt a more general approach. We add a SkipUnresolvable option in pacman.conf. If this option is enabled, we simply skip all unresolvable packages from the target list, no matter if it was caused by an ignored package or not.
I am not sure I understand what you are saying here. Packages are already marked unresolvable whether it was due to being in IgnorePkg/IgnoreGroup or for some other reason, the reason is not kept track of or used in the resolution process. I think that what you're saying is that the skipping should be made automatic instead of prompting the user whether or not to do it. Actually I would agree with that also; perhaps this information can simply be provided in the final "Proceed with installation?" prompt. Something like this: Targets (29): openssl-0.9.8i-3 ca-certificates-20080809-5 libpng-1.2.34-1 xcb-util-0.3.2-1 cairo-1.8.6-1 dbus-glib-0.78-1 diffutils-2.8.1-6 libtasn1-1.7-1 gnutls-2.6.3-1 hal-info-0.20081219-1 hdparm-9.3-1 hwdetect-2008.12-3 intel-dri-7.2-2 klibc-udev-135-1 libdmx-1.0.2-2 libxml2-2.7.2-1 libgsf-1.14.10-1 libxfont-1.3.4-1 nss-3.12.2-1 pacman-3.2.1-2 pycairo-1.8.0-1 subversion-1.5.5-1 sudo-1.7.0-1 tar-1.20-3 ttf-dejavu-2.28-1 udev-135-1 xextproto-7.0.4-1 xkeyboard-config-1.4-2 xorg-font-utils-7.4-1 Skipped Targets (4): firefox-3.0.5, xulrunner-1.0.9, pango-1.6 Total Download Size: 23.64 MB Total Installed Size: 80.21 MB Proceed with installation? [Y/n] n This would be nice because it would avoid the need for another confirmation question (since the "Proceed with installation" serves the same purpose). However, adding the Skipped Targets to this prompt might a) confuse the user, and b) result in too much verbage (could be a very large list if IgnorePkg/IgnoreGroup has packages "low down" in the dependency tree). Also, it would require much bigger changes to the code. Also, the changes currently are nicely self-contained because they simply change the behavior of _alpm_resolvedeps, they don't reach out into other functions, forcing them to keep track of this information so that they can ultimately produce the new Proceed prompt. And finally, I haven't looked to see all of the places that _alpm_resolvedeps is called, but it's possible that there are other situations that it's called in that really should issue that "skip these packages" prompt immediately rather than deferring to some later prompt, which might not even happen for that particular code path. All in all, I like the idea, but I think it might be better left to a v2.0 version of this feature.
+static pkginfo_t *_alpm_findinfo(alpm_list_t *list, pmpkg_t *pkg) +{ + alpm_list_t *i; + + for (i = list; i; i = i->next) { + pkginfo_t *info = (pkginfo_t *) i->data; + if (info->pkg == pkg) { + return info; + } + } + + return NULL; +} + +
<snip>
diff -rupN -x '*\.git*' pacman/lib/libalpm/deps.h pacman-fixbug9395/lib/libalpm/deps.h --- pacman/lib/libalpm/deps.h 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/deps.h 2008-12-31 17:52:41.000000000 +1300 @@ -48,8 +48,8 @@ void _alpm_depmiss_free(pmdepmissing_t * alpm_list_t *_alpm_sortbydeps(alpm_list_t *targets, int reverse); void _alpm_recursedeps(pmdb_t *db, alpm_list_t *targs, int include_explicit); pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, alpm_list_t *excluding, pmpkg_t *tpkg); -int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, alpm_list_t *list, - alpm_list_t *remove, alpm_list_t **data); +int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, alpm_list_t **list, + alpm_list_t **pulled, alpm_list_t *remove, alpm_list_t **data);
Looks like these changes are indeed needed, to be able to remove packages from "list". Maybe for consistency we could do the same change for the "remove" list, even if it is not needed. Well, I am undecided on this one.
I'm afraid I don't quite understand this comment. Can you please clarify?
We must have already something to display a list of targets, somewhere in the pacman frontend code. If the existing functions are not directly usable for this use case, we will probably need to factor out some code.
I will take a look; if there is code that I can easily re-use, I will do so. However, one of my goals with this patch was to keep it very self-contained (it just modifies the behavior of _alpm_resolvedeps and makes a minor change to its signature, also it adds a callback handler to pacman for the new question callback, otherwise it doesn't change anything), because I don't know the pacman code base well enough to feel confident changing it around too much.
diff -rupN -x '*\.git*' pacman/src/pacman/pacman.c pacman-fixbug9395/src/pacman/pacman.c --- pacman/src/pacman/pacman.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/src/pacman/pacman.c 2008-12-31 17:53:00.000000000 +1300 @@ -137,15 +137,16 @@ static void usage(int op, const char * c " ignore a group upgrade (can be used more than once)\n")); printf(_(" -q, --quiet show less information for query and search\n")); } - printf(_(" --config <path> set an alternate configuration file\n")); - printf(_(" --logfile <path> set an alternate log file\n")); - printf(_(" --noconfirm do not ask for any confirmation\n")); - printf(_(" --noprogressbar do not show a progress bar when downloading files\n")); - printf(_(" --noscriptlet do not execute the install scriptlet if one exists\n")); - printf(_(" -v, --verbose be verbose\n")); - printf(_(" -r, --root <path> set an alternate installation root\n")); - printf(_(" -b, --dbpath <path> set an alternate database location\n")); - printf(_(" --cachedir <dir> set an alternate package cache location\n")); + printf(_(" --config <path> set an alternate configuration file\n")); + printf(_(" --logfile <path> set an alternate log file\n")); + printf(_(" --noconfirm do not ask for any confirmation\n")); + printf(_(" --noprogressbar do not show a progress bar when downloading files\n")); + printf(_(" --noscriptlet do not execute the install scriptlet if one exists\n")); + printf(_(" --noignoreprompt don't prompt to confirm packages in IngorePkg and IgnoreGroup\n")); + printf(_(" -v, --verbose be verbose\n")); + printf(_(" -r, --root <path> set an alternate installation root\n")); + printf(_(" -b, --dbpath <path> set an alternate database location\n")); + printf(_(" --cachedir <dir> set an alternate package cache location\n")); } }
These changes are strange. Maybe a whitespace / tab issue?
Almost certainly. My xxdiff showed only the last bits, surrounding the --noignoreprompt option that I had added, as changing, so I'm not sure why diff pulled all the rest in. I will investigate. Thank you again for your comments. I will be issuing an updated patch shortly which incorporates your suggetsions as well as those of later posters (who I will respond to in turn soon). Best wishes, Bryan