[pacman-dev] [PATCH] Fix bug 9395 by allowing pacman to remove unresolvable packages from a transaction
Hi, forgive me if I'm doing this wrong, I'm new to git and this is the first time I've tried to supply a git patch to this list. My patch is attached to this email - is that the correct way to send it to the list? Some notes on this patch: - It is based on the current git sources instead of the release version of the code as was my previous patch on this same topic - It takes into consideration the following feedback from the list on my previous patch: - Fixed static function names (static void _alpm_xxx -> static void xxx) - Removed the prompt that asks the user if they want to un-ignore packages that have upgrades for the current transaction. The reasoning is that if the user didn't want to ignore the package, they wouldn't have put them in their IgnorePkg list, so why ask them during an upgrade? Instead just expect the user to remove packages from IgnorePkg that they don't actually want to ignore. - Removed the NoIgnorePrompt configuration option that I had previously added since instead I'm just removing that prompt altogether - Fixed the issue where if a top-level package is depended upon by unresolvable top-level packages, it would be removed from the transaction, whereas really it should be retained - Fixed the infinite loop when there are dependency loops - Added a few new test cases to pactest: ignore001,ignore002, and ignore003, which test some new ignore package functionality - Changed the expected results of some existing test cases (provision020, provision022, sync021, sync1008, and sync300) because with the new functionality, pacman succeeds in cases where it used to fail (simply ignoring packages which cannot be upgraded instead of issuing an error) even though the results are the same, and also, because the new behavior means that there is no way to sync a group that has a package that is ignored in it (it used to work if the user said 'yes' to the prompt that I have removed, now there is no such prompt and thus no opportunity to un-ignore the offending package) - And one VERY IMPORTANT note: this change means that the release version of libalpm needs to be increased, so it should be libalpm.so.4 instead of libalpm.so.3. This is because a new callback prompt was added and an old one removed. The latter shouldn't affect existing front-end (they just won't get asked a question that they used to get asked), but the former will likely break any front-end by sending a callback with a new "Transaction Conversation" identifier. Comments welcome! Thanks, Bryan From 4a9236bb54031eb4f447279ce40d86d0c6db7da5 Mon Sep 17 00:00:00 2001 From: Bryan Ischo <bryan@ischo.com> Date: Mon, 12 Jan 2009 17:05:02 +1300 Subject: [PATCH] Changed behavior of transactions to optionally remove packages which cannot be resolved This fixes incident 9395, by providing for a call-out from the transaction resolve step to the front-end, asking the user if they would like to ignore any packages which could not be upgraded due to unresolvable dependencies. In most cases, such dependencies are due to packages in IgnorePkg/IgnoreGroup on which packages to upgrade depend. Simply removing the offending packages from the transaction rather than failing the transaction allows such transactions to proceed and do the work that can be done. This makes managing packages with IgnorePkg/IgnoreGroup much easier. Also removed the prompt which asks the user if they'd like to not ignore packages in IgnorePkg/IgnoreGroup, because this is an unnecessary prompt - if the user wants to do this, they can do it in their /etc/pacman.conf, they don't need pacman to prompt them about it every time they do an upgrade. Finally, changed the expected behavior of some tests, to match the new transaction behavior. Signed-off-by: Bryan Ischo <bryan@ischo.com> --- lib/libalpm/alpm.h | 4 +- lib/libalpm/deps.c | 259 +++++++++++++++++++++++++++++++++++------ lib/libalpm/deps.h | 4 +- lib/libalpm/sync.c | 11 +- pactest/README | 14 +-- pactest/tests/ignore001.py | 17 +++ pactest/tests/ignore002.py | 35 ++++++ pactest/tests/ignore003.py | 35 ++++++ pactest/tests/provision020.py | 2 +- pactest/tests/provision022.py | 2 +- pactest/tests/sync021.py | 4 +- pactest/tests/sync1008.py | 2 +- pactest/tests/sync300.py | 2 +- src/pacman/callback.c | 36 ++++-- 14 files changed, 356 insertions(+), 71 deletions(-) create mode 100644 pactest/tests/ignore001.py create mode 100644 pactest/tests/ignore002.py create mode 100644 pactest/tests/ignore003.py diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index eda35d3..68d99f7 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -372,12 +372,12 @@ typedef enum _pmtransevt_t { /* Transaction Conversations (ie, questions) */ typedef enum _pmtransconv_t { - PM_TRANS_CONV_INSTALL_IGNOREPKG = 0x01, + /* 0x01 is available */ PM_TRANS_CONV_REPLACE_PKG = 0x02, 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 --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 96c971a..3abac06 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -513,12 +513,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, alpm_list_t *exclud pmpkg_t *pkg = _alpm_db_get_pkgfromcache(i->data, dep->name); if(pkg && alpm_depcmp(pkg, dep) && !_alpm_pkg_find(excluding, pkg->name)) { if(_alpm_pkg_should_ignore(pkg)) { - int install; - QUESTION(handle->trans, PM_TRANS_CONV_INSTALL_IGNOREPKG, pkg, - tpkg, NULL, &install); - if(!install) { - continue; - } + continue; } return(pkg); } @@ -530,12 +525,7 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, alpm_list_t *exclud if(alpm_depcmp(pkg, dep) && strcmp(pkg->name, dep->name) && !_alpm_pkg_find(excluding, pkg->name)) { if(_alpm_pkg_should_ignore(pkg)) { - int install; - QUESTION(handle->trans, PM_TRANS_CONV_INSTALL_IGNOREPKG, pkg, - tpkg, NULL, &install); - if(!install) { - continue; - } + continue; } _alpm_log(PM_LOG_WARNING, _("provider package was selected (%s provides %s)\n"), pkg->name, dep->name); @@ -546,17 +536,108 @@ pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, alpm_list_t *exclud 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; + /* This marker is used to detect when a dependency cycle exists */ + int marker; +} pkginfo_t; + + +static pkginfo_t *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 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) { + mark_unresolvable(list, findinfo(list, ((pmpkg_t *) i->data))); + } +} + +static void info_free(pkginfo_t *info) +{ + alpm_list_free(info->dependents); + free(info); +} + + +static int is_needed(alpm_list_t *infolist, pkginfo_t *info, int marker) +{ + if (info->unresolvable) { + /* Obviously if it's already been marked unresolvable, it is not + needed */ + return(0); + } else if (!info->pulled) { + /* If it's top-level (not pulled), then it's needed */ + return(1); + } else { + /* Now, if all of the top-level packages which depend on it are + unresolvable, then it is unneeded */ + alpm_list_t *i; + for (i = info->dependents; i; i = i->next) { + pmpkg_t *deppkg = (pmpkg_t *) i->data; + if (info->marker == marker) { + /* This means that a dependency loop has been detected; we've + already marked this package meaning that it's already been + seen this time through. So ignore this dependency. */ + continue; + } + + info->marker = marker; + + if (!is_needed(infolist, findinfo(infolist, deppkg), marker)) { + 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 marker = 0; + 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 +646,30 @@ int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, alpm_list_t *list, } _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; + thisinfo->marker = 0; + 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 = 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 +677,122 @@ int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, alpm_list_t *list, 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 = findinfo(info, spkg); + if (sinfo->unresolvable) { + 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 */ + 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 (!is_needed(info, thisinfo, ++marker)) { + *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 = 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)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 --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h index 2f3c450..f281452 100644 --- a/lib/libalpm/deps.h +++ b/lib/libalpm/deps.h @@ -48,8 +48,8 @@ void _alpm_depmiss_free(pmdepmissing_t *miss); 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 --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index b458874..5741262 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -417,9 +417,10 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync } 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, pmdb_t *db_local, alpm_list_t *dbs_sync } } - 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 --git a/pactest/README b/pactest/README index 5d4e47f..c7d00f7 100644 --- a/pactest/README +++ b/pactest/README @@ -70,12 +70,12 @@ Usage pactest will run the suite of tests defined by the "--test" parameter. Example: - ./pactest.py --test=test/* + ./pactest.py --test tests/*.py -This example will run tests from the "test" directory. +This example will run all tests from the "tests" directory. Note: several "--test" options can be passed to pactest. -Use the ""help" option to get the full list of parameters: +Use the "help" option to get the full list of parameters: ./pactest.py --help @@ -103,15 +103,11 @@ Example: ------ A dictionary that holds the data used in the pacman configuration file. -It has 3 keys, each one of them pointing at a list of strings: - - noupgrade - - noextract - - ignorepkg Examples: - self.option["noupgrade"] = ["etc/X11/xorg.conf", + self.option["NoUpgrade"] = ["etc/X11/xorg.conf", "etc/pacman.conf"] - self.option["noextract"] = ["etc/lilo.conf"] + self.option["NoExtract"] = ["etc/lilo.conf"] filesystem ---------- diff --git a/pactest/tests/ignore001.py b/pactest/tests/ignore001.py new file mode 100644 index 0000000..bf8e8c6 --- /dev/null +++ b/pactest/tests/ignore001.py @@ -0,0 +1,17 @@ +self.description = "Sync with irrelevent ignored packages" + +package1 = pmpkg("package1") +self.addpkg2db("local", package1) + +package2 = pmpkg("package2") +self.addpkg2db("local", package2) + +package2up = pmpkg("package2", "2.0-1") +self.addpkg2db("sync", package2up) + +self.option["IgnorePkg"] = ["irrelevent"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.0-1") +self.addrule("PKG_VERSION=package2|2.0-1") diff --git a/pactest/tests/ignore002.py b/pactest/tests/ignore002.py new file mode 100644 index 0000000..b64b70a --- /dev/null +++ b/pactest/tests/ignore002.py @@ -0,0 +1,35 @@ +self.description = "Sync with relevent ignored packages" + +package1 = pmpkg("package1") +self.addpkg2db("local", package1) + +package2 = pmpkg("package2") +self.addpkg2db("local", package2) + +package3 = pmpkg("package3") +package3.depends = ["package2=1.0-1"] +self.addpkg2db("local", package3) + +package4 = pmpkg("package4") +package4.depends = ["package3=1.0-1"] +self.addpkg2db("local", package4) + +package2up = pmpkg("package2", "2.0-1") +self.addpkg2db("sync", package2up) + +package3up = pmpkg("package3", "2.0-1") +package3up.depends = ["package2=2.0-1"] +self.addpkg2db("sync", package3up) + +package4up = pmpkg("package4", "2.0-1") +package4up.depends = ["package3=2.0-1"] +self.addpkg2db("sync", package4up) + +self.option["IgnorePkg"] = ["package2"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.0-1") +self.addrule("PKG_VERSION=package2|1.0-1") +self.addrule("PKG_VERSION=package3|1.0-1") +self.addrule("PKG_VERSION=package4|1.0-1") diff --git a/pactest/tests/ignore003.py b/pactest/tests/ignore003.py new file mode 100644 index 0000000..f7c1658 --- /dev/null +++ b/pactest/tests/ignore003.py @@ -0,0 +1,35 @@ +self.description = "Sync with relevent ignored packages and dependency loop" + +package1 = pmpkg("package1") +self.addpkg2db("local", package1) + +package2 = pmpkg("package2") +self.addpkg2db("local", package2) + +package3 = pmpkg("package3") +package3.depends = ["package2=1.0-1"] +self.addpkg2db("local", package3) + +package4 = pmpkg("package4") +package4.depends = ["package3=1.0-1"] +self.addpkg2db("local", package4) + +package2up = pmpkg("package2", "2.0-1") +self.addpkg2db("sync", package2up) + +package3up = pmpkg("package3", "2.0-1") +package3up.depends = ["package2=2.0-1", "package4=2.0-1"] +self.addpkg2db("sync", package3up) + +package4up = pmpkg("package4", "2.0-1") +package4up.depends = ["package3=2.0-1"] +self.addpkg2db("sync", package4up) + +self.option["IgnorePkg"] = ["package2"] +self.args = "-Su" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_VERSION=package1|1.0-1") +self.addrule("PKG_VERSION=package2|1.0-1") +self.addrule("PKG_VERSION=package3|1.0-1") +self.addrule("PKG_VERSION=package4|1.0-1") diff --git a/pactest/tests/provision020.py b/pactest/tests/provision020.py index 7cb0a01..c9c0ac3 100644 --- a/pactest/tests/provision020.py +++ b/pactest/tests/provision020.py @@ -10,6 +10,6 @@ self.args = "-S %s" % p.name -self.addrule("PACMAN_RETCODE=1") +self.addrule("PACMAN_RETCODE=0") self.addrule("!PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") diff --git a/pactest/tests/provision022.py b/pactest/tests/provision022.py index 4883d42..190a8b6 100644 --- a/pactest/tests/provision022.py +++ b/pactest/tests/provision022.py @@ -10,6 +10,6 @@ self.args = "-S %s" % p.name -self.addrule("PACMAN_RETCODE=1") +self.addrule("PACMAN_RETCODE=0") self.addrule("!PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") diff --git a/pactest/tests/sync021.py b/pactest/tests/sync021.py index 4c664d8..bb1c277 100644 --- a/pactest/tests/sync021.py +++ b/pactest/tests/sync021.py @@ -16,6 +16,6 @@ self.args = "-S grp" -self.addrule("PACMAN_RETCODE=0") +self.addrule("!PACMAN_RETCODE=0") for p in sp1, sp2, sp3: - self.addrule("PKG_EXIST=%s" % p.name) + self.addrule("!PKG_EXIST=%s" % p.name) diff --git a/pactest/tests/sync1008.py b/pactest/tests/sync1008.py index a606459..90c61df 100644 --- a/pactest/tests/sync1008.py +++ b/pactest/tests/sync1008.py @@ -14,6 +14,6 @@ self.args = "-S pkg" -self.addrule("PACMAN_RETCODE=1") +self.addrule("PACMAN_RETCODE=0") self.addrule("!PKG_EXIST=pkg") self.addrule("!PKG_EXIST=cpkg") diff --git a/pactest/tests/sync300.py b/pactest/tests/sync300.py index 31b520a..36d6758 100644 --- a/pactest/tests/sync300.py +++ b/pactest/tests/sync300.py @@ -9,6 +9,6 @@ self.args = "-S %s" % sp1.name -self.addrule("PACMAN_RETCODE=1") +self.addrule("PACMAN_RETCODE=0") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 9f1cca8..e478bf5 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -247,17 +247,6 @@ void cb_trans_conv(pmtransconv_t event, void *data1, void *data2, void *data3, int *response) { switch(event) { - case PM_TRANS_CONV_INSTALL_IGNOREPKG: - 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), - alpm_pkg_get_name(data1)); - } else { - *response = yesno(_(":: %s is in IgnorePkg/IgnoreGroup. Install anyway?"), - alpm_pkg_get_name(data1)); - } - break; case PM_TRANS_CONV_REMOVE_HOLDPKG: *response = yesno(_(":: %s is designated as a HoldPkg. Remove anyway?"), alpm_pkg_get_name(data1)); @@ -274,6 +263,31 @@ void cb_trans_conv(pmtransconv_t event, void *data1, void *data2, (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?"), -- 1.6.1
Hi! Some comments/suggestions. (Note: First wait for Dan's response, whether he can support your concept (behavior change) or not. I would be sad, if you did useless work because of me.) I must repeat some of my earlier suggestions. We prefer shorter patches. I can fully support in your patch: dropping PM_TRANS_CONV_INSTALL_IGNOREPKG, I don't know why we haven't done this earlier. But: I think we should give a warning where we asked the user earlier. For example, this part could be factorized into a seperate patch.
+static int is_needed(alpm_list_t *infolist, pkginfo_t *info, int marker) +{ + if (info->unresolvable) { + /* Obviously if it's already been marked unresolvable, it is not + needed */ + return(0); + } else if (!info->pulled) { + /* If it's top-level (not pulled), then it's needed */ + return(1); + } else { + /* Now, if all of the top-level packages which depend on it are + unresolvable, then it is unneeded */ + alpm_list_t *i; + for (i = info->dependents; i; i = i->next) { + pmpkg_t *deppkg = (pmpkg_t *) i->data; + if (info->marker == marker) { + /* This means that a dependency loop has been detected; we've + already marked this package meaning that it's already been + seen this time through. So ignore this dependency. */ + continue; + } + + info->marker = marker; + + if (!is_needed(infolist, findinfo(infolist, deppkg), marker)) { + return(0); + }
???
+ } + + return(1);
??? I don't understand this part. (I checked, your earlier patch also had this part.) Probably want to do this: If any dependent is needed, then this package is also needed, otherwise not.
+ } +}
+ 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));
2. The result of this: error: cannot resolve dependencies for: error: foo error: bar ... This is ugly, we use data list for this purpose, the front-end should build this message.
+ if (data) { + alpm_list_t *targ = alpm_list_add(NULL, i->data); + deps = alpm_checkdeps(_alpm_db_get_pkgcache(local), 0, remove, targ);
This is not OK. (If this part existed in your previous patch, sorry for missing it.) In the target list we may have a satisfier.
+ alpm_list_free(targ); + } + }
Nagy Gabor wrote:
Hi!
Some comments/suggestions. (Note: First wait for Dan's response, whether he can support your concept (behavior change) or not. I would be sad, if you did useless work because of me.)
I must repeat some of my earlier suggestions.
We prefer shorter patches. I can fully support in your patch: dropping PM_TRANS_CONV_INSTALL_IGNOREPKG, I don't know why we haven't done this earlier. But: I think we should give a warning where we asked the user earlier. For example, this part could be factorized into a seperate patch.
OK, I can separate that into a different patch if that is required to get the change in. One reason that I thought it made sense to make these changes in one patch is that the new prompt I have added to some degree obsoletes the old prompt: whereas a user used to be asked if they want to un-ignore packages which have updates, the new prompt doesn't ask them this but does as them if they want to remove all packages from the transaction which cannot be updated due to packages being ignored (although it doesn't explicitly state which packages were ignored - something I'd like to do, but in a future checkin to improve the prompt). In both cases, they become aware that some packages are not going to be updated due to being in the ignore list, but in the prior code, they were asked whether to let the transaction proceed by un-ignoring these packages, in the new code they are asked whether to let the transaction proceed by removing all dependencies on ignored packages from the transaction. Because both prompts deal with ignore packages, but in different ways, I thought of the new prompt as a bit of a "replacement" for the old prompt, and thus include the removal of the old prompt in this change, rather than doing it in a later change, which would, in the interim code between the changes, have a "double-prompt". In terms of a warning, pacman already prints out lines like this: warning: kernel26: ignoring package upgrade (2.6.27.8-1 => 2.6.27.10-1) warning: pacman: ignoring package upgrade (3.2.1FIXBUG9395-1 => 3.2.1-2) warning: pango: ignoring package upgrade (1.22.3-2 => 1.22.4-1) Is that not sufficient?
+static int is_needed(alpm_list_t *infolist, pkginfo_t *info, int marker) +{ + if (info->unresolvable) { + /* Obviously if it's already been marked unresolvable, it is not + needed */ + return(0); + } else if (!info->pulled) { + /* If it's top-level (not pulled), then it's needed */ + return(1); + } else { + /* Now, if all of the top-level packages which depend on it are + unresolvable, then it is unneeded */ + alpm_list_t *i; + for (i = info->dependents; i; i = i->next) { + pmpkg_t *deppkg = (pmpkg_t *) i->data; + if (info->marker == marker) { + /* This means that a dependency loop has been detected; we've + already marked this package meaning that it's already been + seen this time through. So ignore this dependency. */ + continue; + } + + info->marker = marker; + + if (!is_needed(infolist, findinfo(infolist, deppkg), marker)) { + return(0); + }
???
+ } + + return(1);
??? I don't understand this part. (I checked, your earlier patch also had this part.) Probably want to do this: If any dependent is needed, then this package is also needed, otherwise not.
It's more complicated than that. This function might not be very well named. It's purpose is to determine which packages are to be removed from the transaction because they either were unresolvable, or were resolvable but are only in the transaction because they were pulled by a package that was unresolvable. This function is run only after the user has been asked about the top-level packages which were unresolvable (i.e. the packages they explicitly requested be updated, but which had dependencies which could not be resolved). If they say that they want these packages removed from the transaction, then the code has to remove not just those packages, but also any dependencies which were pulled only to satisfy dependencies of the packages being removed from the transaction (and so on recursively). It determines which packages to be removed using these tests: - If the package was marked unresolvable, then it must be removed - Else, if the package was top-level (i.e. not pulled, i.e. explicitly requested by the user), then it is NOT removed (since it was already just determined to be resolvable) - Else, it is a pulled package which was not resolvable. But maybe it was pulled only to satisfy dependencies of packages which later on turned out to be unresolvable themselves. Just because the pulled package has one or more unresolvable "immediate" dependents, does not mean that it needs to be removed (because some other dependents may be resolvable and retained in the transaction, thus this package should be too). And just because the pulled package has all resolvable "immediate" dependents, does not mean that it should be retained (because maybe all dependents of its dependents are going to be removed from the transaction, in which case this package should be too). The simplest question to ask about such a package is, "is there any path back through the dependents chain from this package to a top-level package that will be retained in this transaction". In other words, are any top-level dependent "ancestors" of this package needed? If so, then this package is needed too. Otherwise, this package is not needed; we can visualize this as the entire set of subtrees rooted at the top-level packages and which include the package that we are testing, as being unresolvable and thus needing to be removed from the transaction. So the test we finally do is recursively ask each dependent if it is needed (which will only recursively stop at the top-level packages), and if any are, then this package is needed too. HOWEVER, in writing this very last line, I realize that my final test should be: if (is_needed(infolist, findinfo(infolist, deppkg), marker)) { return(1); } This is because we want to consider a package needed if any of its dependent packages are needed. The code as it was was mistakenly considering a package needed only if all of its dependent packages are needed. Does that make sense?
+ } +}
+ 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));
2. The result of this: error: cannot resolve dependencies for: error: foo error: bar ...
This is ugly, we use data list for this purpose, the front-end should build this message.
Please tell me exactly the format you want to see this error message in, and I will make it so. I don't know exactly what you are expecting so I am not sure how to fix the problem you are pointing out.
+ if (data) { + alpm_list_t *targ = alpm_list_add(NULL, i->data); + deps = alpm_checkdeps(_alpm_db_get_pkgcache(local), 0, remove, targ);
This is not OK. (If this part existed in your previous patch, sorry for missing it.) In the target list we may have a satisfier.
I don't understand what you are saying here. A few points: 1. The code already looked like this; I haven't changed how the dependencies are checked in my patch. So what you are objecting to is, I think, already the way that this code worked, not a change that I have made. 2. I think you already talked about this in a previous email, when you said:
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.
3. alpm_checkdeps just returns the list of dependencies for the package. The very first thing that the code then does with each dependency is look for it in the target list. Then if it's not found, it uses _alpm_resolvedep to try to find a satisfier. Here is the code in question: Thanks for your comments. I hope that we're getting closer to getting my changes in :) Bryan
Hey all. Just a minor correction to something I wrote, which I want to correct because the code in question is confusing and errors in discussion can only lead to more confusion. When I wrote: Bryan Ischo wrote:
It determines which packages to be removed using these tests:
- If the package was marked unresolvable, then it must be removed - Else, if the package was top-level (i.e. not pulled, i.e. explicitly requested by the user), then it is NOT removed (since it was already just determined to be resolvable) - Else, it is a pulled package which was not resolvable.
I should have written, in the last part, "Else, it is a pulled package which IS resolvable". Also, sorry for any poor formatting in my emails. I am using Mozilla Thunderbird and I'm finding its compose window to be pretty terrible. I only send my emails in plain text format, I have the option set this way, so I don't know why it makes my editor use non-fixed-width-plain-text format. It's hard to tell what my email is actually going to look like, and Thunderbird's "graphical" handling of quotes is very difficult to work with. Does anyone know how to make Thunderbird use just plain text in its editing window, including standard quote marks (>) instead of this graphical nonsense? Thanks, Bryan
[Bryan, sorry for my previous direct reply.]
In terms of a warning, pacman already prints out lines like this:
warning: kernel26: ignoring package upgrade (2.6.27.8-1 => 2.6.27.10-1) warning: pacman: ignoring package upgrade (3.2.1FIXBUG9395-1 => 3.2.1-2) warning: pango: ignoring package upgrade (1.22.3-2 => 1.22.4-1)
Is that not sufficient?
No. If you check pacman/callback.c, you will see that PM_TRANS_CONV_INSTALL_IGNOREPKG was used for asking the following questions: ":: %s requires installing %s from IgnorePkg/IgnoreGroup. Install anyway?" (by resolvedeps) ":: %s is in IgnorePkg/IgnoreGroup. Install anyway?" (Iirc, after "pacman -S ignoredpkg"). These differ from sysupgrade (-Su) messages.
HOWEVER, in writing this very last line, I realize that my final test should be:
if (is_needed(infolist, findinfo(infolist, deppkg), marker)) { return(1); }
This is because we want to consider a package needed if any of its dependent packages are needed. The code as it was was mistakenly considering a package needed only if all of its dependent packages are needed.
Does that make sense?
Yes. And probably you want to change the final ("fall-back") return(1) to return(0) in the function.
2. The result of this: error: cannot resolve dependencies for: error: foo error: bar ...
This is ugly, we use data list for this purpose, the front-end should build this message.
Please tell me exactly the format you want to see this error message in, and I will make it so. I don't know exactly what you are expecting so I am not sure how to fix the problem you are pointing out.
I guess we shouldn't print anything in alpm. Probably in the front-end we will check pm_errno and print the correct error message using the data list. I know that we had an error message here earlier, but I don't like these redundant error messages. (Front-end always calls _alpm_strerrorlast(), and processes data list).
+ if (data) { + alpm_list_t *targ = alpm_list_add(NULL, i->data); + deps = alpm_checkdeps(_alpm_db_get_pkgcache(local), 0, remove, targ);
This is not OK. (If this part existed in your previous patch, sorry for missing it.) In the target list we may have a satisfier.
I don't understand what you are saying here. A few points:
1. The code already looked like this; I haven't changed how the dependencies are checked in my patch. So what you are objecting to is, I think, already the way that this code worked, not a change that I have made. 2. I think you already talked about this in a previous email, when you said:
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.
3. alpm_checkdeps just returns the list of dependencies for the package. The very first thing that the code then does with each dependency is look for it in the target list. Then if it's not found, it uses _alpm_resolvedep to try to find a satisfier. Here is the code in question:
No. pkg->depends or alpm_pkg_get_depends(pkg) is used to get the dependency list of pkg. In the old code it was easy to determine if a dependency is broken. If we couldn't find a satisfier package in the target list or in the local database (which won't be upgraded), we simply detected that this dependency is unresolvable. (However, our error message was often uninformative, user may got an error message like this: "Cannot resolve foo dependency of pulledpkg". User asked: Pulledpkg? What? I just did "pacman -S bar". [1]) However, your patch makes things more complicated. We start to resolve foo dependency of bar, which may be satisfied. But *later* it may turn out, that we cannot resolve completely the _pulled_ foo satisfier, so we simply remove it from the list. Now it would be elegant if we could say: We could not resolve foo dependency of bar. (At this point we can clearly see, that the old method doesn't work, because *locally* foo is a resolvable dependency.) But it is not so easy to detect, probably we need a more sophisticated info (graph?) structure, and imho we can catch this broken dependency when we "label" the package as unresolvable. If we solve this problem, problem [1] will also disappear, which is an extra benefit. Bye ------------------------------------------------------ SZTE Egyetemi Konyvtar - http://www.bibl.u-szeged.hu This message was sent using IMP: http://horde.org/imp/
No. pkg->depends or alpm_pkg_get_depends(pkg) is used to get the dependency list of pkg. In the old code it was easy to determine if a dependency is broken. If we couldn't find a satisfier package in the target list or in the local database (which won't be upgraded), we simply detected that this dependency is unresolvable. (However, our error message was often uninformative, user may got an error message like this: "Cannot resolve foo dependency of pulledpkg". User asked: Pulledpkg? What? I just did "pacman -S bar". [1])
However, your patch makes things more complicated. We start to resolve foo dependency of bar, which may be satisfied. But *later* it may turn out, that we cannot resolve completely the _pulled_ foo satisfier, so we simply remove it from the list. Now it would be elegant if we could say: We could not resolve foo dependency of bar. (At this point we can clearly see, that the old method doesn't work, because *locally* foo is a resolvable dependency.) But it is not so easy to detect, probably we need a more sophisticated info (graph?) structure, and imho we can catch this broken dependency when we "label" the package as unresolvable.
If we solve this problem, problem [1] will also disappear, which is an extra benefit.
Bye
Oh, I see that I totally misinterpreted something here. If we allow pacman to remove targets from the list, we cannot get any unresolved dependencies, right? So just forget what I said earlier. (But problem [1] is still interesting.) But your method for collecting unresolvable dependencies is still not good. Just put the dependency into data list when it turns out that the dependency (locally) not resolvable. (When resolvedep returns with NULL.) This should be identical to the old behavior, if user doesn't let pacman remove targets. If user resolve this problem by removing some targets, you can free this list. Bye ------------------------------------------------------ SZTE Egyetemi Konyvtar - http://www.bibl.u-szeged.hu This message was sent using IMP: http://horde.org/imp/
Nagy Gabor wrote:
Oh, I see that I totally misinterpreted something here. If we allow pacman to remove targets from the list, we cannot get any unresolved dependencies, right? So just forget what I said earlier. (But problem [1] is still interesting.) But your method for collecting unresolvable dependencies is still not good. Just put the dependency into data list when it turns out that the dependency (locally) not resolvable. (When resolvedep returns with NULL.) This should be identical to the old behavior, if user doesn't let pacman remove targets. If user resolve this problem by removing some targets, you can free this list.
Yes, the technique that I use removes all unresolvable dependencies from the list altogether; the effect is as if the user hadn't actually requested that any packages be upgraded which could not be resolved, and so the remaining list is 'clean' of unresolvable dependencies. I don't think the simplification you propose will work. What if we have: packageA-1.0-1 depends on packageB-1.0-1 packageB-1.01 depends on packageC-1.0-1 User has packageC in their IgnorePkg list. User does pacman -S packageA and new packages are available: packageA-2.0-1 depends on packageB-2.0-1 packageB-2.0-1 depends on packageC-2.0-1 The resolve for packageA looks for packageB-2.0-1 and finds it. The resolve for packageB-2.0-1 looks for packageC-2.0-1 but *does not* find it because packageC is in the IgnorePkg list. Now with my change, the resolve step will determine that packageA and packageB cannot be installed due to missing dependencies, and will prompt the user to ask if they want to remove packageA from the transaction. If they say that they do, it removes packageA and all packages which it depends on (except those packages which are also depended on by packages that are remaining in the transaction, which in this example, is none), which is packageA and packageB. If they say they do not, then it returns an error in the same way that the pre-existing code returned an error at the moment that the unresolvable packages was detected. What you are suggesting, I think, is that we simply put packageC on a separate list of unresolvable packages, and then at the end, ask the user some question (I'm not sure exactly what question since you're not keeping track of which top-level packages are unresolvable - probably you'd ask "packageC is not resolvable, remove from transaction?", which I think would be confusing, since the user never said anything about packageC), and if they want to remove unresolvable packages, just remove packageC from the transaction. However, you're then left with a transaction including packageA and packageB, which *cannot* be completed, because packageB has an unresolvable dependency. Did I understand your suggestion correctly? Let me try to explain the algorithm and data structures I am using, it may help clear up confusion. But first, here's the old algorithm: - While resolving dependencies, if a required package is found to be in IgnorePkg, then prompt the user to see if they want to include the package in this transaction anyway - If they did not, then consider the dependency to be unresolvable - If they did, then put the package in the transaction and proceed as if it wasn't ever in IgnorePkg - Any unresolvable package immediately fails the resolve step and returns 1 Now here's my algorithm: - While resolving dependencies, if a required package is found to be in IgnorePkg, or otherwise cannot be found, then keep this information in an internal dependency graph but don't ask the user anything, just proceed to the end of the resolve step - At the end of the resolve step, if any packages were not resolvable, compute which top-level packages were unresolvable (which could be due to any package in their dependency subtree being unresolvable) - Ask the user if they want to remove these unresolvable top-level packages from the transaction - If they do, remove the unresolvable top-level packages and all of their dependencies from the transaction (except for those dependencies which were also required by a resolvable top-level package too), and return a success code from the resolve step - If they do not, fail the resolve step by returning 1 The data structures are: - Well there's only one really. It's a structure holding the "back-pointers" from packages to the packages which depend on them. It also holds a few values which are computed during the resolve operation. It's possible that what I'm trying to do could be done in a simpler way; but I don't think that your suggestion accomplishes that. I think the part you might be missing is the case where there is a deep dependency chain and only the bottom package cannot be resolved; there has to be some way to detect that all packages which depend on this package also cannot be resolved. Thanks, Bryan
Nagy Gabor wrote:
In terms of a warning, pacman already prints out lines like this:
warning: kernel26: ignoring package upgrade (2.6.27.8-1 => 2.6.27.10-1) warning: pacman: ignoring package upgrade (3.2.1FIXBUG9395-1 => 3.2.1-2) warning: pango: ignoring package upgrade (1.22.3-2 => 1.22.4-1)
Is that not sufficient?
No. If you check pacman/callback.c, you will see that PM_TRANS_CONV_INSTALL_IGNOREPKG was used for asking the following questions: ":: %s requires installing %s from IgnorePkg/IgnoreGroup. Install anyway?" (by resolvedeps) ":: %s is in IgnorePkg/IgnoreGroup. Install anyway?" (Iirc, after "pacman -S ignoredpkg").
These differ from sysupgrade (-Su) messages.
OK, I agree with you - I will put the prompt back for the second case.
HOWEVER, in writing this very last line, I realize that my final test should be:
if (is_needed(infolist, findinfo(infolist, deppkg), marker)) { return(1); }
This is because we want to consider a package needed if any of its dependent packages are needed. The code as it was was mistakenly considering a package needed only if all of its dependent packages are needed.
Does that make sense?
Yes. And probably you want to change the final ("fall-back") return(1) to return(0) in the function.
Yes indeed! This was caught by one of the new test cases I added and I fixed it already :)
2. The result of this: error: cannot resolve dependencies for: error: foo error: bar ...
This is ugly, we use data list for this purpose, the front-end should build this message.
Please tell me exactly the format you want to see this error message in, and I will make it so. I don't know exactly what you are expecting so I am not sure how to fix the problem you are pointing out.
I guess we shouldn't print anything in alpm. Probably in the front-end we will check pm_errno and print the correct error message using the data list. I know that we had an error message here earlier, but I don't like these redundant error messages. (Front-end always calls _alpm_strerrorlast(), and processes data list).
OK, I'm very happy to remove that error logging. I don't like it either, I was just trying to match what I thought the standard behavior was for these functions, which do seem to log errors in such cases, but I absolutely don't need or want to do it ...
No. pkg->depends or alpm_pkg_get_depends(pkg) is used to get the dependency list of pkg. In the old code it was easy to determine if a dependency is broken. If we couldn't find a satisfier package in the target list or in the local database (which won't be upgraded), we simply detected that this dependency is unresolvable. (However, our error message was often uninformative, user may got an error message like this: "Cannot resolve foo dependency of pulledpkg". User asked: Pulledpkg? What? I just did "pacman -S bar". [1])
However, your patch makes things more complicated. We start to resolve foo dependency of bar, which may be satisfied. But *later* it may turn out, that we cannot resolve completely the _pulled_ foo satisfier, so we simply remove it from the list. Now it would be elegant if we could say: We could not resolve foo dependency of bar. (At this point we can clearly see, that the old method doesn't work, because *locally* foo is a resolvable dependency.) But it is not so easy to detect, probably we need a more sophisticated info (graph?) structure, and imho we can catch this broken dependency when we "label" the package as unresolvable.
I'm not sure I follow everything you are saying here, and in a later email you say that what you've written here is due to some misinterpretation, but in terms of the last part, I think you're saying that it would be nice if there were a more sophisticated data structure that could describe all of the dependencies that were calculated, including the ones that could not be resolved, and why, so that the front-end can print out a more informative message, indicating what could not be resolved and why, instead of just listing the packages that could not be resolved (without saying why). I agree - but I have suggested that I would do this in a second checkin. Already there has been push back on this change because people are saying it's too big (I disagree; it's as small as it can be to accomplish the goal, but anyway ...). I'd rather not take on even more stuff for this particular change. Thanks, Bryan
OK, in my own repository I have fixed the bug that I realized while writing my description of the is_needed() function in a previous email, and also have committed an additional 2 test cases which exercise some of the bugs that would have occurred had I not fixed issues pointed out to me on this list. I'm ready to send my patches as 2 patches: - One with everything except the removal of the "would you like to not ignore this package that you put in your IgnorePkg/IgnoreGroup list" dialog - One to subsequently remove said dialog Is there anything that would prevent these patches from being accepted? By the way, discovering the pactest stuff was a very pleasant surprise. I didn't expect such a nice testing facility to be present, I don't know why, I guess because even commercial codebases I have worked on often don't have good testing facilities ... pactest is super awesome. One thing that would be nice would be a way to run pactest with the version of pacman built into the source tree in which pactest is embedded, rather than having pactest just run the system pacman. I wrote a simple shell script wrapper for pactest.py that does this: #!/bin/sh export PATH=/path/to/my/sources/for/pacman/src/pacman/.libs:$PATH export LD_LIBRARY_PATH=/path/to/my/sources/for/pacman/lib/libalpm/.libs:$LD_LIBRARY_PATH pactest.py $@ This works for me, but it took me a while to figure out how pactest.py was working (and if it had some convenient way to run the local pacman instead of the system pacman). Did I miss something, or is there an easier way? Thanks, Bryan
On Tue, Jan 13, 2009 at 4:07 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
This works for me, but it took me a while to figure out how pactest.py was working (and if it had some convenient way to run the local pacman instead of the system pacman). Did I miss something, or is there an easier way?
make check? -Dan
On Tue, Jan 13, 2009 at 1:25 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Tue, Jan 13, 2009 at 4:07 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
This works for me, but it took me a while to figure out how pactest.py was working (and if it had some convenient way to run the local pacman instead of the system pacman). Did I miss something, or is there an easier way?
make check?
make check is indeed the best to do a full check. Once you saw the failing pactests, and you want to check specific ones, I have the following helper in my pacman tree :
cat runpactest.sh #!/bin/sh
python ./pactest/pactest.py --test $@ -p \ ./src/pacman/pacman --debug=1
On Tue, Jan 13, 2009 at 11:07 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
I'm ready to send my patches as 2 patches:
- One with everything except the removal of the "would you like to not ignore this package that you put in your IgnorePkg/IgnoreGroup list" dialog - One to subsequently remove said dialog
Short and simple patches are much more likely to be accepted :) So I would do the short and simple patches first, and then basing the more complex ones on them. That way, you have more chances that at least a part of your work get merged. I have to be honest and admit I didn't find the motivation to review your changes. I do like the idea, but it's still a huge code addition ( lib/libalpm/deps.c | 259 +++++++++++++++++++++++++++++++++++------) and I was hoping there would be a better and simpler way to integrate this in the existing code. Maybe the idea Nagy made several times before, to compute our dependency graph once and then store it and re-use it, would help here. But this is a huge change too :)
On Tue, Jan 13, 2009 at 11:07 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
I'm ready to send my patches as 2 patches:
- One with everything except the removal of the "would you like to not ignore this package that you put in your IgnorePkg/IgnoreGroup list" dialog - One to subsequently remove said dialog
Short and simple patches are much more likely to be accepted :) So I would do the short and simple patches first, and then basing the more complex ones on them. That way, you have more chances that at least a part of your work get merged.
I have to be honest and admit I didn't find the motivation to review your changes. I do like the idea, but it's still a huge code addition ( lib/libalpm/deps.c | 259 +++++++++++++++++++++++++++++++++++------) and I was hoping there would be a better and simpler way to integrate this in the existing code. Maybe the idea Nagy made several times before, to compute our dependency graph once and then store it and re-use it, would help here. But this is a huge change too :) ______________________________________________ OK, I'll do my best to split my patches up. But most of what I am
Xavier wrote: trying to do cannot be split up. If the number of added lines in deps.c scares you, please understand that it's just the addition of one new internal data structure, a few helper methods, and some moderate re-arrangement of one existing function. If you'd apply the patch locally, you'd be able to see how reasonably self-contained it all is. Anyway, I'll make one more attempt and getting the patches into a form that you'll accept. Thanks, Bryan
participants (4)
-
Bryan Ischo
-
Dan McGee
-
Nagy Gabor
-
Xavier