Jack Schwartz wrote: > 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. > I'll probably be changing it before then to go with the workspace stuff I'm doing. So I guess I'm OK with it. >> 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 > OK. I'll probably talk with you about a way to work this out as it's part of my checkpointing the microroot work. >> 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. > > OK. >> 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. > >
Jean > 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 >>> >>> >>> > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
