Nagy Gabor wrote:
Basically this is a neat (and readable) job imho.
Thanks for your kind words and your input. Comments inline below.
Some comments: 1. There is a typo in the description of unresolvable field.
Check, will fix.
2. A new list for pkginfos looks ugly imho.
What do you mean by 'ugly'? Is it redundant given other data structures in libalpm? If so, I will be happy to switch to another data structure. Xavier already pointed out that pmgraph_t may fit the bill. I'll definitely look into it. Or maybe you meant something else by 'ugly'? I'm not sure ...
3. I like the concept of pulled, maybe (pmpkg_t *) pulledby would be even better, then we could print more informative (error) messages (see FS#12536 for example). So we again reached to a ~graph structure. (Hm. Maybe dependents can be also used instead of pulledby...)
This sounds like a good idea. But it would involve greater changes to libalpm then I felt comfortable doing. Unfortunately, when one is just a patch contributor rather than a real developer, one feels the need to keep all changes as minimal as possible, instead of reaching into lots of parts of the code and doing cleanups and refactoring and such. But I would be happy to do some of that, once I feel more comfortable with the code base.
4. It is not easy to determine which package is unresolvable. If a pulled dependency satisfier of foo is unresolvable we may could find an other (resolvable) satisfier. But this is not handled in the current code neither, so your _alpm_mark_unresolvable() is OK.
I see what you mean. Is this something that should/could be fixed in _alpm_resolvedeps? If so, perhaps it can be deferred to a separate bug incident and fixed separately from my change?
5. I may overlook something, but as I see, the following situation can happen: # pacman -S foo bar, where foo depends on bar. During resolvedeps foo may become unresolvable, but bar not. In this case pacman asks the user whether he wants to remove foo from target list. User answers yes. Since your _alpm_is_needed code (btw, _alpm prefix is not needed here) doesn't check pulled flag, it will determine that bar is not needed (which is not true, bar was an explicit package), and pacman will also remove bar as well, and the user wasn't informed about this.
I will remove _alpm prefix from my static functions, I hadn't read the code guidelines carefully enough to realize the convention, mea culpa. As to your problem situation: good catch, I missed that case in my thought process and in testing. Thanks for pointing it out, I will fix it (should be an easy fix in _alpm_is_needed).
6. I think a dependency cycle can lead to an infinite loop due to _alpm_is_needed.
I didn't realize that dependency cycles were possible. But you are right, they would lead to an infinite loop. I'll have to figure out some way to detect and break such cycles. Thanks for pointing that out to me. Best wishes, Bryan