Karen,
I looked at most of the files, excluding test files.
General:
Please run pep8 per gate README instructions. You have quite a few
pep8 issues in the code that need to be addressed. I will try to
refrain from listing specific pep8 issues but one or two might slip
past... :)
doc.insert_children can take a single object so there's no need
to make a list of one element. I see this in a large number of DOC
manipulation lines.
You've got a few import * lines in the files. If possible, can you
import exactly what you need? It makes debugging far simplier in the
future if we know exactly where Slice comes from instead of having to
check all the import * modules.
Also, make sure you run pylint to pick up any unused imports, unused
variables or unreachable code. Take pylint output with a HUGE grain
of salt though. It likes to complain about things it shouldn't.
Don't strive for a perfect score either. It's not worth doing,
usually.
usr/src/cmd/distro_const/checkpoints/boot_archive_configure.py
--------------------------------------------------------------
272: to keep things consistant, use CPIOSpec.UNINSTALL (this was
recently added to install_transfer/info.py)
278-280: For super long lines, a 4 space indent works best:
root_tr_software_node = self.doc.persistent.get_descendants( \
name=TRANSFER_ROOT, class_type=Software, not_found_is_err=True)[0]
320-356: Quick question here for a future RFE the SMF folks are
working on. Is this the spot where the installers specify exactly
which files/dirs are transfered to the system? I ask because there
will be a need to explictly *NOT* copy /etc/svc/repository.db to the
system. It would be awesome if it could be specified here.
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
---------------------------------------------------
276: see line 272 in boot_archive_configure.py
285: commented line?
usr/src/cmd/text-install/solaris_install/__init__.py
----------------------------------------------------
97: Please use dict() instead of {}
55, 228: Why not use install_target/libbe/be.be_list() ?
269-288: Cool. :)
usr/src/cmd/text-install/solaris_install/disk_selection.py
----------------------------------------------------------
224-225: 4 space indent (like boot_archive_configure.py:278)
234: get_children() returns a list even if there are no children.
if not self.disks:
should be all you need.
426: same as 234
usr/src/cmd/text-install/solaris_install/disk_window.py
-------------------------------------------------------
115: s/option/optional
211: can simply change to: if part_list:
215: can simply change to: if slice_list
707: change [] to list()
802, 865, 885: change "gb" to Size.gb_units (to be consistent)
862, 881: change "mb" to Size.mb_units
usr/src/cmd/text-install/solaris_install/fdisk_partitions.py
------------------------------------------------------------
162: see disk_selection.py:234
209: This .. should fail. all_parts will always be a list (it might
be empty though) so, it should change to:
if not all_parts:
found_parts = False
else:
found_parts = True
usr/src/cmd/text-install/solaris_install/install_progress.py
------------------------------------------------------------
146, s/,/as
usr/src/cmd/text-install/solaris_install/log_viewer.py
------------------------------------------------------
73-82: Can you simplify this with a with() clause?
try:
with open(self.install_data.log_location, "w") as log_file:
log_data = logfile.read()
except (OSError, IOError) as error:
self.log_data = _(.......)
usr/src/cmd/text-install/solaris_install/partition_edit_screen.py
-----------------------------------------------------------------
191-193: 4 space indent for long lines
usr/src/cmd/text-install/solaris_install/progress.py
----------------------------------------------------
37: change the argument list to default hostname="localhost" so you
don't have to do lines 38-41
71: no need for this line
99: s/,/as
105: use list()
137: extra line
usr/src/cmd/text-install/solaris_install/summary.py
---------------------------------------------------
182: change to dict()
203: commented line
usr/src/cmd/text-install/solaris_install/ti_install.py
------------------------------------------------------
118: change to list()
236: The default action is "create" so you can remove this line
237: add_zvol has a 'use' argument that you can set to "swap" so this
line can be removed
241, 242: same as 236, 237
247: Use action=IPSSPEC.UNINSTALL instead. (This was just recently
put back)
37, 312: Why not use install_target/libbe/be.py ?
usr/src/cmd/text-install/solaris_install/ti_install_utils.py
------------------------------------------------------------
309, 313: s/,/as
323: s/kb/Size.kb_units
333-343: Whoa. A os.popen() call! :) Can we update this to use
solaris_install.Popen() instead?
memory_size = 0
p = Popen.check_call(["/usr/sbin/prtconf"], stdout=Popen.STORE,
stderr=Popen.STORE, logger=LOGGER)
for line in p.stdout.splitlines():
if "Memory size:" in line:
memory_size = int(line.split()[2])
break
and get rid of all this try/execpt and os.popen() stuff?
usr/src/cmd/text-install/solaris_install/ti_target_utils.py
-----------------------------------------------------------
78-80: 4 space indent for long lines
83: get_children() returns a list even if there are no children.
86: if you want the first child, use get_first_child(). NOTE:
get_first_child does *NOT* return an empty list if the object has no
children. It returns None.
91-93: 4 space indent for long lines
100-102: 4 space indent for long lines
114-115: get_children() returns a list even if there are no children
133-134: get_children() returns a list even if there are no children
150-153: 4 space indent for long lines
174-176: get_children() returns a list even if there are no children
183-185: get_children() returns a list even if there are no children
193-194: get_children() returns a list even if there are no children
317: use list() instead of []
362: get_children() returns a list even if there are no children
365: get_children() returns a list even if there are no children
514, 604, 618: use list() instead of []
641: use Size.gb_units
699: get_children() returns a list even if there are no children
825-843: Did you know install_target.physical.Partition has both of
these methods defined? I'm not sure if you can use them though, but I
wanted to make sure you knew they were available already on Partition
objects. is_solaris() is also defined there.
932-933: you don't need all the parens
965: use list() instead of []
976: outer parens aren't needed
1042-1043, 1046-1047: outer parens aren't needed
usr/src/lib/install_ict/__init__.py
-----------------------------------
70: bump to 57 per CR 7039499
usr/src/lib/install_transfer/media_transfer.py
----------------------------------------------
Your C code is showing. :)
93: outer parens aren't needed
103-108: you don't need all the parens
151, 154: outer parens aren't needed
179, 181: outer parens aren't needed
190-193: outer parens aren't needed
301: change to: for line in p.stdout.splitlines()
330: outer parens aren't needed
361: commented line
On 4/26/11 3:10 PM, Drew Fisher wrote:
I'll look at everything, but I'll focus on groups 1 and 3.
-Drew
On 4/26/11 2:38 PM, Karen Tung wrote:
Hi everyone,
I am looking for reviewers for changes that move
the Text Installer to the CUD architecture.
I divided up the changes into the following 4 groups.
I would like to get at least 1 person looking at each group.
Please email me privately and let me know which group
you like to review.
I don't think a code walk through is needed, but if you think
having one will be useful, please let me know and I can set one
up on Thurs. AM.
The webrev is at:
http://cr.opensolaris.org/~ktung/text-to-cud/
Please provide me your comments as soon as possible.
I am more than happy to get multiple sets of comments as
you go through the code.
I would like to get most of your comments by COB next Monday, 5/2/2011.
Group 1: DC related changes
----------------------------
usr/src/cmd/distro_const/checkpoints/Makefile
usr/src/cmd/distro_const/checkpoints/boot_archive_configure.py
usr/src/cmd/distro_const/checkpoints/pkg_img_mod.py
usr/src/cmd/distro_const/checkpoints/xslt/Makefile
usr/src/cmd/distro_const/checkpoints/xslt/doc2_media_transfer.xslt
Group 2: Text Installer UI changes
-----------------------------------
usr/src/cmd/text-install/solaris_install/__init__.py
usr/src/cmd/text-install/solaris_install/disk_selection.py
usr/src/cmd/text-install/solaris_install/disk_window.py
usr/src/cmd/text-install/solaris_install/fdisk_partitions.py
usr/src/cmd/text-install/solaris_install/install_progress.py
usr/src/cmd/text-install/solaris_install/install_status.py
usr/src/cmd/text-install/solaris_install/log_viewer.py
usr/src/cmd/text-install/solaris_install/partition_edit_screen.py
usr/src/cmd/text-install/solaris_install/progress.py
usr/src/cmd/text-install/solaris_install/summary.py
usr/src/cmd/text-install/solaris_install/text-install
usr/src/cmd/text-install/solaris_install/ti_target_utils.py
usr/src/cmd/text-install/solaris_install/welcome.py
Group 3: Text Installer non-UI changes
--------------------------------------
usr/src/cmd/text-install/Makefile
usr/src/cmd/text-install/solaris_install/Makefile
usr/src/cmd/text-install/solaris_install/ti_install.py
usr/src/cmd/text-install/solaris_install/ti_install_utils.py
Group 4: Other changes
--------------------------------------
usr/src/cmd/slim-install/svc/net-assembly
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile.targ
usr/src/cmd/system-config/xslt/doc2sc_profile.xslt
usr/src/lib/install_ict/__init__.py
usr/src/lib/install_manifest/dtd/Makefile
usr/src/lib/install_manifest/dtd/media-transfer.dtd
usr/src/lib/install_transfer/Makefile
usr/src/lib/install_transfer/cpio.py
usr/src/lib/install_transfer/media_transfer.py
usr/src/pkg/manifests/install-distribution-constructor.mf
usr/src/pkg/manifests/system-install-text-install.mf
usr/src/pkg/manifests/system-library-install.mf
Thanks,
--Karen
_______________________________________________
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
