On 04/26/12 16:17, Drew Fisher wrote:
On 4/26/12 8:09 AM, Jan Damborsky wrote:
Hi Karen,
thank you very much for review.
Sure I can take care of 7093060 :-)
Drew, would you mind if I take over that CR from you ?
It hurts when you twist my arm like that, Jan! Ow! OK! You can have
the CR! :)
Wise decision, Drew. Hope that chopped off little finger doesn't hurt
much :-)
One of the things I was working on with 7093060 was a single class
method which did *all* of the copying/moving at the same time instead
of having 4-5 different 2-5 line methods. Can you investigate
collapsing all of the individual methods down to just one big method?
the GUI subclass will need a second copy/move function for all the
GNOME/X11 files but it should be able to also call the other method
Yep, that sounds reasonable. I replaced all those methods with
just one. Unit tests were modified accordingly.
Being in that code, I also decided to address
7098714 system/network installed via GUI and text installer fails pkg
verify check
WRT code review itself, I addressed all your comments, with one small
exception.
save_menu_lst() considers missing 'boot/grub/menu.lst' to be a valid
case,
while save_files_directories() spits warning if it's asked to save
missing file.
So I decided to keep save_menu_lst() as a separate function - it
contains
necessary check plus call to save_files_directories().
We need to make the relocation of grub files specific to x86. There's
no grub on SPARC and it kicks up warnings it shouldn't. Can you wrap
the checks for boot/grub/menu.lst in 'if platform.processor() == "i386"
Sure, no problem. Updated webrev with all changes can be found here:
https://cr.opensolaris.org/action/browse/caiman/dambi/cr-7162079-3/webrev-3/
I tested Sparc text installer, build of other media is in progress.
Ginnie, Karen, could I please ask you to also take a look at updated
webrev ?
Thank you very much,
Jan
-Drew
I updated webrev accordingly:
https://cr.opensolaris.org/action/browse/caiman/dambi/cr-7162079-2/webrev-2/
So far I have tested Sparc text installation, build of other images
is now in progress.
Jan
On 04/25/12 23:52, Karen Tung wrote:
Hi Jan,
I really like the improvements you made to the
save_files_directories() function
in pre_pkg_img_mod.py. With those improvements, you almost fixed
bug 7093060 also.
Do you mind taking that bug as well? :-)
I agree with Ginnie that the save_etc_files() function you added is not
really needed. I guess you were trying to follow the "style" in
this file.
It would greatly improve readability of the code if you could just call
save_files_directories(["/etc/shadow"], preserve=True)
Another comment I have is that you moved the saving of /etc/system
to the save_etc_files()
function. I think the function doing the modification,
modify_etc_system(), is the better place
to save the file. That way, people don't need to look all over to
see whether /etc/system is saved
before it is modified.
As for the /etc/inet/hosts file that you added to the
save_etc_files() function, and removed
the existing save_etc_inet_hosts(). The save_etc_inet_hosts()
function is no longer necessary,
the saving of /etc/inet/hosts can be done inline.
Because the improvements you did to save_files_directories(), you
can also replace lines 164-183 with
1 single call. Furthermore, you can get rid of the save_menu_lst()
function, and just save that file inline.
Thanks,
--Karen
On 04/24/12 03:42 AM, Jan Damborsky wrote:
Hi,
I would appreciate code review of changes for
7162079 CPIO installation propagates install root password to the
target
webrev:
https://cr.opensolaris.org/action/browse/caiman/dambi/cr-7162079/webrev/
Thank you,
Jan
Testing done:
* built text/AI/LiveCD install images using modified Distro Const.
Workarounds for 7162039 and7162553were applied on LiveCD.
* tested text, AI, LiveCD installations.
Verified that shadow(4) was restored from 'save' area in case
when CPIO transfer method was used for installation (text, LiveCD).
* run unit tests (new unit test added for introduced
method PrePkgImgMod.save_etc_files()
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
