Hi Darren,
I know it is past Friday, but just in case you are still accepting comments,
I have the following comments. They are all minor comments.
-------------
---------------------
usr/src/cmd/Makefile.targ:
---------------------
lines 98-100, 147-149: what's this target for?
---------------------
usr/src/cmd/auto-install/Makefile:
---------------------
- Question: I noticed you remove the target for generating .po file.
How are messages going to be generated?
---------------------
usr/src/cmd/auto-install/auto_install.py:
---------------------
- line 129-131: this check will still be performed and message printed
if list_checkpoints option is specified. Is that desired?
- line 158: need to add "-l" to this message?
- line 191: this comment doesn't completely reflect what happens
in rest of the function. If options.list_checkpoints is specified,
all the checks in lines 210-223 are still performed. Would it
be more efficient to check whether options.list_checkpoints is specified,
if so, just return from the function?
- line 221 and line 223: I think these 2 checkpoint names should be
defined as constant, so, if they ever change in the future elsewhere,
we don't need to worry about changing it in all the places it is used.
- lines 436-443: I think this should this be moved to line 422. This is
because even though ppl who don't want to run target-discovery will want
to know whether there's any problem with parsing the manifest.
- line 485: You don't need to specify resume_checkpoint=None here.
If you don't specify a resume_checkpoint argument at all, the engine
will default to None.
- line 540: why would this ever be not None? I think that having
derived manifest data at this stage would be considered an error.
- line 1041: For the text installer project's progress reporting screen,
I started by copying the AIProgressHandler() code. :-) Through
testing, I found that the parse_progress_msg() function, as it is
implemented
right now does not work well all the time. If a checkpoint regenerates
progress messages "very fast", you will get multiple progress message
records in 1 single message, because in line 1060, you are taking
EVERYTHING in the message record after the first 4 characters. In AI, I
guess
it is not as bad since messages are displayed on the screen. In Text
Installer,
the garbage characters will crash the text installer. I fixed this problem.
If you are interested, check out:
http://src.opensolaris.org/source/xref/caiman/slim_source/usr/src/cmd/text-install/progress.py#123
Also, there's a lot of duplicate code between your implementation and
the text installer's progress implementation. We should file a file
to consolidate the implementation so all the installers will use the
same code.
----------------
usr/src/cmd/auto-install/checkpoints/Makefile
-----------------
- lines 43 and 54: SUBDIRS is not defined in this file.
These should be removed.
---------------------------
/usr/src/cmd/auto-install/xslt/new-to-newer.py
--------------------------
- line 22: copyright year not correct.
- lines 60-93: is there a reason we are not using the
solaris_install.Popen()?
Thanks,
--Karen
On 05/18/11 15:46, Darren Kenny wrote:
Hi,
I think it's about time I got out another version of the code review for the CUD
AI project.
I've uploaded the webrev at:
http://cr.opensolaris.org/~dkenny/cud_ai-to-slim-2/
And for anyone that's reviewed the code before, there is a diff:
http://cr.opensolaris.org/~dkenny/cud_ai-to-slim-2-diffs/
target_selection.py has probably changed the most, so if time is short, we'd
really appreciate you reviewing that file at least.
If at all possible could you please provide any feedback by Friday COB.
Thanks,
Darren.
_______________________________________________
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
