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
_______________________________________________
pacman-dev mailing list
[email protected]
http://archlinux.org/mailman/listinfo/pacman-dev
