Xavier wrote:
On Wed, Dec 31, 2008 at 6:10 AM, Bryan Ischo
<[email protected]> 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
_______________________________________________
pacman-dev mailing list
[email protected]
http://archlinux.org/mailman/listinfo/pacman-dev
