Karen (& Dermot),
Thanks for the review. We appreciate the time you put in on the
review. Responses in-lined.
Thanks,
John
On 06/10/11 13:01, Karen Tung wrote:
Hi Dermot and John,
I looked at all the files in sections 3 and 8. I also looked at a
couple of
other files. Here are my comments:
3. App startup and checkpoint setup; handle screens; misc.
===========================================
usr/src/cmd/gui-install/src/gui-install.py
===========================================
OK
===========================================
usr/src/cmd/gui-install/src/__init__.py
===========================================
lines 134-173: All the logging information in the setup_checkpoints()
function
is logged at level info, which is the default log level for the GUI
installer.
I don't think that's correct. Checkpoints and names are implementation
details for the GUI installer. I don't think it should end up in the
log file in normal circumstances. Perhaps change log level to debug?
Corrected to be debug.
line 225-228: at this point, the InstallProfile object should have be
in the DOC at this point, right? So, shouldn't profile==None be some
type of internal error?
Correct.
Changed to:
if profile is None:
SystemExit("Internal Error, GUI Install DOC not found")
profile.set_locale_data([locale_description], [the_locale], the_locale)
line 304: why pass in loglevel=logging.DEBUG here?
This should be loglevel=options.log_level. Corrected.
===========================================
usr/src/cmd/gui-install/src/base_screen.py
===========================================
ok
===========================================
usr/src/cmd/gui-install/src/screen_manager.py
===========================================
ok
===========================================
usr/src/cmd/gui-install/src/gui_install_common.py
===========================================
line 1: should it be python2.6 instead of just python?
Corrected.
line 170-177: Nit: Since you are doing the diligence of checking to see
whether the pid is indeed from a running process, why not take
the previous_pid, and check to see whether it is running the GUI
installer.
After all, you only care if it is also running the GUI installer.
I'll let Dermot address this one.
line 181: I think this line is over-indented. The way it is now, it will
only return False if os.path.exists(PIDFILE).
Corrected.
line 190: why open the file with "w+" permission? We should always only
have 1 line in this file, right?
In python 'w' and 'w+' truncate the file. However, I have changed it to
be clearer.
===========================================
usr/src/cmd/gui-install/src/help_dialog.py
===========================================
line 43: commented out code?
Yes. Since removed.
===========================================
usr/src/cmd/gui-install/src/install_profile.py
===========================================
OK
8. Progress Screen / Perform Installation
===========================
usr/src/cmd/gui-install/src/progress_screen.py
===========================================
lines 65-67: Why define these values again here? Isn't it better
to define them once in a common place so the same values are used
when checkpoints are registered into the engine?
Redefined in common_gui_install.py and removed from __init__.py
line 104: since there's no try/except around this line, if the
image dictionary file is missing for whatever reason, the whole GUI
installer will fail. Is this really such a fatal error? Should
the GUI installer be able to allowed to function without the nice
pictures?
Corrected.
line 192-221: Perhaps you want to take a look at the parseProgressMsg()
function in the text installer code? When I was working on the text
installer,
I found things doesn't parse correctly if progress are reported "too
fast".
Like my comment to the AI project, we should consolidate the progress
message receiving and parsing stuff into a common place so all installers
can call it.
Will look at the changes and see about fixing it later. I haven't seen
it report
progress too fast but take your word for it.
line 263-282, 293, 301: I am surprised that you have to do this.
The "transfer-prep" checkpoint should have taken care of all the setup
for "what-to-transfer". Do you have to do this to workaround a bug
or something?
That was copied code from the text-installer. Now removed.
line 309-313: check gui_profile is not None before using it? Or use the
not_found_is_err argument when you retrieve the object from DOC?
Using SystemExit if it is None.
===========================================
usr/src/cmd/gui-install/src/failure_screen.py
===========================================
OK
===========================================
usr/src/cmd/gui-install/src/finish_screen.py
===========================================
- line 124: Not supporting fast reboot? I think existing GUI installer
supports that.
Fixed.
===========================================
usr/src/cmd/gui-install/src/textview_dialog.py
===========================================
- lines 71-76: Nit: I don't think you need the check in line 71. You
can just
open the file pretending that it exists. If it doesn't exist, you can
catch the appropriate exception and print the no log file found error.
This is where we get into one of those gray areas. I've been told that
catching
exceptions when a simple check is cleaner and easier is bad form too. I am
going to leave it as is.
===========================================
usr/src/cmd/gui-aux/imageurl.txt
===========================================
- I assume you will fill in those other links later?
Not necessarily. The code reading the file handles that case.
===========================================
usr/src/cmd/system-config/profile/user_info.py
===========================================
OK
----------
Other files I also looked at:
==============================================
usr/src/cmd/gui-install/src/screen_manager.py
==============================================
- line 311: The finishapp() function is called from the
progress_screen.py
when all the checkpoint completes. I do not see where the BE is
unmounted..etc
I looked at some files where I think might have that code but don't
see it.
I guess I am looking for the code that's similar to lines 285-327 of
the text installer code: usr/src/cmd/text-install/ti_install.py.
Are those not needed for the text installer?
I haven't needed it.
==============================================
usr/src/cmd/gui-install/src/fdisk_panel.py
==============================================
- line 209, 216, 235: more suitable as a debug message?
Corrected.
- line 930: the name "size" for this function will lead to confusions.
When I think of UIDisk.size, I will most likely think of size of the
disk,
instead of number of partitions on the disk. Why not name it
num_parts or
something?
I'll let Dermot handle this one.
Thanks,
--Karen
On 06/ 1/11 11:15 AM, Karen Tung wrote:
Hi Dermot,
I am going to review sections 3 and 8.
If you are going to hold a code walk through, I prefer
Monday, 6/6. I can't make it on 6/3, 9am.
Thanks,
--Karen
On 06/01/11 10:12, Dermot McCluskey wrote:
Hi,
I'd like to request a code review for the GUI Install -> CUD project.
I need to set a timeout of Friday June 10th for this first round of
comments.
Please let me know if you intend to review, and also which of the
sections
listed below you will be looking at.
The webrev is here:
http://cr.opensolaris.org/~dermot/webrev-cud-gui-round-1/
The latest iso and usb images for the project are here:
file:///net/indiana-build.us.oracle.com/var/lib/hudson/jobs/dc/cud_gui/media
Below is my suggested grouping of the sources. If reviewers think a
code
walk-through would be helpful, then let me know. I could do this
at, eg,
9am PT on Friday, 3rd or Monday, 6th or some other time if that doesn't
suit.
Thanks,
- Dermot
1. Makefiles and manifest
==================
usr/src/Makefile.master
usr/src/cmd/Makefile
usr/src/cmd/Makefile.targ
usr/src/cmd/gui-aux/Makefile
usr/src/cmd/gui-install/Makefile
usr/src/cmd/gui-install/src/Makefile
usr/src/cmd/gui-install/pixmaps/Makefile
usr/src/cmd/gui-install/xml/Makefile
usr/src/pkg/manifests/system-install-gui-install.mf
2. Glade UI Definition file
==================
(These files are basically the old *.glade files which define the
Gtk+ widgets
and layout of each screen. They have been run through the
gtk-builder-convert
utility to convert them to the newer Gtk+Builder format, plus some
minor
tidying up.)
usr/src/cmd/gui-install/xml/confirmation.xml
usr/src/cmd/gui-install/xml/date-time-zone.xml
usr/src/cmd/gui-install/xml/failure.xml
usr/src/cmd/gui-install/xml/gui-install.xml
usr/src/cmd/gui-install/xml/installation.xml
usr/src/cmd/gui-install/xml/installationdisk.xml
usr/src/cmd/gui-install/xml/users.xml
3. App startup and checkpoint setup; handle screens; misc.
===========================================
usr/src/cmd/gui-install/src/gui-install.py
usr/src/cmd/gui-install/src/__init__.py
usr/src/cmd/gui-install/src/base_screen.py
usr/src/cmd/gui-install/src/screen_manager.py
usr/src/cmd/gui-install/src/gui_install_common.py
usr/src/cmd/gui-install/src/help_dialog.py
usr/src/cmd/gui-install/src/install_profile.py
4. Welcome Screen and Confirmation Screen
=============
usr/src/cmd/gui-install/src/welcome_screen.py
usr/src/cmd/gui-install/src/confirm_screen.py
5. Disk Screen
==========
usr/src/cmd/gui-install/src/disk_screen.py
usr/src/cmd/gui-install/src/fdisk_panel.py
usr/src/cmd/gui-install/src/target_utils.py
usr/src/lib/install_target/controller.py
6. Timezone screen
=============
usr/src/cmd/gui-install/src/timezone_screen.py
usr/src/cmd/gui-install/src/timezone.py
usr/src/cmd/gui-install/src/map.py
usr/src/cmd/gui-install/src/libzoneinfo_ctypes.py
7. User Screen
==========
usr/src/cmd/gui-install/src/user_screen.py
usr/src/cmd/gui-install/src/user_support.py
8. Progress Screen / Perform Installation
===========================
usr/src/cmd/gui-install/src/progress_screen.py
usr/src/cmd/gui-install/src/failure_screen.py
usr/src/cmd/gui-install/src/finish_screen.py
usr/src/cmd/gui-install/src/textview_dialog.py
usr/src/cmd/gui-aux/imageurl.txt
usr/src/cmd/system-config/profile/user_info.py
_______________________________________________
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
_______________________________________________
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
