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 usr/src/cmd/Makefile.cmd, 31: as we discussed offline the other day, I'd rather see xgetsh moved to usr/bin 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. 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? usr/src/cmd/slim-install/finish/install-finish, 246: need to finish something here... usr/src/cmd/slim-install/svc/media-fs-root, 262: missing language configuration on this path usr/src/cmd/distro_const/text_install/tm_gen_cd_content: this is identical to the slim_cd version. Please move one to utils and adjust accordingly. usr/src/cmd/distro_const/text_install/tm_generic_live.xml: as this is mostly common with the slimcd version, we should be using a common base that's included by each of them. usr/src/cmd/text-install/text-mode-menu: why not collapse this into the svc directory? It's part of the service. usr/src/cmd/text-install/svc/text-mode-menu.xml: 36: recommend comment here noting why we're using this specific service name 71: comment here seems wrong; I presume transient is meant rather than child. text-mode-menu/text-mode-menu.ksh: 29: this seems incorrect for cases where you're using a remote console with xterm. Are we missing a terminal type choice screen in the design? 30: shouldn't this be translated back from the method's uid? Perhaps a better question is why we need to set it at all? 41: This shouldn't be present until DU integrates 54: Product name/version needs to be parameterized from the marketing name; perhaps the GRUB_TITLE entry in /.imageinfo would do. Or removed. 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?
