I reviewed all the files in Makefiles and misc, DC and boot and Text
Install sections.
Here are my comments:
>
> Makefile and Misc.: Volunteers
> ----------------------------------------
> usr/src/Makefile.master
OK
> usr/src/Targetdirs
OK
> usr/src/lib/Makefile.targ
OK
> usr/src/lib/install_common/__init__.py.src
Lines 482-500: We already have a function that determines
the amount of memory a system have in lib/install_target/controller.py.
The function is called _get_system_memory(). Looks like it is only
used in the controller.py file. Can we either remove the
other one and use this? Just don't want to have duplicate code.
>
> DC and Boot: Ethan, Seth, Drew
> -----------------------------------------
> usr/src/cmd/distro_const/checkpoints/create_iso.py
OK
> usr/src/lib/install_boot/boot.py
line 676-678: there's no handling of the case where you can't find
any nodes from 673-675. Is that OK?
line 856: commented out line
line 855-859: Looks like the "ident" file is created at the root of
the image. Is this file only used when we boot from media?
Is this file also necessary for the installed system?
If it is not necessary for the installed system, it needs to
be cleaned up by the cleanup_cpio_install checkpoint.
If it is used by both ISO and installed system, would it be better to
deliver
the file as part of a package instead of creating it dynamically here?
line 855-859: can we move these few lines inside the try block, after
line 865. That way, you don't need to do the unlink if the setprop fails
because of Legacy Grub.
line 997: fix this comment line? :-)
> usr/src/lib/install_boot/test/test_boot.py
OK
> usr/src/tools/tests/tests.nose
OK
>
>
> Text Install: Karen + Volunteer
> --------------------------------------
> usr/src/cmd/text-install/Makefile
ok
> usr/src/cmd/text-install/__init__.py
ok
> usr/src/cmd/text-install/disk_selection.py
ok
> usr/src/cmd/text-install/disk_window.py
line 227: why not have this line immediately after line 223, in the else
block? Since it's obvious that we do not have gpt data at that point.
line 355: I noticed that the check to make sure
that get_parts_in_use() always return a list with length bigger
than 0 is removed. (eg: lines 322-323 in the original code).
> usr/src/cmd/text-install/fdisk_partitions.py
OK
> usr/src/cmd/text-install/gpt_partitions.py
OK
> usr/src/cmd/text-install/helpfiles/Makefile
OK
> usr/src/cmd/text-install/helpfiles/gpt_partitions.txt
OK
> usr/src/cmd/text-install/partition_edit_screen.py
OK
> usr/src/cmd/text-install/progress.py
OK
> usr/src/cmd/text-install/summary.py
OK
> usr/src/cmd/text-install/ti_install.py
OK
> usr/src/cmd/text-install/ti_install_utils.py
OK
> usr/src/cmd/text-install/ti_target_utils.py
Some of the comments in the newly added UIGPTPartition() class
still talks about slices, I guess left over from copy-and-paste. :-)
> usr/src/cmd/text-install/welcome.py
OK
> usr/src/pkg/manifests/system-install-text-install.mf
ok
Thanks,
--Karen
On 12/23/11 03:44 AM, Niall Power wrote:
Hi,
Just a reminder that the timeout for round 1 review comments is this
Friday 23rd.
Thanks to everyone who has already provided comments or volunteered to
review.
I'll still check e-mails for review feedback over the holiday break,
but expect responses to be somewhat spotty :-)
I'd also really like to limit the scope of round 2 comments to final
tweaks and polish. So if there is anything you are deeply
uncomfortable with in the webrev but haven't mentioned yet, best to
get it out in the air now :)
I'm getting good feedback on Targets, AI and the GUI.
I haven't heard anything back regarding the following sections:
- Text Install
- Makefiles and Misc.
- DC and Boot
Karen has kindly offered to review the Text and Makefile & misc.
sections.
If anyone else feels like having a stab at any of these let me know.
Thanks,
Niall
On 12/19/11 08:19 PM, Niall Power wrote:
Hi!
Myself, Drew and Dave Marker would like to ask for code reviewers for
the Install UEFI, GRUB2 and large disk boot project.
I've broken down the webrev into component groupings and specified
people who we would particularly like to review particular components
(eg. leads for those compononents) but we welcome and appreciate any
and all review comments from others too of course.
Some things to be aware of:
This is about 95%, but not yet 100% complete.
The core is complete and functional but there are a few odds and ends
that need to be done still:
- usbgen and usbcopy need to be ported to be GRUB2 and UEFI
compatible. I am figuring out this with Seth. Will create a separate
webrev.
- installadm changes for GRUB2 and UEFI. Sue is working on this and
will be handled in a separate review
- Installation of UEFI system into an MBR partitioned disk. This is
a minor amount of work that doesn't impact the existing code changes
(it will essentially add a small amount more code). I'll get this
included in time for round 2, otherwise create a separate webrev.
I'd like to set an initial timeout for collection of round 1 review
comments on Friday Dec 23rd.
Following that I propose to ring in the new year with a second round
of review comments from Monday Jan 2nd until Friday Jan 6th.
Please let me know if you intend to review any components, or if you
are unavailable to review specific requested components.
*Webrev URL:
----------------*
https://cr.opensolaris.org/action/browse/caiman/niall/slim_uefi_version_01/webrev-uefi-ver-1/
Most of the files below have only a very small number of lines of
code changes so it's not as big a task as it looks.
*
Makefile and Misc.: Volunteers
----------------------------------------*
usr/src/Makefile.master
usr/src/Targetdirs*
*usr/src/lib/Makefile.targ
usr/src/lib/install_common/__init__.py.src
*
*
*Targets: Dermot, Darren, Matt
---------------------------------------*
usr/src/pkg/manifests/system-library-install.mf
usr/src/lib/install_manifest/dtd/target.dtd
usr/src/lib/install_target/Makefile
usr/src/lib/install_target/__init__.py
usr/src/lib/install_target/controller.py
usr/src/lib/install_target/cuuid.py
usr/src/lib/install_target/discovery.py
usr/src/lib/install_target/instantiation.py
usr/src/lib/install_target/libdiskmgt/diskmgt.py
usr/src/lib/install_target/libefi/Makefile
usr/src/lib/install_target/libefi/__init__.py
usr/src/lib/install_target/libefi/cfunc.py
usr/src/lib/install_target/libefi/const.py
usr/src/lib/install_target/libefi/cstruct.py
usr/src/lib/install_target/libefi/efi.py
usr/src/lib/install_target/logical.py
usr/src/lib/install_target/physical.py
usr/src/lib/install_target/shadow/__init__.py
usr/src/lib/install_target/shadow/physical.py
usr/src/lib/install_target/test/test_shadow_list.py
*AI: Darren, Matt
--------------------*
usr/src/cmd/auto-install/checkpoints/target_selection.py
usr/src/cmd/auto-install/test/test_target_selection_gpt.py
*DC and Boot: Ethan, Seth, Drew
-----------------------------------------*
usr/src/cmd/distro_const/checkpoints/create_iso.py
usr/src/lib/install_boot/boot.py
usr/src/lib/install_boot/test/test_boot.py
usr/src/tools/tests/tests.nose
*
GUI: Darren, Dermot
---------------------------*
*
*usr/src/cmd/gui-install/src/Makefile
usr/src/cmd/gui-install/src/confirm_screen.py
usr/src/cmd/gui-install/src/disk_panel.py
usr/src/cmd/gui-install/src/disk_screen.py
usr/src/cmd/gui-install/src/fdisk_panel.py
usr/src/cmd/gui-install/src/gpt_panel.py
usr/src/cmd/gui-install/src/screen_manager.py
usr/src/cmd/gui-install/xml/Makefile*
*usr/src/pkg/manifests/system-install-gui-install.mf *
*
*
I seriously wouldn't bother code reviewing these glade auto-generated
XML files but if you enjoy pulling your eyes out :)*
usr/src/cmd/gui-install/xml/fdisk-panel.xml
usr/src/cmd/gui-install/xml/gpt-panel.xml
usr/src/cmd/gui-install/xml/installationdisk.xml
*
**
Text Install: Karen + Volunteer
--------------------------------------*
usr/src/cmd/text-install/Makefile
usr/src/cmd/text-install/__init__.py
usr/src/cmd/text-install/disk_selection.py
usr/src/cmd/text-install/disk_window.py
usr/src/cmd/text-install/fdisk_partitions.py
usr/src/cmd/text-install/gpt_partitions.py
usr/src/cmd/text-install/helpfiles/Makefile
usr/src/cmd/text-install/helpfiles/gpt_partitions.txt
usr/src/cmd/text-install/partition_edit_screen.py
usr/src/cmd/text-install/progress.py
usr/src/cmd/text-install/summary.py
usr/src/cmd/text-install/ti_install.py
usr/src/cmd/text-install/ti_install_utils.py
usr/src/cmd/text-install/ti_target_utils.py
usr/src/cmd/text-install/welcome.py*
*usr/src/pkg/manifests/system-install-text-install.mf
*
*Thanks!!
Niall, Dave & Drew
*
**
*
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
