Tim Foster wrote: > On Wed, 2010-04-28 at 16:42 -0700, Danek Duvall wrote: > > Tim Foster wrote: > > > > > http://cr.opensolaris.org/~timf/pkgdepend-smf > > > smf_manifest.py: > > > > - line 236ff: why not just > > > > try: > > fmris = set(get_smf_dep...) > > except: > > ... > > That's ok, so long as I set fmris before the try..except clause. > (more below)
I think we both meant "dep_fmris" there, but yes, I see your point. Still, you don't need the if. > > - line 247ff: the flow in this for loop is weird. You set manifest > > to None so that you can test it for that on 253, but at that point > > you'd append to the elist twice. > > That's the same reason as above - if I raise an exception trying to > resolve dependencies, then "manifest" isn't a declared name, and so I'd > get a NameError on line ~ 256. I'm not worried about potentially adding > two entries to the elist (in fact, if resolve_smf_dependency(..) raises > an exception, I'm quite interested in being able to show that) But why show it twice? Or are you simply dealing with the possibility that resolve_smf_dependency() returns None, in addition to the one where it raises a ValueError? > > - line 268: I'd change this to "opensolaris.smf.fmri". In general, we > > try to avoid underscores in attribute names. I might suggest > > opensolaris.smf-fmri (which is the usual "fix"), but I could see other > > SMF-related metadata being added. > > I was following the style in pkg-gate/doc/tags-and-attributes.txt - we > should probably do a clean up of that documentation if underscores are to > be discouraged. I've changed the attribute name to > 'opensolaris.smf.fmri'. (I've seen other draft documentation from sch > that uses underscores in attribute names as well - so we need to decide.) Alan's working on a bit of a cleanup of that document, but I hadn't asked him to do that; perhaps that's worthwhile. There are a bunch of existing attributes with underscores which will be a bit tricky to change, and a bunch of proposed attributes with underscores which we can just avoid. > (re. a comment in the source code talking about a broken XML file) > > - line 297: could you file a bug for this, please? There are a bunch of > > others that have the same problem. > > [ ... ] > > It'd take a while to go through the entire list though. I've submitted > defect 15944 and removed the file reference in the comment (which does > appear to have been fixed in nv_138), leaving the explanation as is. Cool, thanks. > > importer.py: > > > > - line 174: once this if triggers, you can break out of the loop. In > > fact, you can set bundle and then break out, and not bother with > > bundle_data. > > Won't it complain if I try to alter a variable I'm currently iterating over? You're not modifying the object you're iterating over, you're just binding a different object to the same name and then immediately breaking out of the iteration. I expect it'll be just fine. > > It might be a useful enhancement to allow search to find appropriate > > subsets of SMF FMRI names. svc:/network/physical:nwam would be found by > > doing a search for any of: > > > > nwam > > physical > > physical:nwam > > network/physical > > network/physical:nwam > > svc:/network/physical > > svc:/network/physical:nwam > > > > The instance name "default" could be a special case that's ignored. > > Arguably, a user could do this already using pkg search wildcards > - do you think it's worth complicating search with specifics about SMF? Not sure, though I think that when people are used to typing "svcadm restart nwam" or "svcadm enable -t network/physical:default", they'll be a bit annoyed if those don't just show up in search. But until we devise better guidelines, we can suggest wildcarding as a workaround. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
