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

[Bryan Ischo]

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