All comments not discussed here accepted as written. Danek Duvall wrote: > On Fri, Jan 16, 2009 at 04:49:00PM -0800, Bart Smaalders wrote: > >> http://cr.opensolaris.org/~barts/FatPkgs/ > > client.py: > > - line 1847: I'd just as soon that -v be reserved for verbose. We don't > have that for image-create right now, but it's not impossible. I'd use > -V, I guess. >
I've eliminated -v; --no-refresh also has no short form option > attribute.py: > > - line 48: I thought you were going to get rid of the short form of this > action. I think it's actually necessary, now, as > > set bob=fred > > is ambiguous in some sense -- is it a set attribute that's been tagged > with the "bob" attribute, but missing the actual attribute data, or is > it just the short form of > > set name=bob value=fred I now assert if name or value is missing, so short-form still works. > api_errors.py: > > - line 109: You can't create objects as default arguments -- they get > created once and reused, which is not likely what you meant. You need > to set each to None, and then in the body of the method, test for None > and set to the empty list. It's also not clear what your comment on > 111 means. I've decided to use an empty tuple instead, which is immutable. I've added code to misc.py to implement immutable dicts as well. > badarch is treated unlike the other plan creation problems. The rest > allow multiple packages to fail plan creation, which is nice because > you don't have to modify your plan one package at a time to find out > all the bad packages. It's not clear to me that it can be any other > way, the way you raise the exception, but it might be good to consider > this when doing the SAT solver work, to see if there's a way to return > all the arch-broken packages you can in one shot. > The SAT solver should be able to catch this earlier... also, having a way to get dependencies and set actions w/o loading all manifests will speed this up a lot. > pkgplan.py: > > - line 154, 155: commented-out? I've removed filter processing for now; this needs to be done using callables ala variants. > > variant.py: > > - line 31: same problem as in api_errors.py. Though shouldn't this be a > full (self, *args, **kwargs), which you then pass on to the superclass > constructor? Wouldn't that obviate the need for the loop on 34,35? > This constructor makes sure that Variants form of __setitem__ is called, so loop is needed. > manifest.py: > > - line 121: Same problem here as in api_errors.py. Plus you might switch > the exclude lists so they match the order of the required arguments. > > I wonder, though, given how much exclude lists are passed around, if > Manifest shouldn't just have an exclude list as a member, and that > unless you explicitly ask for an unfiltered manifest, you always get it > filtered (but the unfiltered one is what's stored in memory). Or do > you need to run different filters at different times on the same > manifest? > The latter case; changing filters, facets, or variants on an installed package requires this. > I also wonder how much our memory footprint will increase once we're > manipulating fat packages. The manifests should be, what, a little > less than twice the size? > Yup. I've updated the webrev if anyone wants another look... - 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
