seth vidal wrote:
Florian,
I looked through the diffs and there is just too much there to accept
as one patch.
Ehm, sorry. I thought I made that clear enough: This patch was never ever
intended to be accepted anywhere. This is a "tech preview", a first attempt,
a basis of discussion.
When we first went to the internal depsolver I said: "we'll be able to
get back a bunch of time b/c we won't have to do the lookups to
compensate for the ts.check() result being that stupid NEVR tuple."
In order to get to there we made a new object from the DepCheck that
would hand us the already-resolved out package object and what it
needed/conflicted with. That's what self._dcobj is. It's the DepCheck
class. Instead of ripping out huge portions of the depsolver it would be
better to take this a little at a time:
1. get rid of all the detection for figuring out where the requiring pkg
is coming from: the _proccesReq bits. you did that - good.
2. change the options passed to
requiringFromTransaction/requiringFromInstalled but nothing else. you
did part of that but then went too far.
3. after we have a handle on what we're getting back from DepCheck and
if it is sufficient on how to change things then we can work on
refactoring the code down some.
I dislike this DepCheck object approach. One reason this patch changes that
much is to examine how everything needs to work together. One result is that
there should not be just one tscheck call but the single checks depend on
the changes made to the transaction and need to be called on their own (see
the while True: loop in the new resolveDeps implementation). So this doesn't
fit that well with an central data container that is updated.
But there is another - much more important - reason: Depsolve._dcobj is
keeping state within the depsolver. There are currently several problems
caused by outdated depsolver vars. As plugins may change the transaction
state without the depsolver having a chance to notice any state in the
depsolver is endangered from being outdated. But things are going to be even
worse. If we start integrating more complex depsolving strategies that will
remove transaction members - like skipbroken or autoerase - into the
depsolver it has to be even more robust against changes during depsolving.
One strategy could be to change the .check* methods to generators. This
would allow restarting the dep check after every change to the transaction.
This could also simplify the decision making as we no longer have to filter
out problems of packages that are no longer part of the transaction. On the
other hand this might change the behavior of yum in some cases (not
installing a duplicate provider - but that isn't guaranteed anyway).
Even if we don't want to go that far we really should stay using return
values that we don't have to care about afterwards.
Otherwise we end up with another big jump in the depsolving code and
figuring out where we regressed becomes difficult.
I understand your desire to change things and make them happen "right
now" but it makes it impossible to follow all the changes and that makes
debugging much much harder for me. And since I'm having to sift
depsolving issues in bugzilla I need to be sure I know where this code
is going so I can answer better.
Although your perception that I am pushing a bit is right I won't try to get
something in that I am less confident about that the code I am replacing. I
also hope finding problems in the depsolver will be easier in the future as
a lot complexity got/is getting removed from it (all patches together reduce
size of depsolve.py by more than 300 lines).
So, in short, I need smaller chunks and smaller changes at a time to
move forward. As an example depsolve.py is 1259 lines of code and your
patch is 760 lines long.
Yes, this is perfectly clear. I will split the changes into single patches
as soon as it is clear what we are doing (and to what basis they will be
applied)
Florian
_______________________________________________
Yum-devel mailing list
Yum-devel@linux.duke.edu
https://lists.dulug.duke.edu/mailman/listinfo/yum-devel
