Hi Alok,
Many thanks for the review. Responses below and a new
webrev showing the fixes for the issues you raised is at:
http://cr.opensolaris.org/~dermot/webrev_mp_mw_05/
On 10/13/10 02:53, Alok Aggarwal wrote:
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?
The DTDs (and XML and XSLT) files here are all purely used as
test input files and are not installed under proto or delivered in any
package. They are separate from the "real" files found in other dirs.
Also, see comment below.
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
I just copied someone's else's Makefile and modified it to build my code.
I have removed the HDRS, PRIVHDRS and EXPHDRS rules now.
fixed
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.
Fixed.
But it's not as simple as that. The comment now reads:
Note: Currently, the on-the-fly validation performed if
validate_from_docinfo=True occurs *before* XInclude statements
are processed and validation triggered when
validate_from_docinfo=None or when dtd_file is specified
occurs
*after* XInclude processing. XInclude processing may affect
whether validation succeeds or not, so this ordering may need
to be considered.
We've discussed this before: this awkwardness is caused by how the lxml
APIs operate. I'm open to changing this to make it more consistent, but I
think we need to wait a little to see how XInclude is going to be used in
the install apps first so we can decide what works best.
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.
When I try that and re-run all my unit tests, only the output from one
of the
24 tests is in the log - all the other logging output is missing. Any idea
what's going on here?
I've also noticed that even without making this change, some (but not all)
of the output from my full set of unit tests is missing from the log.
It seems
that tests that use the InstallEngine are not logging, *if* those tests
are run
after some other tests that log output.
Do you have any examples where you have run more than one unit test
file, with more than one test per file, where all the tests log something?
It would be very helpful to have a template of how to initialize and reset
logging, in Engine checkpoints, non-Engine code, and unit tests. Do you
know if such examples exist?
parser.py: line 229: What use case does the cancel_requested
handle?
I'm not sure if I understand the question correctly, but this will be
invoked
in the user requested Cancel since the previous check, ie during the call
to _load_manifest(). I think Karen specifically requested I add this one
during her review.
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 ..
By default, lxml will not fetch any entities (files, DTDs, XIncluded
files, etc) that required network access. This was discussed during my
design review and it was agreed to not override this, for security reasons.
parser.py: line 330: Am I misremembering that the xinclude happens
*before* validation? The code seems to indicate otherwise.
See above - there can be validation during the initial load (the call to
etree.parse()) which is before XInclude, or when calling
validate_manifest(),
which occurs after XInclude.
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.
It was a mistake on my part to leave these files with names that show
they were taken from the DC manifest. These files are for unit testing
MP/MW only and should be separate from the DC files and there should
not be any dependency or relationship with DC's files.
I've renamed them now.
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 ..
See previous comment - I've renamed those files now.
test/*.py: Just like parser.py, in the tests with the engine,
self.logger could should be all that's needed for logging.
See previous comment re: logging.
writer.py: line 319: Should a failure to close the file cause
a fatal error? Seems a little harsh.
I've rarely (if ever) seen a file close operation, specifically,
fail, but if it does it would definitely indicate a serious problem
that the user would want to be notified about.
In this case, we are creating and immediately closing a file, which
will be opened again back in the main code. If the close fails here,
then I'd imagine the re-open will also fail.
system-library-install.mf: I think you need to sync with slim_source
I know I'll need to break out my changes from the other CUD changes
to this file before I push, but I've just included the latest version
from the
cud project gate here to show which pkgs the MP/MW files would be going
into.
Alok
Thanks,
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
