Hi Karen,
My comments for the UI portion or below. Mostly nits, other than the
Threading points I brought up in person.
Thanks,
Keith
General:
The directory structure used to be:
usr/src/cmd/text-install/osol_install/{text_install,profile}/<file>
Originally, this was so that the Text Installer UI could be run without
building the gate; I think that stopped working or became rather
complicated at some point since the Text Installer went back - in any
case, I think it's easier to run from the proto area these days.
Given that, instead of collapsing to text-install/solaris_install (which
initially confused me, looking at
text-install/solaris_install/__init__.py), can you just collapse it to:
usr/src/cmd/text-install
and
usr/src/cmd/text-install/profile
Logging:
In a few cases, I'll point out that it's better to use:
logger.debug("my message: %s", my_arg)
rather than
logger.debug("my message: " + my_arg)
The reason is that for the former, if the log level is set such that
debug statements are ignored, then the relatively expensive string
formatting operation will be skipped; in the latter, the concatenation
is done regardless of log level, which isn't desirable. For both the
cases I mention and the ones I don't, please update usage.
import *:
It's best to avoid 'import *' - it becomes difficult, when coming back
to the code later, to figure out where a given function was imported
from. I'd suggest replacing with 'import x as y; y.func(); y.variable" etc.
if <list> is None or len(<list>) == 0:
These sorts of checks can universally be replaced with a simple: "if
<list>:". Both empty lists and None return False in a boolean context.
=========
usr/src/cmd/text-install/solaris_install/__init__.py
41, 90: Nit: Spaces around '='
58/253: StderrCalledProcessError is a subclass of CalledProcessError;
based on your usage, you only need CalledProcessError
97: Maybe just me, but it would seem this is bending things a bit more
than needed - rather than create all these values here, can we not just
invoke eng.register_checkpoint() with desired arguments as part of
prepare_engine()? I don't see much gain by separating the data as such.
257, 345-348: Nit: Spacing
402: Would it be possible to log the install_data object here? Or does
that not provide the same value as before?
usr/src/cmd/text-install/solaris_install/disk_selection.py
113: Since you never read from the global LOGGER, you don't need to set
it as a global - the instances can all be replaced (in this file and
elsewhere) with logger (or LOGGER) = logging.getLogger(INSTALL_LOGGER_NAME)
128: This looks like it's no longer used and can be deleted.
247: There's an extraneous set of parentheses here.
257: This should reference NO_TARGETS still. Also, I think 229 should
probably use the NO_DISKS message.
319, 393: Can you clarify why the handling of GPT labeled disks is being
changed?
331: Nit: on_active() has a reference to target controller via
"disk_select.tc"; a separate parameter isn't strictly required
355: Nit: Change to:
LOGGER.debug("disk_selection.on_change_screen, saved_index: %s",
self.selected_disk_index)
367-389: This function should be collapsed into 359:start_discovery. It
shouldn't be necessary to run in a separate thread explicitly (see 362),
since you can invoke execute_checkpoints() without blocking.
wait_for_disks() on 170 needs to be integrated with some form of callback.
426: Change to "if not parts:"
usr/src/cmd/text-install/solaris_install/disk_window.py
250: Change to:
LOGGER.debug("have_partition: %s", self.has_partition_data)
337: Logging formatting
345/348: Since the statements are the same now, this can get pulled
outside the innermost 'if' block
458/460: If this doesn't need any special formatting, change %i to %s
and remove the cast to int()
545-546: I'm concerned to see this code removed. Admittedly, it was a
workaround for an issue I was having with the "size" field for deleted
logical partitions still appearing. Can you point me to the equivalent
replacement?
717-719: Nit: Replace with:
if in_use["used_by"]:
723, 852: I don't think it's correct to call activate_solaris_data. The
user will want the currently selected field to remain the same. Although
after seeing what was done here, I can see perhaps how 545-546 worked;
it's re-drawing the entire screen.
764: This function looks like it no longer does any work - please remove it.
usr/src/cmd/text-install/solaris_install/fdisk_partitions.py
162: Nit: "if not all_slices:"
209-212: Could be:
found_parts = bool(all_parts)
246: This seems to be a behavioral change. The 'if' block there was
intended to ensure that if the user selected a given option, moved
forward/backward screens, then came back to this screen, the selection
would be unchanged.
The functional behavior that needs to be preserved is: after getting to
the summary screen, I should be able to hit F2/F3 to bounce between any
set of screens without having my selections get modified from what I
first chose.
usr/src/cmd/text-install/solaris_install/install_progress.py
As with TD, the engine should handle the threading now.
usr/src/cmd/text-install/solaris_install/install_status.py
69/80: Should the reference to this object not get pulled from the
engine's DOC?
usr/src/cmd/text-install/solaris_install/log_viewer.py
Same question
usr/src/cmd/text-install/solaris_install/partition_edit_screen.py
115, 120, 125: I believe these need to be reverted.
177-202: Not the ideal place for this logic, but that's partly my fault.
Still, with the "old" stuff we had a function called
"create_default_layout()" to cover similar scenarios; perhaps
TargetController needs something like
create_default_{disk,partition,slice}_layout() functions?
Also, what happens with perform_final_validation() here - I doubt it
would fail, but what if it does?
270/282: Why is perform_final_validation() only called for
x86_slice_mode here?
usr/src/cmd/text-install/solaris_install/progress.py
No comments
usr/src/cmd/text-install/solaris_install/summary.py
48: Nit: Spaces around '='
203: Lingering commented line
usr/src/cmd/text-install/solaris_install/text-install
No comments
usr/src/cmd/text-install/solaris_install/ti_target_utils.py
51, 52, 53, 55: These variables seem like they should be 'owned' by
something in target
75/78: The must_have_target_disk parameter seems at odds with the usage.
For example, if there is not target disk, the list returned on 78 by the
get_descendants call would be length 0, correct? So there would be an
immediate IndexError from trying to get the [0] item from that list, or
from trying to call get_children on line 81.
83-84: Seems odd to raise a RuntimeError here when the DOC has its own
mechanism for raising errors when an expected object isn't found.
Some of the other functions in this file suffer from similar concerns.
537, 553: "Wrong class" errors should be TypeError
571, 580: Nit: Might just be me, but I think these should be ValueErrors
(or AttributeErrors?)
566-569: Nit: With how frequently this if/else block is repeated, it
might be convenient to have an "obj()" method. Then these could be
replaced with:
return self.obj().name
Where obj() returns self.doc_obj or self.empty_space_obj as appropriate
(or raises Value/Attribute/RuntimeError if ui_type isn't properly set)
obj() could also be a property, if desired
738-741: Nit: could be:
return (discovered_size_byte - size_byte > precision)
UIDisk/UIPartition/UISlice: There's room to condense this file with a
proper abstract parent class. There's quite a bit of very similar code
in functions of the same name. Admittedly an existing problem, of sorts,
and not immediately critical; though filing a bug to follow-up and
address this later would be good, if there's not time/inclination to do
so now.
1116: I'm concerned by the hardcoding of 'rpool' here (and elsewhere).
Is this going to limit us, should we, say, decide to allow the user to
enter an arbitrary name for the ROOT POOL in the future?
usr/src/cmd/text-install/solaris_install/welcome.py
No comments
On 04/26/11 01: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.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
