Hi Danek, Thanks very much for the code review, much appreciated.
I've been in touch with Brock separately about the upcoming '-d' option to 'pkgdepend generate' to add multiple proto areas, so more work will be needed on this changeset if that change goes back first. On Wed, 2010-04-28 at 16:42 -0700, Danek Duvall wrote: > Tim Foster wrote: > > > http://cr.opensolaris.org/~timf/pkgdepend-smf I've got a new webrev at http://cr.opensolaris.org/~timf/pkgdepend-smf-v2 with an incremental webrev at http://cr.opensolaris.org/~timf/pkgdepend-smf-v2-vs-v1 > smf_manifest.py: > > - Docstrings shouldn't start or end with a space (though the terminal > triple-quote can be on its own line if it won't fit in the 80 character > limit). Fixed. > - You should be wrapping strings in _() so they can be translated. Any > strings that take more than one argument should use dictionaries > instead of lists: > > _("Problem resolving %(fmri)s: %(err)s") % locals() > > or > > _("Problem resolving %(fmri)s: %(err)s") % {"fmri": fmri, "err": err} Fixed. > - line 64: you shouldn't need this declaration here, as you're not > modifying the name binding at all. Fixed. > - 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) > - line 249: bad indent Fixed. > - 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) > - line 267: why do this line and the next in two statements instead of > one: > > pkg_attrs = { > "opensolaris.smf_fmri": instance_mf.keys() > } Fixed. > - 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.) (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. This appears to be fixed in nv_138. I checked all packaged files in /usr on a system with redistributable installed, ensuring that anything that file(1) reported as XML was valid. Of those, 638 were reported as XML documents, but failed to parse with xml.dom.minidom. Some of these errors were just due to missing namespace prefix definitions, others due to trying to parse dtd files or xml snippets intended to be used as boilerplate, but there were a few examples of plain broken XML according to xmllint. 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. > - line 342: won't this give you an absolute path, rather than one > relative to the image (or proto area) root? We're ok with an absolute path to the file in the proto area here - we pass this path to SmfManifestDependency(), along with the proto area where the manifest was found, which Dependency.make_relative() deals with. With Brock's multiple proto-area wad, I may need to revisit this. > - line 361: specifically, when delivering instances of a single service > in separate files is supported. Fixed. > t_dependencies.py: > > - line 52: This doesn't cause all the smf_manifests tests to be re-run? No, there's no tests in that module, it just contains a heap of test data, SMF manifests mostly. I figured storing it there rather than in the main test module would make things look a bit tidier. > 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? I have changed it to break out early though, which makes sense - thanks. > 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? cheers, tim _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
