Hi Karen, Thanks for the review...
On 24/05/2011 00:10, Karen Tung wrote: > 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. No problem... > > ------------- > --------------------- > usr/src/cmd/Makefile.targ: > --------------------- > lines 98-100, 147-149: what's this target for? They were old Derived Manifest targets, they have been removed now. > > --------------------- > usr/src/cmd/auto-install/Makefile: > --------------------- > - Question: I noticed you remove the target for generating .po file. > How are messages going to be generated? They aren't - there is no localized text in the auto installer right now. This is a known issue to be addressed yet, but not a priority for AI since there is no way to change the running locale on the AI ISO. I removed the .po generation since it only worked on C files, of which there were none remaining. > > --------------------- > 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? Yes - for testing purposes... There is a -s option that is a hidden argument, but useful. > > - line 158: need to add "-l" to this message? No, it's another hidden option. > > - 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? Unfortunately we can't just list the checkpoint here. We need to parse the manifest before we can list all the checkpoints - this is because for transfers we need to know what the user wants to do, generate checkpoints for each software node, and only then can we list all the checkpoints. But, we also limit the output list of checkpoints with the -I/-i/-s options too. > > - 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. Agreed, added one for TI, but the other one is being removed... > > - 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. Makes sense, done. > > - 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. > OK, but I don't see any harm in being explicit here, do you? > - line 540: why would this ever be not None? I think that having > derived manifest data at this stage would be considered an error. Well... It may be that we inject a value for testing purposes into the DOC. This allows for that to be possible. > > - 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. That would make sense to consolidate, and probably best done in the logging module to be honest. If you could log a bug in logging to do this that would be great. I'll take a look at your code here for our uses, but we've not seen the issue to mention - maybe the machines we're using aren't fast enough... > > ---------------- > usr/src/cmd/auto-install/checkpoints/Makefile > ----------------- > > - lines 43 and 54: SUBDIRS is not defined in this file. > These should be removed. Removed. > > --------------------------- > /usr/src/cmd/auto-install/xslt/new-to-newer.py > -------------------------- > > - line 22: copyright year not correct. Fixed. > > - lines 60-93: is there a reason we are not using the > solaris_install.Popen()? This is a simple script, meant to be runnable without all the install packages. Thanks, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
