Hi Jean. Thanks for your review...
Webrev with your and Karen's comments incorporated is in the same location: http://cr.opensolaris.org/~schwartz/080905.1/webrev/ Responses inline... Jean McCormack wrote: > Jack, > > I'll have to agree with Karen about the key value pairs you added. I'd > say for now, leave them as they were for the old DC. I've actually > been working > on moving the boot root area to be under the build area, i.e. more > like what you have but not configurable. > > bootroot_archive.py: > With the above comment in mind, lines 75-88 should be redone. I'd like to leave this for now, knowing it will be changed when bootroot autosizing is implemented. > line 52: I'm concerned about the Note here. Is this the case > where you have already done a ti_release_target > in a previous run and then resume the build post initialize but > before this script? Yes. We saw this when we ran step 2 and step 4 in separate runs. > Have you investigated > catching the mentioned error and then proceeding? This is because the temp dir name includes a PID. It would be hard to know which bootroot to use here, but in step 2 it sets up a new one. I actually clean up old stale bootroots in step 2. I thought the comment in this file, plus the cleanup in step 2 would be enough. > Does the os.system call raise any exceptions we should be catching? I don't think so. That's why I check status and raise my own exceptions when I find error values returned. > > bootroot_configure.bash > 48: Is this OK? What about cd'ing to BOOTROOT and back to the saved > pwd at the end? The function where this comment exists is local, called only from this script. I cd to $BOOTROOT before calling this function. > > line 55: Might be a good idea to check the number of args. Done. > > line 117: ditto Done. > > line 128: Back to the comment about the key value pairs in the > manifest. Yeah, yeah, yeah... :) > > bootroot_initialize.py > line 60ish: It would be nice to check for number of args. Done. > line 84+: See comment about the manifest key value pairs. > line 137: I thought we had moved this back down to 200000 after > Karen's fix for the packaging stuff? Good catch! I'll change this. Thanks. > > lines 155-160: delete. Replaced with a single TBD comment. > lines 182-185: delete I think these comments are still useful. Will replace multiple CILs with one TBD though. > line 188: Does os.chdir raise and exception if you can't chdir to > the specified directory? If so, you shouldn't > need the following if. Good point. Will remove the if. > line 208: itme should be item. Fixed. > line 224: How about this: > find_no_excl_cmd = "/usr/bin/find ./var ! -type f | /usr/bin/cpio > -pdum " + ABS_BR_ROOT > status = os.system(find_no_excl_cmd) I'll meet you half way. I'll add in the ABS_BR_ROOT, but want to keep "item" separate from the format string. that way we can reuse the string twice. > > line 231: similar. I find it easier to read if the cmd is spelled out. See above. > lines 236-247: delete Gone. > lines 250-284: Do any of these os calls need to be checked for > exceptions? This stuff has been moved to bootroot_configure bash script so this is now a moot question. Thanks for your time. Jack > > > > > Jack Schwartz wrote: >> Hi everyone. >> >> I sent this to caiman-discuss but hasn't yet shown up in the big bad >> world out there... Since it's somewhat urgent, I'm forwarding to >> ejray-staff so someone can review it. >> >> thanks, >> Jack >> >> -------- Original Message -------- >> Subject: Please review: new DC bootroot constructor >> Date: Fri, 05 Sep 2008 14:12:08 -0700 >> From: Jack Schwartz <jack.a.schwartz at sun.com> >> To: caiman-discuss <caiman-discuss at opensolaris.org> >> >> >> >> Hi everyone. >> >> Here is a webrev which installs a new bootroot constructor for DC. >> It introduces three finalizer scripts which together handle putting >> together the bootroot. It ushers out build_dist.bash and >> build_dist.lib. It also includes a change to dc_utils.py to support >> manifest keys. This is the final piece needed for our phase 0 release! >> >> Thanks go to Channing Lovely, who did a significant amount of the >> work, particularly on the initialization script and the packaging. >> >> Please send your comments before Monday noon. >> >> Webrev is at: >> http://cr.opensolaris.org/~schwartz/080905.1/webrev/index.html >> >> Thanks, >> Jack >> >> >
