[pacman-dev] PATCH: Fix bug 9395 (allow sync to remove unresolvable packages if user wishes)
Bryan Ischo
bji-keyword-pacman.3644cb at www.ischo.com
Wed Dec 31 14:27:39 EST 2008
Xavier wrote:
> On Wed, Dec 31, 2008 at 6:10 AM, Bryan Ischo
> <bji-keyword-pacman.3644cb at 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
More information about the pacman-dev
mailing list