Brock Pytlik wrote: > Just to be sure, the way the version testing works is that if an > incorporation dependency is specified on [EMAIL PROTECTED] for example > [EMAIL PROTECTED] would not work, but [EMAIL PROTECTED] would, correct? >
This is correct; the easy way to think of it is one of significant figures; the incorporation defines [EMAIL PROTECTED] to 3 significant "digits" of precision; within that measurement tolerance, [EMAIL PROTECTED] is equivalent but [EMAIL PROTECTED] is not. > constraint.py > Nit 74: space after # > ok > 88: Is this just checking whether c.combine(constraint) raises a > constraint exception? > Yes; we need to make sure that the new constraint is compatible w/ any existing ones. In contrast to the previous implementation, this code keeps constraints distinct per package, so optional constraints and incorporation(s) can both be enforced, and error messages about constraint violations can correctly identify the package containing the conflicting constraint. [see comment at eom] > 90-95: I don't understand why this is right. What if an incorporation > dependency on [EMAIL PROTECTED] is already in self.constraints, and the > argument > to update_constraints is an incorporation dependency on [EMAIL PROTECTED] If > I > understood combine right, this will pass the check on line 88, but then > the constraint stored in self.contratints will be loosened to 1.2. What > am I missing? :) > Note that the code replacing the existing constraint (91-94) does so only for a new version of the same package; an additional (looser) constraint could be added onto the list of constraints on this package. Any changes to a package that has constraints needs to meet all of them. I'll comment this code more heavily, and add a demonstrative test case. > 99: I think I understand what's happening here, but a doc string would > be very helpful I think. Done. > Also, couldn't lines 103-104 be written as: > cl.append(constraint) > return reduce(constraint_combine, cl) > I'd probably prefer that approach, but if you prefer the other I have no > objections. > That would need to be cl[:].append(constraint) since otherwise I'd modify the stored copy... that seemed ugly to me... > 107: Have I got this right? This function takes an fmri, and applies the > available constraints If the fmri isn't allowed b/c a constraint has > NEVER set, then it catches the ConstraintException and raises one with > better information. If the provided fmri doesn't have a high enough > minimum version, then a new fmri with the version bumped is returned? > Basically, what happens in this: We convert an FMRI into a Required min_fmri = xxx, max_fmri = None constraint, which is how this routine is called. We then try to merge w/ any applicable installed constraints which tosses an exception if a conflict is present. If we get the same constraint back (eg our fmri was more specific than the constraint), or there was no constraint (None), we return the original FMRI. If the constraints required a more specific version of the fmri, we cons up that and return it. > 111-117: Comments or more verbose would makes this much more readable I > think > Done. > 174+ check_for_work: Should this function do any verification that the > fmri it's passed matches the fmri it is a constraint on? I'm just > wondering if a check (or assertion or something) that self.pkg_name == > fmri_present.pkg_name would be a safety check. Sure.... added. > > 196-197: I think these mean that the relative ordering of the constants > defined on lines 124-127 matters, and the the constants aren't > independent of each other. If that's the case, could you add a comment > to that effect around 124? Ok. > > image.py > Nit 46-64: while this looks nicer, i think it'll be a headache to > maintain as we add or remove imports Hmmm - I found sorting the list made things easier to find. You don't like the spaces? > 1657: Does the spinner still work reasonably with this line removed? I > didn't see it replaced anyplace similar. Load_optional_dependencies (and > this seems to have a similar overhead) took enough time that having the > spinner going during it was a win I think. Mismerge on my part; fixed. > 2091: This feels wrong to me, but maybe I'm the only one. Perhaps you could be more explicit? > 2151, 2154: Should 2154 be constraint_violations=constraint_violations > or should 2151 be removed? Good catch; I changed my mind half way through, I think. > > t_pkg_api_install.py: > 476:I don't understand the changes made here. This should still fail > during plan creation unless I've misunderstood something. The problem was we were calling api_obj.execute_plan() even though there was nothing to do; this meant we set the result code twice and blew up in the history modules. Failing in plan_creation is fine, we just need to handle the case when there's nothing to do correctly. > > Nits: > > depend.py: > 196: is there a more descriptive name than parse2 maybe? > Can parse be completely replaced by parse2? Seems like it returns a > superset of the information that parse does. I thought about that. I'll change the code in verify to use the new parse, and change it's name. > My inclination is to say that constructing the strings in 248-254 isn't > the right place to do that, but I wouldn't hold up the putback for that. > Moved into __str__. > api.py: > 345: ']' should line up with the p in pkg_list if I've finally got the > style for these things down > fixed > api_errors.py: > 126, 131: Extra white space > removed; newlines left in place > imageplan.py: > 275: extra whitespace > removed; newlines left in place Thanks for looking at this! New webrev posted in the same place: http://cr.opensolaris.org/~barts/incorp/ This implementation has a known bug I discovered by inspection while writing doc strings for Brock's comments: it doesn't deal correctly w/ a single package defining multiple incorporation dependencies on a another single package, and incorporation packages that drop dependencies when upgraded via a require dependency... Neither case happens in OpenSolaris right now, but I'll fix this tomorrow by flushing constraints defined by all older versions of a package being evaluated, and I'll add test cases to cover this. - Bart -- Bart Smaalders Solaris Kernel Performance [EMAIL PROTECTED] http://blogs.sun.com/barts "You will contribute more with mercurial than with thunderbird." _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
