[pacman-dev] Some comments on Bryan patches (bugs)
Bryan Ischo
bji-keyword-pacman.3644cb at www.ischo.com
Fri Mar 6 23:30:26 EST 2009
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
More information about the pacman-dev
mailing list