Hi Dave, Thanks for the review. Comments below.
- Keith Dave Miner wrote: > Keith Mitchell wrote: >> Hi Glenn and all other reviewers, >> >> I have posted an updated webrev at: >> http://cr.opensolaris.org/~kemitche/text_v2/ >> > > My comments on the Image/Distro-const and Packaging/Makefiles, plus a > couple of other random things, are below. > > Dave > > usr/src/Makefile.master, 110 and usr/src/Targetdirs, 82: for > consistency with other components, I'd suggest text_install gui-install, ai-webserver, auto-install, install-tools, slim-install - I think using text-install as the directory name under usr/src/cmd is more consistent. The folders/files that drop into python directories use an underscore, and are consistent in that respect. > > usr/src/cmd/Makefile.cmd, 31: as we discussed offline the other day, > I'd rather see xgetsh moved to usr/bin Agreed. > > usr/src/cmd/distro_const/auto_install/ai_plat_setup.py, > ai_post_boot_archive_pkg_image_mod: Please use hg rename for these > moves to utils rather than delete & add. That should be doable. > > usr/src/cmd/distro_const/utils/grub_setup.py, 248: How about moving > this block outside the conditionals (since it appears in all of them) > and delete the blocks at 200 and 229? Agreed. > > usr/src/cmd/slim-install/finish/install-finish, 246: need to finish > something here... Quite right, that will be fixed. > > usr/src/cmd/slim-install/svc/media-fs-root, 262: missing language > configuration on this path Indeed. We'll get it fixed. [...] (DC comments will be addressed by Jack) [...] > > > usr/src/cmd/text-install/osol_install/text_install/ti_install.py, > 317ff: I'm rather confused why some ICT's are called directly, yet > install-finish is also used and modified. Seems using install-finish > for all would obviate the need to call (and thus package) ict_test in > the absence of a fix for 6256. Something I'm missing here? install-finish doesn't call the C ICTs, so this is the best solution that involves the least amount of throwaway code, in light of 6256. This is also a good spot to add that we're also suffering from 6255 here - with how install-finish is written, we're forced to spawn a subprocess to run it, even though it's Python code and we're running in a Python context.
