[pacman-dev] Some comments on Bryan patches (bugs)
Hi! I know that I speak too late, but I haven't time/motivation to review this patch nowadays. 1. See alpm/sync.c/sync_prepare(). When user decides to remove unresolvable targets from the list, our code doesn't remove any targets from target->packages, just adds the pulled dependencies. The actual removal is done in the sortbydeps part (sortbydeps is called with list param!). This works, but it is hard to understand (imho), and it is a memleak: some pmsyncpkg_t structs magically disappeared. 2. Behaviour change (this pactest now fails) --- sync993.py --- self.description = "Choose a dependency satisfier (target-list vs. database)" sp1 = pmpkg("pkg1") sp1.depends = ["dep"] sp2 = pmpkg("pkg2") sp2.provides = ["dep"] sp3 = pmpkg("pkg3") sp3.provides = ["dep"] for p in sp1, sp2, sp3: self.addpkg2db("sync", p) self.args = "-S pkg1 pkg3" self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") self.addrule("PKG_EXIST=pkg3") ------------------ If you define conflict between pkg2 and pkg3, pacman will refuse to commit the transation. Before this patch, pacman always chose from target list. Bye
I know that I speak too late, but I haven't time/motivation to review this patch nowadays.
I forgot to say that I think Bryan did a great work: The implementation is much simpler than I expected. Bye ------------------------------------------------------ SZTE Egyetemi Konyvtar - http://www.bibl.u-szeged.hu This message was sent using IMP: http://horde.org/imp/
Nagy Gabor wrote:
Hi!
I know that I speak too late, but I haven't time/motivation to review this patch nowadays.
1. See alpm/sync.c/sync_prepare(). When user decides to remove unresolvable targets from the list, our code doesn't remove any targets from target->packages, just adds the pulled dependencies. The actual removal is done in the sortbydeps part (sortbydeps is called with list param!). This works, but it is hard to understand (imho), and it is a memleak: some pmsyncpkg_t structs magically disappeared.
I don't think that it's confusing that it happens this way; the resolve operation builds up a new list of packages for the transaction, and that list is then used to replace the trans->packages list. This is opposed to before where the resolve operation modified trans->packages in place. You are right that there is a memory leak there, though; my change failed to free up any pmsyncpkg_t structs which were in trans->packages but were not in the replacement list. I will submit a small and simple patch which fixes this issue shortly. Good catch!
2. Behaviour change (this pactest now fails) --- sync993.py --- self.description = "Choose a dependency satisfier (target-list vs. database)"
sp1 = pmpkg("pkg1") sp1.depends = ["dep"]
sp2 = pmpkg("pkg2") sp2.provides = ["dep"]
sp3 = pmpkg("pkg3") sp3.provides = ["dep"]
for p in sp1, sp2, sp3: self.addpkg2db("sync", p)
self.args = "-S pkg1 pkg3"
self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") self.addrule("PKG_EXIST=pkg3") ------------------
If you define conflict between pkg2 and pkg3, pacman will refuse to commit the transation. Before this patch, pacman always chose from target list.
sync993 does not exist in the pacman sources. If it did, I would have seen this problem and addressed it. I will get back to you on this once I have figured out what's going on (I suspect that it's a difference in the way the new resolve code works, it may stop too short now to figure out that there is a 'better' package to choose for resolving the dependency). Thanks, Bryan
Nagy Gabor wrote:
Hi!
I know that I speak too late, but I haven't time/motivation to review this patch nowadays.
1. See alpm/sync.c/sync_prepare(). When user decides to remove unresolvable targets from the list, our code doesn't remove any targets from target->packages, just adds the pulled dependencies. The actual removal is done in the sortbydeps part (sortbydeps is called with list param!). This works, but it is hard to understand (imho), and it is a memleak: some pmsyncpkg_t structs magically disappeared.
I don't think that it's confusing that it happens this way; the resolve operation builds up a new list of packages for the transaction, and that list is then used to replace the trans->packages list. This is opposed to before where the resolve operation modified trans->packages in place.
You are right that there is a memory leak there, though; my change failed to free up any pmsyncpkg_t structs which were in trans->packages but were not in the replacement list. I will submit a small and simple patch which fixes this issue shortly.
OK, but please insert a short comment somewhere to make our code more readable. (The unresolvable targets will be removed later...)
sync993 does not exist in the pacman sources. If it did, I would have seen this problem and addressed it.
That's why I posted it here :) I've just created it to show the problem. (I like pactests, because we can test our patches easily with them.)
I will get back to you on this once I have figured out what's going on (I suspect that it's a difference in the way the new resolve code works, it may stop too short now to figure out that there is a 'better' package to choose for resolving the dependency).
In the old code, resolvedeps got the whole target list, now it gets only the first (already resolved) part of that list. Bye
Nagy Gabor wrote:
Nagy Gabor wrote:
Hi!
I know that I speak too late, but I haven't time/motivation to review this patch nowadays.
1. See alpm/sync.c/sync_prepare(). When user decides to remove unresolvable targets from the list, our code doesn't remove any targets from target->packages, just adds the pulled dependencies. The actual removal is done in the sortbydeps part (sortbydeps is called with list param!). This works, but it is hard to understand (imho), and it is a memleak: some pmsyncpkg_t structs magically disappeared.
I don't think that it's confusing that it happens this way; the resolve operation builds up a new list of packages for the transaction, and that list is then used to replace the trans->packages list. This is opposed to before where the resolve operation modified trans->packages in place.
You are right that there is a memory leak there, though; my change failed to free up any pmsyncpkg_t structs which were in trans->packages but were not in the replacement list. I will submit a small and simple patch which fixes this issue shortly.
OK, but please insert a short comment somewhere to make our code more readable. (The unresolvable targets will be removed later...)
I have submitted a patch which fixes this memory leak and which adds the comment you requested. Thanks for detecting this bug, I never would have thought of it myself.
sync993 does not exist in the pacman sources. If it did, I would have seen this problem and addressed it.
That's why I posted it here :) I've just created it to show the problem. (I like pactests, because we can test our patches easily with them.)
Oh, I see, I was confused about that. I will add your sync993.py file to my own change to fix the issue if you don't mind.
I will get back to you on this once I have figured out what's going on (I suspect that it's a difference in the way the new resolve code works, it may stop too short now to figure out that there is a 'better' package to choose for resolving the dependency).
In the old code, resolvedeps got the whole target list, now it gets only the first (already resolved) part of that list.
Well I've examined this issue in more detail and it's quite complicated. Basically, the old code (before my change) used to first check to see if any package in the list of packages to upgrade satisfied a dependency before going out to look for a package to resolve the dependency in the sync database. The new code doesn't do that, it resolves each package individually without regards to the target list of packages to upgrade. The old code hade the nice side benefit of allowing a user to "force" which package to use to satisfy a dependency by putting that package on the command line. This is the behavior that you are testing with your sync993.py test. The new code resolves each package individually and independently of the target-list. It is for this reason that the first package found during resolve, the one in the sync database, is being used instead of the one on that target-list, so pkg2 is getting pulled in to satisfy a dependency of pkg1 that pkg3 would also have satisfied, and pkg3 is also getting installed because it's in the target-list. Unfortunately, the obvious fix of first looking through the target list for packages which can resolve dependencies does not work - because it might find packages which satisfy the dependency but which themselves have not yet been resolved. And if it then uses those unresolved packages to satisfy dependencies, and later on it is discovered that those packages are not themselves resolvable (because of IgnorePkg/IgnoreGroup), then the result will be the inclusion of unresolvable packages in the transaction, which then fails. A shorter way to say this is: we cannot resolve dependencies just by looking in the target-list to find them, because the target-list packages have not necessarily been resolved yet, and we can only resolve dependencies with packages which themselves have been resolved. The big problem is that to satisfy dependencies when more than one package provides that dependency but only one of those packages can actually be installed, requires a different approach than the existing code base (both before and after my changes) takes. It requires a way to resolve a package at the time that it itself is picked as the resolution of a dependency, which basically means some kind of recursion in package resolution. But I am sure that nobody wants to add recursion to the package resolution code due to the added complexity. I think that to some degree it's not really clear that specifying a package in the target-list should mean that that package should be used first to satisfy dependencies over other packages in the sync database which could also satisfy the same dependencies. The general issue with the code base, both before and after my changes, is that it cannot look past the first package that it found to resolve a dependency to see if other packages might be better candidates. It always just resolves a dependency with the first satisfying package that it finds. The difference between the old and new code is that the old code allowed the user to control the dependency resolution process manually on the command line, although I'm not entirely sure that many pacman users are aware that they can do this or that it is a means for solving such unusual dependency problems. A question: is the behavior of first searching the target-list for packages to satisfy dependencies a documented and expected behavior of pacman? Thanks, Bryan
sync993 does not exist in the pacman sources. If it did, I would have seen this problem and addressed it.
That's why I posted it here :) I've just created it to show the problem. (I like pactests, because we can test our patches easily with them.)
Oh, I see, I was confused about that. I will add your sync993.py file to my own change to fix the issue if you don't mind.
I don't mind. :)
I will get back to you on this once I have figured out what's going on (I suspect that it's a difference in the way the new resolve code works, it may stop too short now to figure out that there is a 'better' package to choose for resolving the dependency).
In the old code, resolvedeps got the whole target list, now it gets only the first (already resolved) part of that list.
Well I've examined this issue in more detail and it's quite complicated.
Basically, the old code (before my change) used to first check to see if any package in the list of packages to upgrade satisfied a dependency before going out to look for a package to resolve the dependency in the sync database.
The new code doesn't do that, it resolves each package individually without regards to the target list of packages to upgrade.
The old code hade the nice side benefit of allowing a user to "force" which package to use to satisfy a dependency by putting that package on the command line. This is the behavior that you are testing with your sync993.py test.
The new code resolves each package individually and independently of the target-list. It is for this reason that the first package found during resolve, the one in the sync database, is being used instead of the one on that target-list, so pkg2 is getting pulled in to satisfy a dependency of pkg1 that pkg3 would also have satisfied, and pkg3 is also getting installed because it's in the target-list.
The "already-resolved" target list (== *packages list) is checked first. So the resolving is not independent from the target list, however, it is true that resolvedeps cannot see the *whole* target list now. For example, if you change the command line to "-S pkg3 pkg1" in my pactest, it will pass. (Btw, the order of packages in the target list should not affect the result of resolvedeps!)
Unfortunately, the obvious fix of first looking through the target list for packages which can resolve dependencies does not work - because it might find packages which satisfy the dependency but which themselves have not yet been resolved. And if it then uses those unresolved packages to satisfy dependencies, and later on it is discovered that those packages are not themselves resolvable (because of IgnorePkg/IgnoreGroup), then the result will be the inclusion of unresolvable packages in the transaction, which then fails.
A shorter way to say this is: we cannot resolve dependencies just by looking in the target-list to find them, because the target-list packages have not necessarily been resolved yet, and we can only resolve dependencies with packages which themselves have been resolved.
I don't see the problem. Then resolvedeps will "recursively" resolve the pulled package from target list without any code modification, and when we want to resolve that package (in a new resolvedeps call from sync_prepare), resolvedeps will detect that the package is already in *packages list. I mean, you may want to add a new param, "lookfirst" (== target list). However, I hoped that you have a nicer idea, that's why I didn't send a patch.
A question: is the behavior of first searching the target-list for packages to satisfy dependencies a documented and expected behavior of pacman?
Not documented, but expected :). Moreover, see my comment above about the current asymmetric behavior, that is quite odd imho. -- new suggestions :) [optional] -- 3. Now it can happen, that a missing dependency is added to the *data list many times. For example, if there is a popular unresolvable package in our repo, let's say gtk2, and we have 10 packages in the target list depend on that, then we get "gtk2 requires foo" error 10 times (if user doesn't want to remove unresolvable packages). So we may want to check *data list before adding a new missing dependency to avoid duplication. 4. One more comment (I've just realised this during writing the previous one). I never liked our unresolvable dependencies errors: User does "pacman -S foo1 foo2 foo3" and he may get "error: bar requires baz", and he says "What?". Now, after your patch we can easily say, what package "induced" that error (pkg param of resolvedeps). We have a field causingpkg in pmdepmissing_t, we may want to use that for this purpose. (With -R that field has an other meaning, so this may be not a good idea.) Or we can print this info via PM_TRANS_CONV_REMOVE_PKGS. If we implement 4., we can forget 3. Ovbiously, if you don't like these ideas, you have nothing to do :) (There are no bugs here.) Bye
On Sun, Mar 8, 2009 at 2:42 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
A question: is the behavior of first searching the target-list for packages to satisfy dependencies a documented and expected behavior of pacman?
Not documented, but expected :). Moreover, see my comment above about the current asymmetric behavior, that is quite odd imho.
I completely agree.
-- new suggestions :) [optional] --
3. Now it can happen, that a missing dependency is added to the *data list many times. For example, if there is a popular unresolvable package in our repo, let's say gtk2, and we have 10 packages in the target list depend on that, then we get "gtk2 requires foo" error 10 times (if user doesn't want to remove unresolvable packages). So we may want to check *data list before adding a new missing dependency to avoid duplication.
+1
4. One more comment (I've just realised this during writing the previous one). I never liked our unresolvable dependencies errors: User does "pacman -S foo1 foo2 foo3" and he may get "error: bar requires baz", and he says "What?". Now, after your patch we can easily say, what package "induced" that error (pkg param of resolvedeps). We have a field causingpkg in pmdepmissing_t, we may want to use that for this purpose. (With -R that field has an other meaning, so this may be not a good idea.) Or we can print this info via PM_TRANS_CONV_REMOVE_PKGS.
+1 I just found a funny test case on my first try. Funny because besides the same missing dependency message being printed many times, there is also a "provider package was selected" printed multiple times, which makes the whole thing worse. If I understood you correctly, all these missing dependency messages look the same because we are missing one information : the "causingpkg", which is a target package that has an unresolvable dependency, correct? So with that additional info, all these missing dependency messages would be different. It seems fine to me to reuse the causingpkg field for that, because that name is quite general. We would just need to document well what it means in -R case, and what it means in -S/-U case.
sudo LANG=C pacman -S kde --ignore libxinerama kde package not found, searching for group... :: group kde (including ignored packages): kdeaccessibility kdeadmin kdeartwork kdebase kdebase-runtime kdebase-workspace kdeedu kdegames kdegraphics kdelibs kdemultimedia kdenetwork kdepim kdepimlibs kdeplasma-addons kdesdk kdetoys kdeutils kdewebdev :: Install whole content? [Y/n] resolving dependencies... warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" :: the following package(s) cannot be upgraded due to unresolvable dependencies: kdeaccessibility kdeadmin kdeartwork kdebase kdebase-runtime kdebase-workspace kdeedu kdegames kdegraphics kdelibs kdemultimedia kdenetwork kdepim kdepimlibs kdeplasma-addons kdesdk kdetoys kdeutils kdewebdev
Do you want to skip the above package(s) for this upgrade? [Y/n] looking for inter-conflicts... local database is up to date
I just found a funny test case on my first try. Funny because besides the same missing dependency message being printed many times, there is also a "provider package was selected" printed multiple times, which makes the whole thing worse. If I understood you correctly, all these missing dependency messages look the same because we are missing one information : the "causingpkg", which is a target package that has an unresolvable dependency, correct?
Yes.
So with that additional info, all these missing dependency messages would be different. It seems fine to me to reuse the causingpkg field for that, because that name is quite general. We would just need to document well what it means in -R case, and what it means in -S/-U case.
sudo LANG=C pacman -S kde --ignore libxinerama kde package not found, searching for group... :: group kde (including ignored packages): kdeaccessibility kdeadmin kdeartwork kdebase kdebase-runtime kdebase-workspace kdeedu kdegames kdegraphics kdelibs kdemultimedia kdenetwork kdepim kdepimlibs kdeplasma-addons kdesdk kdetoys kdeutils kdewebdev :: Install whole content? [Y/n] resolving dependencies... warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" :: the following package(s) cannot be upgraded due to unresolvable dependencies: kdeaccessibility kdeadmin kdeartwork kdebase kdebase-runtime kdebase-workspace kdeedu kdegames kdegraphics kdelibs kdemultimedia kdenetwork kdepim kdepimlibs kdeplasma-addons kdesdk kdetoys kdeutils kdewebdev
Do you want to skip the above package(s) for this upgrade? [Y/n] looking for inter-conflicts... local database is up to date
Wow, this is weird. And now I am a bit unsure about my "warning: ignoring foo..." patch. :) By the way, I don't think those "warning: cannot resolve libxinerama" messages are useful, these kind of infos should be collected "at the end" (using *data list or whatever). In fact, now the "provider package was selected" can be false, because we may finally remove that package (freeglut) from the target list. So that message should be completely removed imho. (However, that message was introduced when I switched to _alpm_resolvedep usage in sync_addtarget... Now I am not sure that was a good idea, see its prompt param. But if we keep this resolvedep stuff, we can detect providers comparing depend->name and target->name in sync_addtarget.) The main problem is (which was predicted in Bryan's first "Another idea" mail), that resolvedeps want to resolve the same dependency path many times (which is slower), which is not successful here... Bye
-- new suggestions :) [optional] --
3. Now it can happen, that a missing dependency is added to the *data list many times. For example, if there is a popular unresolvable package in our repo, let's say gtk2, and we have 10 packages in the target list depend on that, then we get "gtk2 requires foo" error 10 times (if user doesn't want to remove unresolvable packages). So we may want to check *data list before adding a new missing dependency to avoid duplication.
+1
4. One more comment (I've just realised this during writing the previous one). I never liked our unresolvable dependencies errors: User does "pacman -S foo1 foo2 foo3" and he may get "error: bar requires baz", and he says "What?". Now, after your patch we can easily say, what package "induced" that error (pkg param of resolvedeps). We have a field causingpkg in pmdepmissing_t, we may want to use that for this purpose. (With -R that field has an other meaning, so this may be not a good idea.) Or we can print this info via PM_TRANS_CONV_REMOVE_PKGS.
+1
I just found a funny test case on my first try. Funny because besides the same missing dependency message being printed many times, there is also a "provider package was selected" printed multiple times, which makes the whole thing worse. If I understood you correctly, all these missing dependency messages look the same because we are missing one information : the "causingpkg", which is a target package that has an unresolvable dependency, correct? So with that additional info, all these missing dependency messages would be different. It seems fine to me to reuse the causingpkg field for that, because that name is quite general. We would just need to document well what it means in -R case, and what it means in -S/-U case.
sudo LANG=C pacman -S kde --ignore libxinerama kde package not found, searching for group... :: group kde (including ignored packages): kdeaccessibility kdeadmin kdeartwork kdebase kdebase-runtime kdebase-workspace kdeedu kdegames kdegraphics kdelibs kdemultimedia kdenetwork kdepim kdepimlibs kdeplasma-addons kdesdk kdetoys kdeutils kdewebdev :: Install whole content? [Y/n] resolving dependencies... warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: cannot resolve "libxinerama", a dependency of "qt" warning: provider package was selected (freeglut provides glut) warning: cannot resolve "libxinerama", a dependency of "qt" :: the following package(s) cannot be upgraded due to unresolvable dependencies: kdeaccessibility kdeadmin kdeartwork kdebase kdebase-runtime kdebase-workspace kdeedu kdegames kdegraphics kdelibs kdemultimedia kdenetwork kdepim kdepimlibs kdeplasma-addons kdesdk kdetoys kdeutils kdewebdev
Do you want to skip the above package(s) for this upgrade? [Y/n] looking for inter-conflicts... local database is up to date
Bump on these. As I see, 1-2 are fixed now (mentioned in the first mail of this thread). But the "duplicated messages/data-list" issue is still here. After my "Print warning in _alpm_resolvedep() if a satisfier package is ignored without QUESTION" patch this issue became even worse, maybe that patch should be reverted, I dunno... The most precise [but ugly source-code] solution would be the following (imho): Maintain a dropped list for dropped (unresolvable) packages. This would prevent duplicated work (and duplicated messages), and this would speed-up our algorithm a bit in xav's example. Bye
Nagy Gabor wrote:
Bump on these. As I see, 1-2 are fixed now (mentioned in the first mail of this thread). But the "duplicated messages/data-list" issue is still here. After my "Print warning in _alpm_resolvedep() if a satisfier package is ignored without QUESTION" patch this issue became even worse, maybe that patch should be reverted, I dunno...
The most precise [but ugly source-code] solution would be the following (imho): Maintain a dropped list for dropped (unresolvable) packages. This would prevent duplicated work (and duplicated messages), and this would speed-up our algorithm a bit in xav's example.
Bye
Hi, I was on vacation in Australia for 3 weeks and am in the process of moving now, so I'm really out of the loop here. I can barely remember the issues involved here. I believe I submitted two patches before I left that solved some of the issues that were pointed out by Nagy. Have these been accepted into the pacman sources? Nagy, are you running your test with these patches? If so, then I agree that the final problems that exist are ugly, but fortunately just cosmetic. I also continue to believe that the warning message you've added about ignored packages is unnecessary. The user knows what packages they've put in their ignore list and that these packages may be ignored at any time as dependency satisfiers. I don't think they need to be warned about it. At the very least, it should be an optional warning turned on by a command-line flag. That's my opinion anyway. If your warning is taken out, what does the new output of pacman look like in the situation you are reporting? I imagine there are still some duplicated messages, but what exactly? Thanks, Bryan p.s. I'm moving back to the USA and starting a new job this week so I may not be very responsive to the list for a while ... my apologies in advance.
Nagy Gabor wrote:
Bump on these. As I see, 1-2 are fixed now (mentioned in the first mail of this thread). But the "duplicated messages/data-list" issue is still here. After my "Print warning in _alpm_resolvedep() if a satisfier package is ignored without QUESTION" patch this issue became even worse, maybe that patch should be reverted, I dunno...
The most precise [but ugly source-code] solution would be the following (imho): Maintain a dropped list for dropped (unresolvable) packages. This would prevent duplicated work (and duplicated messages), and this would speed-up our algorithm a bit in xav's example.
Bye
Hi, I was on vacation in Australia for 3 weeks and am in the process of moving now, so I'm really out of the loop here. I can barely remember the issues involved here. I believe I submitted two patches before I left that solved some of the issues that were pointed out by Nagy. Have these been accepted into the pacman sources? Nagy, are you running your test with these patches?
Your "Look in target-list first to resolve dependencies" and "Remove duplicates from the unresolvable list before..." patches were accepted.
If so, then I agree that the final problems that exist are ugly, but fortunately just cosmetic.
I also continue to believe that the warning message you've added about ignored packages is unnecessary. The user knows what packages they've put in their ignore list and that these packages may be ignored at any time as dependency satisfiers. I don't think they need to be warned about it. At the very least, it should be an optional warning turned on by a command-line flag. That's my opinion anyway.
If your warning is taken out, what does the new output of pacman look like in the situation you are reporting? I imagine there are still some duplicated messages, but what exactly?
The problem is that pacman may want to pull a package many times (without success, of course), and all the warnings around this package may be echoed many times: a. provider package was selected b. cannot resolve "foo", a dependency of "bar" c. ignoring package [NEW]. Basically, I think that a. is useful, because pacman's automatic provision selection is not always suitable for the user [libgl], b. can be simply removed (finally we always print a nice error message, which should be tuned using causingpkg, read back ;-), c. may be unnecessary (But I don't really like when pacman is totally silent about IgnorePkg. Don't forget that we also have IgnoreGroup.). My standpoint is that some of these messages are useful, but at most once ;-). I have no clear solution for this issue, we may have to make some compromises... Bye
Nagy Gabor wrote:
The new code resolves each package individually and independently of the target-list. It is for this reason that the first package found during resolve, the one in the sync database, is being used instead of the one on that target-list, so pkg2 is getting pulled in to satisfy a dependency of pkg1 that pkg3 would also have satisfied, and pkg3 is also getting installed because it's in the target-list.
The "already-resolved" target list (== *packages list) is checked first. So the resolving is not independent from the target list, however, it is true that resolvedeps cannot see the *whole* target list now. For example, if you change the command line to "-S pkg3 pkg1" in my pactest, it will pass. (Btw, the order of packages in the target list should not affect the result of resolvedeps!)
I agree, but this is very tricky now that we allow for some packages in the target-list to be removed from the transaction by the user *after* the resolve step is done. Because now not all of the packages in the target-list are guaranteed to survive the resolve step, so it's not a good idea to pick one of them during the resolve.
Unfortunately, the obvious fix of first looking through the target list for packages which can resolve dependencies does not work - because it might find packages which satisfy the dependency but which themselves have not yet been resolved. And if it then uses those unresolved packages to satisfy dependencies, and later on it is discovered that those packages are not themselves resolvable (because of IgnorePkg/IgnoreGroup), then the result will be the inclusion of unresolvable packages in the transaction, which then fails.
A shorter way to say this is: we cannot resolve dependencies just by looking in the target-list to find them, because the target-list packages have not necessarily been resolved yet, and we can only resolve dependencies with packages which themselves have been resolved.
I don't see the problem. Then resolvedeps will "recursively" resolve the pulled package from target list without any code modification, and when we want to resolve that package (in a new resolvedeps call from sync_prepare), resolvedeps will detect that the package is already in *packages list.
I mean, you may want to add a new param, "lookfirst" (== target list). However, I hoped that you have a nicer idea, that's why I didn't send a patch.
Your new param "lookfirst" is exactly the approach that I tried to fix this problem (although I called the parameter "preferred", but it's the same idea), and it didn't work. It solved the problem for your sync993.py test, but it caused failures in other tests, most notably my ignoreXXX.py tests. However, I believe that the cause was a mistake in my patch, which did not add the package that it found in the "lookfirst" list to the "*packages" list, and continue to try to resolve those. I will try my patch again with this fix and see if it works properly.
A question: is the behavior of first searching the target-list for packages to satisfy dependencies a documented and expected behavior of pacman?
Not documented, but expected :). Moreover, see my comment above about the current asymmetric behavior, that is quite odd imho.
-- new suggestions :) [optional] --
3. Now it can happen, that a missing dependency is added to the *data list many times. For example, if there is a popular unresolvable package in our repo, let's say gtk2, and we have 10 packages in the target list depend on that, then we get "gtk2 requires foo" error 10 times (if user doesn't want to remove unresolvable packages). So we may want to check *data list before adding a new missing dependency to avoid duplication.
Definitely. I will submit a patch for this too.
4. One more comment (I've just realised this during writing the previous one). I never liked our unresolvable dependencies errors: User does "pacman -S foo1 foo2 foo3" and he may get "error: bar requires baz", and he says "What?". Now, after your patch we can easily say, what package "induced" that error (pkg param of resolvedeps). We have a field causingpkg in pmdepmissing_t, we may want to use that for this purpose. (With -R that field has an other meaning, so this may be not a good idea.) Or we can print this info via PM_TRANS_CONV_REMOVE_PKGS.
That sounds good. Thanks, Bryan
Nagy Gabor wrote:
Hi!
I know that I speak too late, but I haven't time/motivation to review this patch nowadays.
1. See alpm/sync.c/sync_prepare(). When user decides to remove unresolvable targets from the list, our code doesn't remove any targets from target->packages, just adds the pulled dependencies. The actual removal is done in the sortbydeps part (sortbydeps is called with list param!). This works, but it is hard to understand (imho), and it is a memleak: some pmsyncpkg_t structs magically disappeared.
I don't think that it's confusing that it happens this way; the resolve operation builds up a new list of packages for the transaction, and that list is then used to replace the trans->packages list. This is opposed to before where the resolve operation modified trans->packages in place.
You are right that there is a memory leak there, though; my change failed to free up any pmsyncpkg_t structs which were in trans->packages but were not in the replacement list. I will submit a small and simple patch which fixes this issue shortly.
OK, but please insert a short comment somewhere to make our code more readable. (The unresolvable targets will be removed later...)
Oh, I was blind, there is a good comment in the code :-) "/* 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 */" So simply forget what I said here about comments. Bye
participants (3)
-
Bryan Ischo
-
Nagy Gabor
-
Xavier