On 08/29/12 17:35, Brock Pytlik wrote:
On 08/29/12 16:58, Shawn Walker wrote:
Greetings,
The following webrev contains fixes for the following issue:
7194891 pkg solver operations can omit package dependencies
https://cr.opensolaris.org/action/browse/pkg/swalker/pkg-solver/webrev/
I've verified that this resolves the original issue described in the
bug. Unfortunately, despite many hours of trying to devise a test
case, I've been unable to create one.
Best I can tell, this bug only occurs whenever a certain chain of
dependencies is parsed an even number of times, which causes the
dependency list to be reversed at a point where the FMRI ids (position
in possible dict) is used to generate the SAT solver clauses internally.
The good new is that it's a tiny fix and seems pretty obvious that it
is the right one.
Once reviewed, I will push to both the update branch and to the
default one.
...
I'd like a bit more of a comment on line 1077 and 1131. Specifically, I
think it should mention that the reason we have to make a copy is that
the list has been cached in possible_dict. I'm also curious about two
other potential solutions/optimizations weren't chosen. First, why not
just have __get_catalog_fmris return a copy of the list itself. That
seems safer and better code structure (since the code that's making the
copy is the code that knows the list has been cached in the dictionary).
I considered that option, but didn't pursue it because it felt like a
potentially riskier change. Not only that, these are the only two cases
(that I could discover) that expected to be able to modify the list, so
imposing the (admittedly minor) overhead of a copy on every caller
seemed overkill.
If for some reason that solution doesn't work, then I would think it
makes sense to change lines 1078 and 1132 so that a copy is made only in
the case where self.__trimdone is true. If those lines are going to know
that the list may have been cached in a dictionary, then there's no
reason for them not to determine for certain whether or not that's
happened.
I'd rather not be overly clever about optimisations here as this bug is
esoteric enough as it is, plus if we change that function we'd then have
to potentially remove the trimdone qualifier. As for the lines "going
to know that the list may have been cached", I think that's actually a
pretty standard thing. Some functions in Python are the same way; some
return something mutable, some do not. I don't think there's anything
particularly special about that.
I have updated the comments as follows:
# Always use a copy; return value may be cached.
I do plan on filing a bug to have the solver use more immutable data
structures.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
