Hi Dermot, On Thu, 30 Sep 2010, Dermot McCluskey wrote:
Hi All, I'd like a code review for the ManifestParser/ManifestWriter checkpoints for the CUD project. The changes are here: http://cr.opensolaris.org/~dermot/webrev_mp_mw_01/ Please sent comments by 10/12/2010.
I reviewed the following since I figured it was the latest: http://cr.opensolaris.org/~dermot/webrev_mp_mw_04/ General: I thought that the DTDs would be located under install_manifest in the source and installed in /usr/share/install. Is that not correct? install_manifest/common/Makefile: What does the HDRS rule do here? It seems to me that it can be removed as can be PRIVHDRS and EXPHDRS parser/Makefile: same comment as above writer/Makefile: same comment as above parser.py: line 133. It might be useful to put a comment here indicating what the behavior is if call_xinclude is True and validation needs to be performed - i.e. xinclude processing occurs before validation does. parser.py: lines 145-148. Since this is the entry point when invoked from the engine, shouldn't simply using self.logger work? FWIW, the DC checkpoints used to do something like what you have here but we changed it to simply use self.logger and that works just as well. parser.py: line 229: What use case does the cancel_requested handle? parser.py: line 235: What happens if the DOCINFO specifies an Internet URL that can be traversed only using a proxy - is there a way to support validation through a proxy? I'm just asking I don't have a concrete use case for this .. parser.py: line 330: Am I misremembering that the xinclude happens *before* validation? The code seems to indicate otherwise. test/manifests/*.dtd: Rather than duplicating the DTDs here, I think it might be more useful to just reference the DTDs that will get installed under /usr/share/install or something. That way the tests will always be running the latest and the greatest. test/manifests/*.xml: DC is going to deliver manifests under /usr/share/distro_const, I wonder if there's a way to just use those manifests and customize them appropriately rather than keeping copies of the manifests here. I see Karen made a similar comment but I haven't had a chance to go through your replies on that .. test/*.py: Just like parser.py, in the tests with the engine, self.logger could should be all that's needed for logging. writer.py: line 319: Should a failure to close the file cause a fatal error? Seems a little harsh. system-library-install.mf: I think you need to sync with slim_source Alok _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
