On 08/30/12 09:14, Shawn Walker wrote:
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.
I really think this is the better solution. I don't think having a
function that sometimes returns something that can be modified and
sometimes something that's been cached is good code structure. I don't
really think it's a riskier change either.
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.
Yes, but not many functions (none that I can think of off the top of my
head) sometimes return something mutable and sometimes return a copy. If
you're not going to do the optimization now (and you're going to stick
with this method of fixing the bug), then please file an RFE for the
optimization.
Brock
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
