[pacman-dev] Some comments on Bryan patches (bugs)

[Bryan Ischo]

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