On Mon, Jan 26, 2009 at 6:48 AM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote: I think your subject got cut off. Git limits you to 76 or so characters, so maybe shorten it up and add something to the body? Anyway, I realized I never really reviewed this guy (really sorry about that), and there are a few minor issues with it...
From: Bryan Ischo <bryan@ischo.com>
Signed-off-by: Bryan Ischo <bryan@ischo.com> --- lib/libalpm/alpm.h | 3 ++- lib/libalpm/deps.c | 2 +- lib/libalpm/sync.c | 23 ++++++++++++++++++----- pactest/tests/provision020.py | 2 +- pactest/tests/provision022.py | 2 +- pactest/tests/sync1008.py | 2 +- pactest/tests/sync300.py | 2 +- src/pacman/callback.c | 25 +++++++++++++++++++++++++ 8 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 7b7ca4e..3836d60 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -371,7 +371,8 @@ typedef enum _pmtransconv_t { PM_TRANS_CONV_REPLACE_PKG = 0x02, PM_TRANS_CONV_CONFLICT_PKG = 0x04, PM_TRANS_CONV_CORRUPTED_PKG = 0x08, - PM_TRANS_CONV_LOCAL_NEWER = 0x10 + PM_TRANS_CONV_LOCAL_NEWER = 0x10, + PM_TRANS_CONV_REMOVE_PKGS = 0x20, } pmtransconv_t;
/* Transaction Progress */ diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index da21087..8bec4c0 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -609,7 +609,7 @@ int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, pmpkg_t *pkg, if(!spkg) { pm_errno = PM_ERR_UNSATISFIED_DEPS; char *missdepstring = alpm_dep_compute_string(missdep); - _alpm_log(PM_LOG_ERROR, _("cannot resolve \"%s\", a dependency of \"%s\"\n"), + _alpm_log(PM_LOG_WARNING, _("cannot resolve \"%s\", a dependency of \"%s\"\n"), missdepstring, tpkg->name); free(missdepstring); if(data) { diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 5e5ca92..ea903f4 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -442,12 +442,25 @@ int _alpm_sync_prepare(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t *dbs_sync dependencies not already on the list */ }
- /* If there were unresolvable top-level packages, fail the - transaction. */ + /* If there were unresolvable top-level packages, prompt the user to + see if they'd like to ignore them rather than failing the sync */ if(unresolvable != NULL) { - /* pm_errno is set by resolvedeps */ - ret = -1; - goto cleanup; + int remove_unresolvable = 0; + QUESTION(handle->trans, PM_TRANS_CONV_REMOVE_PKGS, unresolvable, NULL, NULL, &remove_unresolvable); You can wrap this if you want, ignore my earlier weird recommendations. + if (remove_unresolvable) { + /* User wants to remove the unresolvable packages from the + transaction, so simply drop the unresolvable list. The + packages will be removed from the actual transaction when + the transaction packages are replaced with a + dependency-reordered list below */ + alpm_list_free(unresolvable); + unresolvable = NULL; + } + else { + /* pm_errno is set by resolvedeps */ + ret = -1; + goto cleanup; + } }
/* Add all packages which were "pulled" (i.e. weren't already in the 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") I think I understand why all these changed from 1 to 0, but isn't it a bit odd that some of these "success" codes now result from pacman doing nothing in the --noconfirm case? I guess a little explanation for me might be in order, it might just be me that is confused. Putting a note in the commit message might be helpful, e.g. "The return codes for several pacman commands are now true because <reason>..."
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/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 6e7930c..1e2bff3 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -270,6 +270,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", You have trailing whitespace here. See my suggestion about enabling the pre-commit hook. + alpm_pkg_get_name(i->data), (i->next) ? "," : ""); + } I really don't like reinventing the wheel here. I realize that list_display() in util.c only takes an alpm_list_t of strings right now, so that doesn't perfectly fit the bill, but I would rather we use that if possible. What about something like the following (psuedo-C):
+ *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" : ""); These tricks are cool until translators are involved. You can't do
namelist = NULL; for (i = unresolved; i; i = i->next) { namelist = alpm_list_add(namelist, alpm_pkg_get_name(i->data); } .... list_display(" ", namelist); .... alpm_list_free(namelist); this in an i18n application- you are going to have to switch on the whole word, or something. I'd rather just put the string "package(s)" in there.
+ 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
If you can't get to these changes, then I will sometime as I feel I owe you one for being so late on reviewing these. -Dan