[pacman-dev] PATCH: Fix bug 9395 (allow sync to remove unresolvable packages if user wishes)
Hi, attached is a patch to fix bug 9395, against the current git tree for pacman, as described in an earlier email to this list. You may find a description of the changes I made in the Arch bug database for bug 9395. Please let me know if there are any questions. Thank you, and best wishes, Bryan diff -rupN -x '*\.git*' pacman/doc/pacman.8.txt pacman-fixbug9395/doc/pacman.8.txt --- pacman/doc/pacman.8.txt 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/doc/pacman.8.txt 2008-12-31 17:52:33.000000000 +1300 @@ -152,6 +152,11 @@ Options If an install scriptlet exists, do not execute it. Do not use this unless you know what you are doing. +*\--noignoreprompt*:: + Do not prompt the user to ask for confirmation that packages listed in + IgnorePkg and IgnoreGroup really should be ignored; instead, assume + that they should be and silently ignore them. + Query Options[[QO]] ------------------- diff -rupN -x '*\.git*' pacman/lib/libalpm/alpm.h pacman-fixbug9395/lib/libalpm/alpm.h --- pacman/lib/libalpm/alpm.h 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/alpm.h 2008-12-31 17:59:38.000000000 +1300 @@ -377,7 +377,7 @@ typedef enum _pmtransconv_t { PM_TRANS_CONV_CONFLICT_PKG = 0x04, PM_TRANS_CONV_CORRUPTED_PKG = 0x08, PM_TRANS_CONV_LOCAL_NEWER = 0x10, - /* 0x20 flag can go here */ + PM_TRANS_CONV_REMOVE_PKGS = 0x20, PM_TRANS_CONV_REMOVE_HOLDPKG = 0x40 } pmtransconv_t; diff -rupN -x '*\.git*' pacman/lib/libalpm/deps.c pacman-fixbug9395/lib/libalpm/deps.c --- pacman/lib/libalpm/deps.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/deps.c 2008-12-31 17:52:41.000000000 +1300 @@ -546,17 +546,92 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *de return(NULL); } +typedef struct __pkginfo_t +{ + /* The package for which this info is being kept */ + pmpkg_t *pkg; + /* 0 if this package has been determined to be unresolvable, meaning that + it has dependencies that cannot be resolved, nonzero otherwise */ + int unresolvable; + /* 0 if this package was not pulled, nonzero if this package was pulled */ + int pulled; + /* Packages that are immediately dependent on this package. */ + alpm_list_t *dependents; +} pkginfo_t; + + +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; +} + + +static void _alpm_mark_unresolvable(alpm_list_t *list, pkginfo_t *info) +{ + alpm_list_t *i; + + if (info->unresolvable) { + return; + } + + info->unresolvable = 1; + + for (i = info->dependents; i; i = i->next) { + _alpm_mark_unresolvable(list, _alpm_findinfo(list, ((pmpkg_t *) i->data))); + } +} + +static void _alpm_info_free(pkginfo_t *info) +{ + alpm_list_free(info->dependents); + free(info); +} + + +static int _alpm_is_needed(alpm_list_t *infolist, pkginfo_t *info) +{ + /* Obviously if it's already been marked unresolvable, it is not needed */ + if (info->unresolvable) { + return(0); + } + /* Now, if all of the top-level packages which depend on it are + unresolvable, then it is unneeded */ + else { + alpm_list_t *i; + for (i = info->dependents; i; i = i->next) { + if (!_alpm_is_needed(infolist, _alpm_findinfo(infolist, (pmpkg_t *) i->data))) { + return(0); + } + } + + return(1); + } +} + + /* populates list with packages that need to be installed to satisfy all * dependencies of packages in list * * @param remove contains packages elected for removal */ -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) { + int ret = 0; alpm_list_t *i, *j; alpm_list_t *targ; alpm_list_t *deps = NULL; + alpm_list_t *info = NULL; + alpm_list_t *unresolvable = NULL; ALPM_LOG_FUNC; @@ -565,8 +640,29 @@ int _alpm_resolvedeps(pmdb_t *local, alp } _alpm_log(PM_LOG_DEBUG, "started resolving dependencies\n"); - for(i = list; i; i = i->next) { + + /* Build up a list of pkginfo_t structures describing the root level + packages */ + for(i = *list; i; i = i->next) { + pkginfo_t *thisinfo; + thisinfo = (pkginfo_t *) malloc(sizeof(pkginfo_t)); + if (thisinfo == NULL) { + pm_errno = PM_ERR_MEMORY; + ret = -1; + goto cleanup; + } + thisinfo->pkg = i->data; + thisinfo->unresolvable = 0; + thisinfo->pulled = 0; + thisinfo->dependents = NULL; + info = alpm_list_add(info, thisinfo); + } + + /* Now resolve */ + for(i = *list; i; i = i->next) { pmpkg_t *tpkg = i->data; + /* Find the info for tpkg */ + pkginfo_t *tinfo = _alpm_findinfo(info, tpkg); targ = alpm_list_add(NULL, tpkg); deps = alpm_checkdeps(_alpm_db_get_pkgcache(local), 0, remove, targ); alpm_list_free(targ); @@ -574,38 +670,122 @@ int _alpm_resolvedeps(pmdb_t *local, alp pmdepmissing_t *miss = j->data; pmdepend_t *missdep = alpm_miss_get_dep(miss); /* check if one of the packages in list already satisfies this dependency */ - if(_alpm_find_dep_satisfier(list, missdep)) { + pmpkg_t *spkg = _alpm_find_dep_satisfier(*list, missdep); + if(spkg != NULL) { + /* Check spkg to make sure that it was not unresolvable; if it is, mark tpkg as + unresolvable also */ + pkginfo_t *sinfo = _alpm_findinfo(info, spkg); + if (sinfo->unresolvable) { + _alpm_mark_unresolvable(info, tinfo); + /* No need to do any further dependency + checking for tpkg, even if it has other + dependencies, tpkg will not be installed + because it is not resolvable */ + break; + } + sinfo->dependents = alpm_list_add(sinfo->dependents, tpkg); continue; } /* find a satisfier package in the given repositories */ - pmpkg_t *spkg = _alpm_resolvedep(missdep, dbs_sync, list, tpkg); - if(!spkg) { - pm_errno = PM_ERR_UNSATISFIED_DEPS; - char *missdepstring = alpm_dep_get_string(missdep); - _alpm_log(PM_LOG_ERROR, _("cannot resolve \"%s\", a dependency of \"%s\"\n"), - missdepstring, tpkg->name); - free(missdepstring); - if(data) { - pmdepmissing_t *missd = _alpm_depmiss_new(miss->target, - miss->depend, miss->causingpkg); - if(missd) { - *data = alpm_list_add(*data, missd); - } - } - alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_depmiss_free); - alpm_list_free(deps); - return(-1); - } else { + spkg = _alpm_resolvedep(missdep, dbs_sync, *list, tpkg); + if(spkg != NULL) { _alpm_log(PM_LOG_DEBUG, "pulling dependency %s (needed by %s)\n", alpm_pkg_get_name(spkg), alpm_pkg_get_name(tpkg)); - list = alpm_list_add(list, spkg); + *list = alpm_list_add(*list, spkg); + pkginfo_t *sinfo; + sinfo = (pkginfo_t *) malloc(sizeof(pkginfo_t)); + if (sinfo == NULL) { + pm_errno = PM_ERR_MEMORY; + ret = -1; + goto cleanup; + } + sinfo->pkg = spkg; + sinfo->unresolvable = 0; + sinfo->pulled = 1; + sinfo->dependents = alpm_list_add(NULL, tpkg); + info = alpm_list_add(info, sinfo); + } else { + /* tpkg is not resolvable, so mark it and all of + its dependents as such */ + _alpm_mark_unresolvable(info, tinfo); } } alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_depmiss_free); alpm_list_free(deps); + deps = NULL; + } + + /* Build up a list of unresolvable top-level packages, so that they + can be queried about */ + for(i = info; i; i = i->next) { + pkginfo_t *info = (pkginfo_t *) i->data; + if (info->unresolvable && !info->pulled) { + unresolvable = alpm_list_add(unresolvable, info->pkg); + } + } + + /* If there were unresolvable packages, query the user to see if they + should be removed from the transaction */ + if(unresolvable != NULL) { + int remove_unresolved = 0; + QUESTION(handle->trans, PM_TRANS_CONV_REMOVE_PKGS, unresolvable, NULL, NULL, &remove_unresolved); + if (remove_unresolved) { + /* Need to remove all packages which are: + - unresolvable, OR + - are pulled elements whose entire list of top-level + dependents are unresolvable */ + for (i = info; i; i = i->next) { + pkginfo_t *thisinfo = (pkginfo_t *) i->data; + if (!_alpm_is_needed(info, thisinfo)) { + *list = alpm_list_remove(*list, thisinfo->pkg, _alpm_pkg_cmp, NULL); + } + } + } else { + alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free(deps); + deps = NULL; + pm_errno = PM_ERR_UNSATISFIED_DEPS; + _alpm_log(PM_LOG_ERROR, _("cannot resolve dependencies for:\n")); + for (i = unresolvable; i; i = i->next) { + _alpm_log(PM_LOG_ERROR, _("\t%s\n"), alpm_pkg_get_name((pmpkg_t *) i->data)); + if (data) { + alpm_list_t *targ = alpm_list_add(NULL, i->data); + deps = alpm_checkdeps(_alpm_db_get_pkgcache(local), 0, remove, targ); + alpm_list_free(targ); + } + } + for(i = deps; i; i = i->next) { + pmdepmissing_t *miss = i->data; + pmdepmissing_t *missd = _alpm_depmiss_new(miss->target, miss->depend, miss->causingpkg); + if(missd) { + *data = alpm_list_add(*data, missd); + } + } + ret = -1; + } } + + /* Set pulled to be the last top-level item in the list */ + *pulled = NULL; + for(i = *list; i; i = i->next) { + pkginfo_t *thisinfo = _alpm_findinfo(info, i->data); + if (thisinfo->pulled) { + *pulled = i; + break; + } + } + + cleanup: + + alpm_list_free_inner(deps, (alpm_list_fn_free)_alpm_depmiss_free); + alpm_list_free(deps); + alpm_list_free_inner(info, (alpm_list_fn_free)_alpm_info_free); + alpm_list_free(info); + alpm_list_free(unresolvable); + _alpm_log(PM_LOG_DEBUG, "finished resolving dependencies\n"); - return(0); + + return(ret); } /* Does pkg1 depend on pkg2, ie. does pkg2 satisfy a dependency of pkg1? */ 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); int _alpm_dep_edge(pmpkg_t *pkg1, pmpkg_t *pkg2); pmdepend_t *_alpm_splitdep(const char *depstring); pmpkg_t *_alpm_find_dep_satisfier(alpm_list_t *pkgs, pmdepend_t *dep); diff -rupN -x '*\.git*' pacman/lib/libalpm/sync.c pacman-fixbug9395/lib/libalpm/sync.c --- pacman/lib/libalpm/sync.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/sync.c 2008-12-31 18:01:17.000000000 +1300 @@ -417,9 +417,10 @@ int _alpm_sync_prepare(pmtrans_t *trans, } if(!(trans->flags & PM_TRANS_FLAG_NODEPS)) { - /* store a pointer to the last original target so we can tell what was - * pulled by resolvedeps */ - alpm_list_t *pulled = alpm_list_last(list); + /* resolvedeps returns a pointer to the first element of the + * list which is a pulled element, and all elements after that + * are pulled as well */ + alpm_list_t *pulled; /* Resolve targets dependencies */ EVENT(trans, PM_TRANS_EVT_RESOLVEDEPS_START, NULL, NULL); _alpm_log(PM_LOG_DEBUG, "resolving target's dependencies\n"); @@ -432,13 +433,13 @@ int _alpm_sync_prepare(pmtrans_t *trans, } } - if(_alpm_resolvedeps(db_local, dbs_sync, list, remove, data) == -1) { + if(_alpm_resolvedeps(db_local, dbs_sync, &list, &pulled, remove, data) == -1) { /* pm_errno is set by resolvedeps */ ret = -1; goto cleanup; } - for(i = pulled->next; i; i = i->next) { + for(i = pulled; i; i = i->next) { pmpkg_t *spkg = i->data; pmsyncpkg_t *sync = _alpm_sync_new(PM_PKG_REASON_DEPEND, spkg, NULL); if(sync == NULL) { diff -rupN -x '*\.git*' pacman/src/pacman/callback.c pacman-fixbug9395/src/pacman/callback.c --- pacman/src/pacman/callback.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/src/pacman/callback.c 2008-12-31 18:04:16.000000000 +1300 @@ -248,7 +248,10 @@ void cb_trans_conv(pmtransconv_t event, { switch(event) { case PM_TRANS_CONV_INSTALL_IGNOREPKG: - if(data2) { + if(config->noignoreprompt) { + *response = 0; + } + else if(data2) { /* TODO we take this route based on data2 being not null? WTF */ *response = yesno(_(":: %s requires installing %s from IgnorePkg/IgnoreGroup. Install anyway?"), alpm_pkg_get_name(data2), @@ -274,6 +277,31 @@ void cb_trans_conv(pmtransconv_t event, (char *)data2, (char *)data2); break; + case PM_TRANS_CONV_REMOVE_PKGS: + { + /* Allocate a buffer big enough to hold all of the + package names */ + char *packagenames; + alpm_list_t *unresolved = (alpm_list_t *) data1; + alpm_list_t *i; + int len = 1, /* for trailing \0 */ where = 0, count = 0; + for (i = unresolved; i; i = i->next) { + count += 1; + len += 3 /* for \t, comma, and \n */ + + strlen(alpm_pkg_get_name(i->data)); + } + packagenames = (char *) malloc(len); + for (i = unresolved; i; i = i->next) { + where += snprintf(&(packagenames[where]), len - where, "\t%s%s\n", + alpm_pkg_get_name(i->data), (i->next) ? "," : ""); + } + *response = yesno(_(":: the following package%s cannot be upgraded due to unresolvable " + "dependencies:\n%s\nDo you want to skip %s package%s for this upgrade?"), + (count > 1) ? "s" : "", packagenames, (count > 1) ? "these" : "this", + (count > 1) ? "s" : ""); + free(packagenames); + } + break; case PM_TRANS_CONV_LOCAL_NEWER: if(!config->op_s_downloadonly) { *response = yesno(_(":: %s-%s: local version is newer. Upgrade anyway?"), diff -rupN -x '*\.git*' pacman/src/pacman/conf.c.rej pacman-fixbug9395/src/pacman/conf.c.rej --- pacman/src/pacman/conf.c.rej 1970-01-01 12:00:00.000000000 +1200 +++ pacman-fixbug9395/src/pacman/conf.c.rej 2008-12-31 17:53:00.000000000 +1300 @@ -0,0 +1,16 @@ +*************** +*** 48,53 **** + newconfig->dbpath = NULL; + newconfig->logfile = NULL; + newconfig->syncfirst = NULL; + + return(newconfig); + } +--- 48,54 ---- + newconfig->dbpath = NULL; + newconfig->logfile = NULL; + newconfig->syncfirst = NULL; ++ newconfig->noignoreprompt = 0; + + return(newconfig); + } diff -rupN -x '*\.git*' pacman/src/pacman/conf.h pacman-fixbug9395/src/pacman/conf.h --- pacman/src/pacman/conf.h 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/src/pacman/conf.h 2008-12-31 18:04:47.000000000 +1300 @@ -69,6 +69,7 @@ typedef struct __config_t { * downloaded of the total download list */ unsigned short totaldownload; unsigned short cleanmethod; /* select -Sc behavior */ + unsigned short noignoreprompt; /* don't ask user to confirm ignored packages */ alpm_list_t *syncfirst; } config_t; 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")); } } @@ -366,6 +367,7 @@ static int parseargs(int argc, char *arg {"debug", optional_argument, 0, 1003}, {"noprogressbar", no_argument, 0, 1004}, {"noscriptlet", no_argument, 0, 1005}, + {"noignoreprompt", no_argument, 0, 1006}, {"cachedir", required_argument, 0, 1007}, {"asdeps", no_argument, 0, 1008}, {"logfile", required_argument, 0, 1009}, @@ -422,6 +424,7 @@ static int parseargs(int argc, char *arg break; case 1004: config->noprogressbar = 1; break; case 1005: config->flags |= PM_TRANS_FLAG_NOSCRIPTLET; break; + case 1006: config->noignoreprompt = 1; break; case 1007: if(alpm_option_add_cachedir(optarg) != 0) { pm_printf(PM_LOG_ERROR, _("problem adding cachedir '%s' (%s)\n"), @@ -680,6 +683,9 @@ static int _parseconfig(const char *file } else if(strcmp(key, "TotalDownload") == 0) { config->totaldownload = 1; pm_printf(PM_LOG_DEBUG, "config: totaldownload\n"); + } else if(strcmp(key, "NoIgnorePrompt") == 0) { + config->noignoreprompt = 1; + pm_printf(PM_LOG_DEBUG, "config: noignoreprompt\n"); } else { pm_printf(PM_LOG_ERROR, _("config file %s, line %d: directive '%s' not recognized.\n"), file, linenum, key);
On Wed, Dec 31, 2008 at 6:10 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
Hi, attached is a patch to fix bug 9395, against the current git tree for pacman, as described in an earlier email to this list. You may find a description of the changes I made in the Arch bug database for bug 9395. Please let me know if there are any questions.
Thank you, and best wishes, Bryan
diff -rupN -x '*\.git*' pacman/doc/pacman.8.txt pacman-fixbug9395/doc/pacman.8.txt --- pacman/doc/pacman.8.txt 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/doc/pacman.8.txt 2008-12-31 17:52:33.000000000 +1300 @@ -152,6 +152,11 @@ Options If an install scriptlet exists, do not execute it. Do not use this unless you know what you are doing.
+*\--noignoreprompt*:: + Do not prompt the user to ask for confirmation that packages listed in + IgnorePkg and IgnoreGroup really should be ignored; instead, assume + that they should be and silently ignore them. +
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.
Query Options[[QO]] ------------------- diff -rupN -x '*\.git*' pacman/lib/libalpm/alpm.h pacman-fixbug9395/lib/libalpm/alpm.h --- pacman/lib/libalpm/alpm.h 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/alpm.h 2008-12-31 17:59:38.000000000 +1300 @@ -377,7 +377,7 @@ typedef enum _pmtransconv_t { PM_TRANS_CONV_CONFLICT_PKG = 0x04, PM_TRANS_CONV_CORRUPTED_PKG = 0x08, PM_TRANS_CONV_LOCAL_NEWER = 0x10, - /* 0x20 flag can go here */ + PM_TRANS_CONV_REMOVE_PKGS = 0x20, PM_TRANS_CONV_REMOVE_HOLDPKG = 0x40 } pmtransconv_t;
diff -rupN -x '*\.git*' pacman/lib/libalpm/deps.c pacman-fixbug9395/lib/libalpm/deps.c --- pacman/lib/libalpm/deps.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/deps.c 2008-12-31 17:52:41.000000000 +1300 @@ -546,17 +546,92 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *de return(NULL); }
+typedef struct __pkginfo_t +{ + /* The package for which this info is being kept */ + pmpkg_t *pkg; + /* 0 if this package has been determined to be unresolvable, meaning that + it has dependencies that cannot be resolved, nonzero otherwise */ + int unresolvable; + /* 0 if this package was not pulled, nonzero if this package was pulled */ + int pulled; + /* Packages that are immediately dependent on this package. */ + alpm_list_t *dependents; +} pkginfo_t; + +
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. 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.
+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.
int _alpm_dep_edge(pmpkg_t *pkg1, pmpkg_t *pkg2); pmdepend_t *_alpm_splitdep(const char *depstring); pmpkg_t *_alpm_find_dep_satisfier(alpm_list_t *pkgs, pmdepend_t *dep); diff -rupN -x '*\.git*' pacman/lib/libalpm/sync.c pacman-fixbug9395/lib/libalpm/sync.c --- pacman/lib/libalpm/sync.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/lib/libalpm/sync.c 2008-12-31 18:01:17.000000000 +1300 @@ -417,9 +417,10 @@ int _alpm_sync_prepare(pmtrans_t *trans, }
if(!(trans->flags & PM_TRANS_FLAG_NODEPS)) { - /* store a pointer to the last original target so we can tell what was - * pulled by resolvedeps */ - alpm_list_t *pulled = alpm_list_last(list); + /* resolvedeps returns a pointer to the first element of the + * list which is a pulled element, and all elements after that + * are pulled as well */ + alpm_list_t *pulled; /* Resolve targets dependencies */ EVENT(trans, PM_TRANS_EVT_RESOLVEDEPS_START, NULL, NULL); _alpm_log(PM_LOG_DEBUG, "resolving target's dependencies\n"); @@ -432,13 +433,13 @@ int _alpm_sync_prepare(pmtrans_t *trans, } }
- if(_alpm_resolvedeps(db_local, dbs_sync, list, remove, data) == -1) { + if(_alpm_resolvedeps(db_local, dbs_sync, &list, &pulled, remove, data) == -1) { /* pm_errno is set by resolvedeps */ ret = -1; goto cleanup; }
- for(i = pulled->next; i; i = i->next) { + for(i = pulled; i; i = i->next) { pmpkg_t *spkg = i->data; pmsyncpkg_t *sync = _alpm_sync_new(PM_PKG_REASON_DEPEND, spkg, NULL); if(sync == NULL) { diff -rupN -x '*\.git*' pacman/src/pacman/callback.c pacman-fixbug9395/src/pacman/callback.c --- pacman/src/pacman/callback.c 2008-12-31 16:45:31.000000000 +1300 +++ pacman-fixbug9395/src/pacman/callback.c 2008-12-31 18:04:16.000000000 +1300 @@ -248,7 +248,10 @@ void cb_trans_conv(pmtransconv_t event, { switch(event) { case PM_TRANS_CONV_INSTALL_IGNOREPKG: - if(data2) { + if(config->noignoreprompt) { + *response = 0; + } + else if(data2) { /* TODO we take this route based on data2 being not null? WTF */ *response = yesno(_(":: %s requires installing %s from IgnorePkg/IgnoreGroup. Install anyway?"), alpm_pkg_get_name(data2), @@ -274,6 +277,31 @@ void cb_trans_conv(pmtransconv_t event, (char *)data2, (char *)data2); break; + case PM_TRANS_CONV_REMOVE_PKGS: + { + /* Allocate a buffer big enough to hold all of the + package names */ + char *packagenames; + alpm_list_t *unresolved = (alpm_list_t *) data1; + alpm_list_t *i; + int len = 1, /* for trailing \0 */ where = 0, count = 0; + for (i = unresolved; i; i = i->next) { + count += 1; + len += 3 /* for \t, comma, and \n */ + + strlen(alpm_pkg_get_name(i->data)); + } + packagenames = (char *) malloc(len); + for (i = unresolved; i; i = i->next) { + where += snprintf(&(packagenames[where]), len - where, "\t%s%s\n", + alpm_pkg_get_name(i->data), (i->next) ? "," : ""); + } + *response = yesno(_(":: the following package%s cannot be upgraded due to unresolvable " + "dependencies:\n%s\nDo you want to skip %s package%s for this upgrade?"), + (count > 1) ? "s" : "", packagenames, (count > 1) ? "these" : "this", + (count > 1) ? "s" : ""); + free(packagenames); + } + break;
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.
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?
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
On Wed, Dec 31, 2008 at 6:10 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
Hi, attached is a patch to fix bug 9395, against the current git tree for pacman, as described in an earlier email to this list. You may find a description of the changes I made in the Arch bug database for bug 9395. Please let me know if there are any questions.
Another thing, it looks like you are quite able to read and write code :) So in case you didn't lose your motivation for that, you should really have a look at these wiki pages : http://wiki.archlinux.org/index.php/Super_Quick_Git_Guide http://wiki.archlinux.org/index.php/Pacman_Development Using git might be painful in the beginning, but after the initial period, it gets very nice to work with.
Hi, attached is a patch to fix bug 9395, against the current git tree for pacman, as described in an earlier email to this list. You may find a description of the changes I made in the Arch bug database for bug 9395. Please let me know if there are any questions.
Thank you, and best wishes, Bryan
Basically this is a neat (and readable) job imho. Some comments: 1. There is a typo in the description of unresolvable field. 2. A new list for pkginfos looks ugly imho. 3. I like the concept of pulled, maybe (pmpkg_t *) pulledby would be even better, then we could print more informative (error) messages (see FS#12536 for example). So we again reached to a ~graph structure. (Hm. Maybe dependents can be also used instead of pulledby...) 4. It is not easy to determine which package is unresolvable. If a pulled dependency satisfier of foo is unresolvable we may could find an other (resolvable) satisfier. But this is not handled in the current code neither, so your _alpm_mark_unresolvable() is OK. 5. I may overlook something, but as I see, the following situation can happen: # pacman -S foo bar, where foo depends on bar. During resolvedeps foo may become unresolvable, but bar not. In this case pacman asks the user whether he wants to remove foo from target list. User answers yes. Since your _alpm_is_needed code (btw, _alpm prefix is not needed here) doesn't check pulled flag, it will determine that bar is not needed (which is not true, bar was an explicit package), and pacman will also remove bar as well, and the user wasn't informed about this. 6. I think a dependency cycle can lead to an infinite loop due to _alpm_is_needed. Bye
On Wed, Dec 31, 2008 at 2:43 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
Since your _alpm_is_needed code (btw, _alpm prefix is not needed here)
Oh indeed, all the static functions should not have any prefix. Maybe the README file is not perfectly clear about this, but it can be understood implicitly. 26 In a general manner, public library functions are named "alpm_<type>_<action>" 27 (examples: alpm_trans_commit(), alpm_release(), alpm_pkg_get_name(), ...). 28 Internal (and thus private) functions should be named "_alpm_XXX" for instance 29 (examples: _alpm_needbackup(), _alpm_runscriplet(), ...). Functions defined and 30 used inside a single file should be defined as "static".
Nagy Gabor wrote:
Basically this is a neat (and readable) job imho.
Thanks for your kind words and your input. Comments inline below.
Some comments: 1. There is a typo in the description of unresolvable field.
Check, will fix.
2. A new list for pkginfos looks ugly imho.
What do you mean by 'ugly'? Is it redundant given other data structures in libalpm? If so, I will be happy to switch to another data structure. Xavier already pointed out that pmgraph_t may fit the bill. I'll definitely look into it. Or maybe you meant something else by 'ugly'? I'm not sure ...
3. I like the concept of pulled, maybe (pmpkg_t *) pulledby would be even better, then we could print more informative (error) messages (see FS#12536 for example). So we again reached to a ~graph structure. (Hm. Maybe dependents can be also used instead of pulledby...)
This sounds like a good idea. But it would involve greater changes to libalpm then I felt comfortable doing. Unfortunately, when one is just a patch contributor rather than a real developer, one feels the need to keep all changes as minimal as possible, instead of reaching into lots of parts of the code and doing cleanups and refactoring and such. But I would be happy to do some of that, once I feel more comfortable with the code base.
4. It is not easy to determine which package is unresolvable. If a pulled dependency satisfier of foo is unresolvable we may could find an other (resolvable) satisfier. But this is not handled in the current code neither, so your _alpm_mark_unresolvable() is OK.
I see what you mean. Is this something that should/could be fixed in _alpm_resolvedeps? If so, perhaps it can be deferred to a separate bug incident and fixed separately from my change?
5. I may overlook something, but as I see, the following situation can happen: # pacman -S foo bar, where foo depends on bar. During resolvedeps foo may become unresolvable, but bar not. In this case pacman asks the user whether he wants to remove foo from target list. User answers yes. Since your _alpm_is_needed code (btw, _alpm prefix is not needed here) doesn't check pulled flag, it will determine that bar is not needed (which is not true, bar was an explicit package), and pacman will also remove bar as well, and the user wasn't informed about this.
I will remove _alpm prefix from my static functions, I hadn't read the code guidelines carefully enough to realize the convention, mea culpa. As to your problem situation: good catch, I missed that case in my thought process and in testing. Thanks for pointing it out, I will fix it (should be an easy fix in _alpm_is_needed).
6. I think a dependency cycle can lead to an infinite loop due to _alpm_is_needed.
I didn't realize that dependency cycles were possible. But you are right, they would lead to an infinite loop. I'll have to figure out some way to detect and break such cycles. Thanks for pointing that out to me. Best wishes, Bryan
On Wed, Dec 31, 2008 at 2:55 PM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
I didn't realize that dependency cycles were possible. But you are right, they would lead to an infinite loop. I'll have to figure out some way to detect and break such cycles. Thanks for pointing that out to me.
We have a few cycles in core alone. Pacman deals with it elegantly enough, in that it just kinda decides on an order and warns us "A is being installed before its dependency B". If you want to see this in action, try: $ pacman -r some-dir/ -Sy base You should get 2 or 3, mainly relating to things like glibc and bash
2. A new list for pkginfos looks ugly imho.
What do you mean by 'ugly'? Is it redundant given other data structures in libalpm? If so, I will be happy to switch to another data structure. Xavier already pointed out that pmgraph_t may fit the bill. I'll definitely look into it. Or maybe you meant something else by 'ugly'? I'm not sure ...
Well, this is subjective. I don't like these "_alpm_findinfo()" calls and double (list) bookkeeping. However, after thinking a bit I have no better idea atm [probably in the future we should introduce a "transaction package" type (successor of pmsyncpkg_t) to store internal data (including delta_path etc.)], and this is a minor issue (we can rework this part easily later, if needed), so I revoke my complaint ;-) Bye
participants (4)
-
Aaron Griffin
-
Bryan Ischo
-
Nagy Gabor
-
Xavier