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