Hi Dermot,
Your responses below and in your other follow-up email detailing
decisions from the meeting all sound good to me.
- Keith
On 06/23/10 05:37 AM, Dermot McCluskey wrote:
Keith,
Thanks for reviewing. Responses below.
On 06/23/10 00:26, Keith Mitchell wrote:
Hi Dermot,
First comment - the link given points to the wiki "preview" page
which requires edit privileges. The correct link would be:
http://hub.opensolaris.org/bin/view/Project+caiman/OpenSolarisInstaller+-+ManifestParser+and+ManifestWriter
Sorry for the error and thanks for correcting this.
My comments are below. Overall, the design looks good; I have a few
questions about the behavior of the interfaces, as well as the
exceptions exposed. (Thanks for calling out the differences from the
prior ManifestParser - that was interesting and a valuable point of
comparison)
3.4.1.1: The behavior and defaults for validate_from_docinfo leaves
something to be desired in the scenario of the XML not having a
DOCINFO section with a listed DTD. Better default behavior could be
achieved by setting the default to None, and then, "if
validate_from_docinfo is None" AND the supplied XML file has a given
DTD, perform the validation. If the caller explicitly wants
validation from docinfo, they pass in True (in which case, absence of
the DOCINFO/DTD data would be an error, and if they explicitly don't
want validation, then pass in False.
I fully expected that this bit would need to be re-worked, so I'm glad
you
commented on this. I was basically trying to map onto the functionality
provided by lxml and allow the document to be parsed and validated in
one pass, instead of having to parse it first and see if it has a DTD
before
validating.
In any event, the time taken for ManifestParser is miniscule compared to
other checkpoints, so it's not really significant, but I like your
suggestion,
where validate_from_docinfo = [None|True|False] which allows for a
sensible default and still supports one-pass parsing -and-validation,
if desired.
I'll change to do as you suggest.
[btw, the error returned by lxml for DTD validation failure differs
slightly
depending of whether single-pass or two-pass parsing and validation is
done.
So, perhaps this is a reason not to support single-pass: to keep error
reporting
consistent?]
page 13 - execute/dry_run behavior: I think a dry_run for
ManifestParser or ManifestWriter would still add items to the DOC.
Consider for example, a full dry-run of AI - the subsequent
checkpoints would need the data to perform their dry_run actions.
Makes sense. I'll change it so dry_run will be ignored for
ManifestParser.
page 13: execute/raises: For an application attempting to recover
from such an error, how would they differentiate between the
different error cases and inform the user? If the manifest doesn't
validate, how can the app inform the user exactly where in the XML
file the parsing failed? I think ManifestParser needs a very solid,
usable method for informing the user of how to fix an error in their
manifests, which means the errors raised need to be readily
understandable by the application and provide plenty of data, so I
think this section should be expanded. For example, right now,
depending on the error one has in an AI or DC manifest, it's possible
to end up with a generic, and useless "could not parse manifest"
error with no additional data - that doesn't really help an end-user
figure out that they had a typo on line 78 and forgot to close the
"<pkg_repo_addl_authority" tag.
In particular, subclasses of ManifestError that group the errors into
different cases would be valuable. And for some of those cases, the
error may benefit from providing more than simply string data. (e.g.,
line number, etc.)
lxml returns a fairly useful text description for XML syntax errors,
etc. My intention was that all
ManifestParser errors, from cannot-find-file to does-not-validate
would return the same exception,
but the exception's message would give a detailed description, eg:
Unable to read Manifest file [dc.xml]: No such file
or
XML syntax error in Manifest file [dc.xml]: Opening and ending tag
mismatch: software_spec line 59 and transfer, line 66, column 12
or
Validation against DTD [dc.dtd] failed: No declaration for attribute
mod_name of element checkpoint, line 43, column 27
and the application would just present this message to the user.
If more finely grained control is needed, then I can certainly
sub-class ManifestError as you suggest,
although offering the line number in a separate value seems excessive
- would we really use this
in our applications?
An indication of how the ManifestErrors wrap around or enhance the
errors raised by lxml would be valuable as well.
The exceptions being caught from lxml are:
IOError
etree.XMLSyntaxError
etree.DTDParseError
In all cases, a ManifestError is raised, taking its message from the
above exception
(plus some additional text).
When DTD validation fails, lxml returns an error code and puts the
reason in an
error log. ManifestParser raises ManifestError, using the text from
the error log
(plus some additional text)
I'll add a summary of the above to the document.
page 14: What factors, if any, would affect the value returned from
get_progress_estimate() for this checkpoint? For example,
transfer/IPS depends on the number and size of packages being
installed - are there any similar factors at play here, or is it
expected to be static (I'm mostly just curious - I can't think of
anything off-hand).
- size of manifest and DTD files, but this would not be significant
- network access is disabled for loading XML and DTD files,
so that should not be a factor
I can't imagine get_progress_estimate() will return anything other
than 1, really.
Also, I should add a comment to the spec to state that ManifestParser
and ManifestWriter
will not be providing progress updates during their execution.
Execution time is
expected to be minimal (c. 0.12 seconds in my prototype), so updating
progress
would not be very useful.
page 16: ManifestWriter/execute/dry_run behavior: For the
ManifestWriter, on the one hand it could be argued that a dry_run
could still benefit from presenting the user with an output manifest
(perhaps just on stdout?). Depending on how a user or developer
running an app specifies dry_run and an output file, it may be best
to just always dump to XML.
How about if I write it to the log, but do not create the output file,
when dry_run = True?
page 16: execute/raises: Again, more detail would be valuable,
although the ManifestWriter is probably better protected from
end-user errors/typos then the parser so this is perhaps less critical.
As per ManifestParser, I can sub-class the exception class if more
precise
errors are required?
Thanks,
- Dermot
Thanks,
Keith
On 06/17/10 06:02 AM, Dermot McCluskey wrote:
Hi,
I hope there is still some enthusiasm left for reviewing design specs?
I'd like to request a review for my design for ManifestParser and
ManifestWriter.
The latest version of the document can be found here:
http://hub.opensolaris.org/bin/preview/Project+caiman/OpenSolarisInstaller+-+ManifestParser+and+ManifestWriter
As this component is slightly less complex than others that have
recently been
reviewed, I'd like to request that comments be sent by Friday June
25th. If this
is not practical, I will extend this date.
Please let me know if you intend to review this so I can be sure I
have enough
reviewers. I would particularly appreciate comments from some of
the following:
Jack, Alok, Karen, Darren, Sarah.
Thanks,
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
