[pacman-dev] sync_prepare clean-up III, TODO
Hi! Well, the remaining part is very messy and disappointing: The whole conflict checking should be refactored: as I assume this wants to do something similar to checkdeps(...TRANS_TYPE_UPDATE), but that is buggy: this should be fixed as deps.c (conflict001.py shows this.) However, as Xavier said earlier a new data stucture is needed for conflicts; and all conflict resolve stuff should be moved to conflict.c. The second pactest file shows checkdeps cannot get enough information from sync.c to figure out if there is any broken dependency: it should know BOTH the remove and upgrade list to decide properly. Well, both of this bugs needs hard work: a proper conflict resolution is a difficult (probably recursive) problem, it would be easier to let user do this work manually. Bye, ngaba PS: I don't like inefficient pmsyncpkg_t neither.
On Thu, Jul 19, 2007 at 08:32:36PM +0200, Nagy Gabor wrote:
The second pactest file shows checkdeps cannot get enough information from sync.c to figure out if there is any broken dependency: it should know BOTH the remove and upgrade list to decide properly. self.description = "Induced removal would break dependency"
Actually, that's how I first understood the trans argument that you removed from the checkdeps function. I thought it existed exactly for that (knowing both upgrade and remove list). But it appears it was totally wrongly used, and I thought it would be possible to deal with this in sync.c anyway, but didn't think too much about it.
sp1 = pmpkg("pkg1", "1.0-2") sp1.replaces = [ "pkg2" ] self.addpkg2db("sync", sp1)
sp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("local", sp2)
sp3 = pmpkg("pkg3", "1.0-2") sp3.depends = ["pkg2=1.0-2"] self.addpkg2db("sync", sp3)
lp1 = pmpkg("pkg1", "1.0-1") self.addpkg2db("sync", lp1)
lp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("local", lp2)
lp3 = pmpkg("pkg3", "1.0-1") self.addpkg2db("local", lp3)
self.args = "-Su"
self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=pkg2")
That pactest doesn't look right, does it?
That pactest doesn't look right, does it?
Oh, yes; copy/paste (fix sync / local):-P I'm on holiday now, there is no linux machine here, but you can probably fix the pactest file; after reading the code: the corrected pactest file probably fails. Bye, ngaba
On Mon, Jul 23, 2007 at 10:49:53PM +0200, Nagy Gabor wrote:
That pactest doesn't look right, does it?
Oh, yes; copy/paste (fix sync / local):-P I'm on holiday now, there is no linux machine here, but you can probably fix the pactest file; after reading the code: the corrected pactest file probably fails.
Should the pactest look like that ? (I'm not sure how you wanted pkg2 versions in local and sync repo) : self.description = "Induced removal would break dependency" sp1 = pmpkg("pkg1", "1.0-2") sp1.replaces = [ "pkg2" ] self.addpkg2db("sync", sp1) sp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("sync", sp2) sp3 = pmpkg("pkg3", "1.0-2") sp3.depends = ["pkg2=1.0-2"] self.addpkg2db("sync", sp3) lp1 = pmpkg("pkg1", "1.0-1") self.addpkg2db("local", lp1) lp2 = pmpkg("pkg2", "1.0-1") self.addpkg2db("local", lp2) lp3 = pmpkg("pkg3", "1.0-1") self.addpkg2db("local", lp3) self.args = "-Su" self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=pkg2") What happens here is that pacman first want to replace pkg2 by pkg1, but since pkg1 doesn't provide pkg2 (is that a packaging error or what?), pacman will pull it back when resolving the dependencies of sp3. And so it'll actually upgrade the 3 packages without problems, since pkg1 doesn't conflict with pkg2. So this pactest is not really interesting imo. I don't think that what pacman does is really wrong here. What's really wrong is sp1, because it shouldn't only replaces pkg2, it should also conflicts/provides it. So the usual case looks like this (and it works fine) : self.description = "Induced removal would break dependency" sp1 = pmpkg("pkg1", "1.0-2") sp1.replaces = [ "pkg2" ] sp1.conflicts = [ "pkg2" ] sp1.provides = [ "pkg2" ] self.addpkg2db("sync", sp1) sp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("sync", sp2) sp3 = pmpkg("pkg3", "1.0-2") sp3.depends = ["pkg2=1.0-2"] self.addpkg2db("sync", sp3) lp1 = pmpkg("pkg1", "1.0-1") self.addpkg2db("local", lp1) lp2 = pmpkg("pkg2", "1.0-1") self.addpkg2db("local", lp2) lp3 = pmpkg("pkg3", "1.0-1") self.addpkg2db("local", lp3) self.args = "-Su" self.addrule("PACMAN_RETCODE=0") self.addrule("!PKG_EXIST=pkg2") self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg3")
On Thu, Jul 19, 2007 at 08:32:36PM +0200, Nagy Gabor wrote:
self.description = "target vs db conflict will disappear after upgrade"
sp1 = pmpkg("pkg1") sp1.conflicts = ["imaginary"] self.addpkg2db("sync", sp1);
sp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("sync", sp2)
lp = pmpkg("pkg2", "1.0-1") lp.provides = ["imaginary"] self.addpkg2db("local", lp)
self.args = "-S %s" % " ".join([p.name for p in sp1, sp2])
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") self.addrule("PKG_EXIST=pkg2") self.addrule("PKG_VERSION=pkg2|1.0-2")
I thought this was already handled, but no :) Actually, this would just need the check I added to chk_db_vs_targets here : http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=cb164c3130f15... but for the chk_target_vs_db this time. Anyway, by looking at this code again, I thought about a way of cleaning it up a bit, so there is now less code for doing the same thing better imo :)
From 01046d565192b13724f10116708ea9782f83f772 Mon Sep 17 00:00:00 2001 From: Chantry Xavier <shiningxc@gmail.com> Date: Fri, 20 Jul 2007 01:22:45 +0200 Subject: [PATCH] libalpm/conflict.c : cleanup + fix for conflict001.
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/conflict.c | 228 ++++++++++++++---------------------------------- 1 files changed, 65 insertions(+), 163 deletions(-) diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 98f4d66..acfcda9 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -47,172 +47,83 @@ #include "deps.h" -/** See if potential conflict 'name' matches package 'pkg'. - * @param target the name of the parent package we're checking - * @param depname the name of the dependency we're checking - * @param pkg the package to check - * @param conflict the name of the possible conflict - * @return A depmissing struct indicating the conflict - * @note The first two paramters are here to simplify the addition - * of new 'depmiss' objects. - * - * TODO WTF is a 'depmissing' doing indicating a conflict?? +/** Check if pkg1 conflicts with pkg2 + * @param pkg1 package we are looking at + * @param conflict name of the possible conflict + * @param pkg2 package to check */ -static pmdepmissing_t *does_conflict(const char *target, const char *depname, - pmpkg_t *pkg, const char *conflict) +static int does_conflict(pmpkg_t *pkg1, const char *conflict, pmpkg_t *pkg2) { - alpm_list_t *i; - - /* check the actual package name, easy */ - if(strcmp(alpm_pkg_get_name(pkg), conflict) == 0) { - _alpm_log(PM_LOG_DEBUG, " found conflict '%s' : package '%s'", - conflict, target); - return(_alpm_depmiss_new(target, PM_DEP_TYPE_CONFLICT, - PM_DEP_MOD_ANY, depname, NULL)); - } else { - /* check what this package provides, harder */ - for(i = alpm_pkg_get_provides(pkg); i; i = i->next) { - const char *provision = i->data; - - if(strcmp(provision, conflict) == 0) { - _alpm_log(PM_LOG_DEBUG, " found conflict '%s' : package '%s' provides '%s'", - conflict, target, provision); - return(_alpm_depmiss_new(target, PM_DEP_TYPE_CONFLICT, - PM_DEP_MOD_ANY, depname, NULL)); - } - } + const char *pkg1name = alpm_pkg_get_name(pkg1); + const char *pkg2name = alpm_pkg_get_name(pkg2); + pmdepend_t *conf = alpm_splitdep(conflict); + int match = 0; + + match = alpm_depcmp(pkg2, conf); + if(match) { + _alpm_log(PM_LOG_DEBUG, "package %s conflicts with %s (by %s)", + pkg1name, pkg2name, conflict); } - return(NULL); /* not a conflict */ + free(conf); + return(match); } -static alpm_list_t *chk_pkg_vs_db(alpm_list_t *baddeps, pmpkg_t *pkg, pmdb_t *db) +/** Adds the pkg1/pkg2 conflict to the baddeps list + * + * TODO WTF is a 'depmissing' doing indicating a conflict?? + */ +static void add_conflict(alpm_list_t **baddeps, const char *pkg1, const char *pkg2) { - pmdepmissing_t *miss = NULL; - const char *pkgname; - alpm_list_t *i, *j; - - pkgname = alpm_pkg_get_name(pkg); - - for(i = alpm_pkg_get_conflicts(pkg); i; i = i->next) { - const char *conflict = i->data; - - if(strcmp(pkgname, conflict) == 0) { - /* a package cannot conflict with itself -- that's just not nice */ - _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with itself - packaging error", - pkgname); - continue; - } - - /* CHECK 1: check targets against database */ - _alpm_log(PM_LOG_DEBUG, "checkconflicts: target '%s' vs db", pkgname); - - for(j = _alpm_db_get_pkgcache(db); j; j = j->next) { - pmpkg_t *dbpkg = j->data; - - if(strcmp(alpm_pkg_get_name(dbpkg), pkgname) == 0) { - /* skip the package we're currently processing */ - continue; - } - - miss = does_conflict(pkgname, alpm_pkg_get_name(dbpkg), dbpkg, conflict); - if(miss && !_alpm_depmiss_isin(miss, baddeps)) { - baddeps = alpm_list_add(baddeps, miss); - } else { - FREE(miss); - } - } + pmdepmissing_t *miss = _alpm_depmiss_new(pkg1, PM_DEP_TYPE_CONFLICT, + PM_DEP_MOD_ANY, pkg2, NULL); + if(miss && !_alpm_depmiss_isin(miss, *baddeps)) { + *baddeps = alpm_list_add(*baddeps, miss); + } else { + FREE(miss); } - return(baddeps); } -static alpm_list_t *chk_pkg_vs_targets(alpm_list_t *baddeps, - pmpkg_t *pkg, pmdb_t *db, - alpm_list_t *targets) -{ - pmdepmissing_t *miss = NULL; - const char *pkgname; - alpm_list_t *i, *j; - - pkgname = alpm_pkg_get_name(pkg); +/** Check if packages from list1 conflict with packages from list2 + * Add the conflicts to the baddeps list, following the order. */ +static void check_conflict(alpm_list_t *list1, alpm_list_t *list2, alpm_list_t **baddeps, int order) { + alpm_list_t *i, *j, *k; - for(i = alpm_pkg_get_conflicts(pkg); i; i = i->next) { - const char *conflict = i->data; - - if(strcmp(pkgname, conflict) == 0) { - /* a package cannot conflict with itself -- that's just not nice */ - _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with itself - packaging error", - pkgname); - continue; - } - - /* CHECK 2: check targets against targets */ - _alpm_log(PM_LOG_DEBUG, "checkconflicts: target '%s' vs all targets", pkgname); + if(!baddeps) { + return; + } + for(i = list1; i; i = i->next) { + pmpkg_t *pkg1 = i->data; + const char *pkg1name = alpm_pkg_get_name(pkg1); - for(j = targets; j; j = j->next) { - const char *targetname; - pmpkg_t *target = j->data; - targetname = alpm_pkg_get_name(target); + for(j = alpm_pkg_get_conflicts(pkg1); j; j = j->next) { + const char *conflict = j->data; - if(strcmp(targetname, pkgname) == 0) { - /* skip the package we're currently processing */ + if(strcmp(pkg1name, conflict) == 0) { + /* a package cannot conflict with itself -- that's just not nice */ + _alpm_log(PM_LOG_DEBUG, "package '%s' conflicts with itself - packaging error", + pkg1name); continue; } - miss = does_conflict(pkgname, targetname, target, conflict); - if(miss && !_alpm_depmiss_isin(miss, baddeps)) { - baddeps = alpm_list_add(baddeps, miss); - } else { - FREE(miss); - } - } - } - return(baddeps); -} - -static alpm_list_t *chk_db_vs_targets(alpm_list_t *baddeps, pmpkg_t *pkg, - pmdb_t *db, alpm_list_t *targets) -{ - pmdepmissing_t *miss = NULL; - const char *pkgname; - alpm_list_t *i, *j; - - pkgname = alpm_pkg_get_name(pkg); - - _alpm_log(PM_LOG_DEBUG, "checkconflicts: db vs target '%s'", pkgname); - - for(i = _alpm_db_get_pkgcache(db); i; i = i->next) { - alpm_list_t *conflicts = NULL; - const char *dbpkgname; - - pmpkg_t *dbpkg = i->data; - dbpkgname = alpm_pkg_get_name(dbpkg); - - if(strcmp(dbpkgname, pkgname) == 0) { - /* skip the package we're currently processing */ - continue; - } - - if(_alpm_pkg_find(dbpkgname, targets)) { - /* skip targets, already handled by chk_pkg_vs_targets in checkconflicts */ - _alpm_log(PM_LOG_DEBUG, "target '%s' is also in target list, ignoring it", - dbpkgname); - continue; - } - - conflicts = alpm_pkg_get_conflicts(dbpkg); + for(k = list2; k; k = k->next) { + pmpkg_t *pkg2 = k->data; + const char *pkg2name = alpm_pkg_get_name(pkg2); - for(j = conflicts; j; j = j->next) { - const char *conflict = j->data; + if(strcmp(pkg1name, pkg2name) == 0) { + /* skip the package we're currently processing */ + continue; + } - miss = does_conflict(pkgname, dbpkgname, pkg, conflict); - if(miss && !_alpm_depmiss_isin(miss, baddeps)) { - baddeps = alpm_list_add(baddeps, miss); - } else { - FREE(miss); + if(does_conflict(pkg1, conflict, pkg2)) { + if(order >= 0) { + add_conflict(baddeps, pkg1name, pkg2name); + } else { + add_conflict(baddeps, pkg2name, pkg1name); + } + } } } } - return(baddeps); } /* Returns a alpm_list_t* of pmdepmissing_t pointers. @@ -221,7 +132,7 @@ static alpm_list_t *chk_db_vs_targets(alpm_list_t *baddeps, pmpkg_t *pkg, */ alpm_list_t *_alpm_checkconflicts(pmdb_t *db, alpm_list_t *packages) { - alpm_list_t *i, *baddeps = NULL; + alpm_list_t *baddeps = NULL; ALPM_LOG_FUNC; @@ -229,23 +140,14 @@ alpm_list_t *_alpm_checkconflicts(pmdb_t *db, alpm_list_t *packages) return(NULL); } - for(i = packages; i; i = i->next) { - pmpkg_t *pkg = i->data; - if(pkg == NULL) { - continue; - } + alpm_list_t *dblist = alpm_list_diff(_alpm_db_get_pkgcache(db), packages, _alpm_pkg_cmp); - /* run three different conflict checks on each package */ - baddeps = chk_pkg_vs_db(baddeps, pkg, db); - baddeps = chk_pkg_vs_targets(baddeps, pkg, db, packages); - baddeps = chk_db_vs_targets(baddeps, pkg, db, packages); - } - - /* debug loop */ - for(i = baddeps; i; i = i->next) { - pmdepmissing_t *miss = i->data; - _alpm_log(PM_LOG_DEBUG, "\tCONFLICTS:: %s conflicts with %s", miss->target, miss->depend.name); - } + _alpm_log(PM_LOG_DEBUG, "check targets vs db"); + check_conflict(packages, dblist, &baddeps, 1); + _alpm_log(PM_LOG_DEBUG, "check db vs targets"); + check_conflict(dblist, packages, &baddeps, -1); + _alpm_log(PM_LOG_DEBUG, "check targets vs targets"); + check_conflict(packages, packages, &baddeps, 0); return(baddeps); } -- 1.5.2.4
Well, the fixed pactest file, this should fail: -------- self.description = "Induced removal would break dependency" sp1 = pmpkg("pkg1", "1.0-2") sp1.replaces = [ "pkg2" ] self.addpkg2db("sync", sp1) sp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("sync", sp2) sp3 = pmpkg("pkg3", "1.0-2") sp3.depends = ["pkg2=1.0-2"] self.addpkg2db("sync", sp3) lp1 = pmpkg("pkg1", "1.0-1") self.addpkg2db("local", lp1) lp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("local", lp2) lp3 = pmpkg("pkg3", "1.0-1") self.addpkg2db("local", lp3) self.args = "-Su" self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=pkg2")
On Wed, Jul 25, 2007 at 11:35:58AM +0200, Nagy Gabor wrote:
Well, the fixed pactest file, this should fail: -------- self.description = "Induced removal would break dependency"
sp1 = pmpkg("pkg1", "1.0-2") sp1.replaces = [ "pkg2" ] self.addpkg2db("sync", sp1)
sp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("sync", sp2)
sp3 = pmpkg("pkg3", "1.0-2") sp3.depends = ["pkg2=1.0-2"] self.addpkg2db("sync", sp3)
lp1 = pmpkg("pkg1", "1.0-1") self.addpkg2db("local", lp1)
lp2 = pmpkg("pkg2", "1.0-2") self.addpkg2db("local", lp2)
lp3 = pmpkg("pkg3", "1.0-1") self.addpkg2db("local", lp3)
self.args = "-Su"
self.addrule("PACMAN_RETCODE=1") self.addrule("PKG_EXIST=pkg2")
You were right to ignore everything I just said, it doesn't apply. The local version of pkg2 being 1.0-2 is very important, that's why I wanted you to submit the new corrected pactest yourself :) So yes, there is indeed a problem when a dependency is satisfied by a locally installed package going to be removed (pkg2 here). Btw it's possible to achieve the same with conflicts instead of replaces. Not sure which one we should choose, maybe both? Anyway, is here the same (I think) pactest simplified a bit, in a git patch :
From fa7f06fce067baf1fd5d857fcee4178444c2f811 Mon Sep 17 00:00:00 2001 From: Chantry Xavier <shiningxc@gmail.com> Date: Wed, 25 Jul 2007 14:47:46 +0200 Subject: [PATCH] Add sync1003 pactest.
This test shows a problem when a dependency is satisfied by a locally installed package going to be removed (pkg2 here). checkdeps doesn't know about packages going to be removed, and so fails to do its job (checking dependencies) correctly. Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- pactest/tests/sync1003.py | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-) create mode 100644 pactest/tests/sync1003.py diff --git a/pactest/tests/sync1003.py b/pactest/tests/sync1003.py new file mode 100644 index 0000000..27a7aa5 --- /dev/null +++ b/pactest/tests/sync1003.py @@ -0,0 +1,17 @@ +self.description = "Induced removal would break dependency" + +sp1 = pmpkg("pkg1", "1.0-1") +sp1.conflicts = [ "pkg2" ] +self.addpkg2db("sync", sp1) + +lp2 = pmpkg("pkg2", "1.0-2") +self.addpkg2db("local", lp2) + +lp3 = pmpkg("pkg3", "1.0-1") +lp3.depends = ["pkg2=1.0-2"] +self.addpkg2db("local", lp3) + +self.args = "-S pkg1" + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=pkg2") -- 1.5.2.4
On Wed, Jul 25, 2007 at 02:50:18PM +0200, Xavier wrote:
You were right to ignore everything I just said, it doesn't apply. The local version of pkg2 being 1.0-2 is very important, that's why I wanted you to submit the new corrected pactest yourself :)
So yes, there is indeed a problem when a dependency is satisfied by a locally installed package going to be removed (pkg2 here). Btw it's possible to achieve the same with conflicts instead of replaces. Not sure which one we should choose, maybe both?
Anyway, is here the same (I think) pactest simplified a bit, in a git patch :
From fa7f06fce067baf1fd5d857fcee4178444c2f811 Mon Sep 17 00:00:00 2001 From: Chantry Xavier <shiningxc@gmail.com> Date: Wed, 25 Jul 2007 14:47:46 +0200 Subject: [PATCH] Add sync1003 pactest.
This test shows a problem when a dependency is satisfied by a locally installed package going to be removed (pkg2 here).
checkdeps doesn't know about packages going to be removed, and so fails to do its job (checking dependencies) correctly.
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- pactest/tests/sync1003.py | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-) create mode 100644 pactest/tests/sync1003.py
diff --git a/pactest/tests/sync1003.py b/pactest/tests/sync1003.py new file mode 100644 index 0000000..27a7aa5 --- /dev/null +++ b/pactest/tests/sync1003.py @@ -0,0 +1,17 @@ +self.description = "Induced removal would break dependency" + +sp1 = pmpkg("pkg1", "1.0-1") +sp1.conflicts = [ "pkg2" ] +self.addpkg2db("sync", sp1) + +lp2 = pmpkg("pkg2", "1.0-2") +self.addpkg2db("local", lp2) + +lp3 = pmpkg("pkg3", "1.0-1") +lp3.depends = ["pkg2=1.0-2"] +self.addpkg2db("local", lp3) + +self.args = "-S pkg1" + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_EXIST=pkg2") -- 1.5.2.4
You can ignore that again.. That pactest is broken, pkg2 is required by pkg3, but requiredby field of pkg2 is empty. Sorry.. By adding lp2.requiredby = [ "pkg3" ] , it works fine. But that's actually the problem. checkdeps use the requiredby fields, but there is no requiredby fields for sync packages, only for local ones.
On Wed, Jul 25, 2007 at 05:00:43PM +0200, Xavier wrote:
You can ignore that again.. That pactest is broken, pkg2 is required by pkg3, but requiredby field of pkg2 is empty. Sorry.. By adding lp2.requiredby = [ "pkg3" ] , it works fine.
But that's actually the problem. checkdeps use the requiredby fields, but there is no requiredby fields for sync packages, only for local ones.
I wonder if the requiredby fields couldn't be updated while doing the checkdeps / resolvedeps stuff, and then commit these changes to the database when the transaction is done, instead of computing requiredby again in the update_requiredby and update_depends functions. I don't think that's easy to do however. Maybe just adding the list of packages to be installed as an arg of checkdeps would be good enough, even if it looks a little hackish.
Hi! This is an answer to this thread (I didn't get mails from this ML because my mail "address" didn't work, I changed to my secondary mail address now). So, I think requiredby handling of syncpkgs is correct, this is needed for to-be-removed packages only and that field is correctly copied from pkgcache in sync.c, as I see. I think pm_syncpkgt is not efficient: a kernel-list-like structure would be much better, or new .sync member(s) to pm_pkg_t struct (Or in more general: graph-like structures are even better). My solution to sync1003.py: simply pass the remove_list to checkdeps and handle it. Bye, ngaba
On Wed, Jul 25, 2007 at 09:20:59PM +0200, Nagy Gabor wrote:
Hi! This is an answer to this thread (I didn't get mails from this ML because my mail "address" didn't work, I changed to my secondary mail address now). So, I think requiredby handling of syncpkgs is correct, this is needed for to-be-removed packages only and that field is correctly copied from pkgcache in sync.c, as I see. I think pm_syncpkgt is not efficient: a kernel-list-like structure would be much better, or new .sync member(s) to pm_pkg_t struct (Or in more general: graph-like structures are even better). My solution to sync1003.py: simply pass the remove_list to checkdeps and handle it.
I'm attaching a git patch, so that this pactest could be easily added to git.
On Wed, Jul 25, 2007 at 09:20:59PM +0200, Nagy Gabor wrote:
So, I think requiredby handling of syncpkgs is correct, this is needed for to-be-removed packages only and that field is correctly copied from pkgcache in sync.c, as I see.
That's not exactly what I meant. I thought we could see pkg2 as required by pkg3, because the new pkg3 version that is going to be installed requires pkg2, while local pkg3 doesn't. That was just an idea, not necessarily a good one, and in any cases, the solution you proposed is most probably much easier to implement.
I think pm_syncpkgt is not efficient: a kernel-list-like structure would be much better, or new .sync member(s) to pm_pkg_t struct (Or in more general: graph-like structures are even better).
I'm not sure I see what's the performance problem with this type, but I'm curious, so could you elaborate a bit ? :) If you see a better way that would also clean up the code, it could be interesting. If it complicates the code a lot, for nearly 0 performance gain (what I did with testdb ;)), then it's probably worthless.
> I'm not sure I see what's the performance problem with this type, but I'm > curious, so could you elaborate a bit ? :) > If you see a better way that would also clean up the code, it could be > interesting. Hi! 1. Almost all alpm functions need a list of pmpkg_ts; and the pmpkg_t <-> pmsyncpkg_t conversion is costly (see _alpm_sync_prepare: after topo sorting the list, you need to do an O(n^2) conversion); a kernel list-like implemetation would be more efficient (or merging pmsyncpkt_t to pmpkg_t) 2. The implementation of replaces is very ugly. 3. alpm_sync_free doesn't free (or what is FREE?!) the pkg member of spkg struct, however other parts of the code assume that it does it: huge memleak (?!) 4. in more general: this pmpkg_t of alpm_list implementation is not efficient, internally graph-like structures would be better, now you need to do at least O (n) computation to find the "edges" (and we should do this only once) etc. Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
3. alpm_sync_free doesn't free (or what is FREE?!) the pkg member of spkg struct, however other parts of the code assume that it does it: huge memleak Well, this is not true, pkg usually points to a pkgcache entry; sry. However, this can be dangerous (if you reload pkgcache) Bye, ngaba
---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
participants (3)
-
Nagy Gabor
-
ngaba＠bibl.u-szeged.hu
-
Xavier