Dermot,
That depends, if you want to catch any exceptions from it with a specific name,
it does need to be imported, see bug:
7041360 Unable to catch exceptions from checkpoints created using path for
module
That is probably unnecessary now if we run the engine with debug=False, which is
something we only recently did.
We have opted for the import mechanism to ensure this still works.
Darren.
On 02/09/2011 15:08, Dermot McCluskey wrote:
> Matt,
>
>
> On 09/02/11 14:13, Matt Keenan wrote:
>> Thanks for the review dermot.
>>
>> On 09/02/11 11:09, Dermot McCluskey wrote:
>>> Hi Matt,
>>>
>>> Mostly nits:
>>>
>>>
>>> auto_install.py:
>>> line 74 - you haven't made any changes that require
>>> importing varshared
>>
>> Registering of the VarSharedDataset checkpoint requires the varshared
>> module to be imported.
>
> No it doesn't! In gui and textinstall you don't
> import it. The call to register_checkpoints()
> only passes strings containing the module
> path and class name, it does not require any
> definitions be imported from the module.
>
>
>>
>>> line 99 - how come the list CHECKPOINTS_*BEFORE*_TI
>>> includes TI itself?
>>
>> A good point I suppose, naming of the variable is possible poor, and
>> should be CHECKPOINTS_UPTO_TI or something similar. but it's been
>> called this and including TI checkpoint since CUD/AI was integrated
>> last May.
>>
>> If you feel strongly I can change it.
>
> No - it's fine.
>
>>
>>> line 310: if GUI and Text, you now pause before VarShared.
>>> Should that not also be the case in AI?
>>>
>>
>> The options here are very specific to stop before or after Target
>> Instantiation. Depending on whether -i or -I command line argument has
>> been passed in. This is unique to AI.
>
> OK
>
>>
>>
>>> gui-install/src/__init__.py
>>> line 47, nit: could you take the opportunity to alphabetically sort
>>> the imports within editing this statement?
>>
>> Done
>
> Thanks
>
>>
>>> line 50 - why move the definition of TARGET_INIT from
>>> gui_install_common to this module?
>>>
>>
>> As it's not referenced anywhere else, and that's what all other non
>> referenced checkpoint definitions do.
>
> ok
>
>>
>>> progress_screen.py
>>> line 49, nit: could you take the opportunity to alphabetically sort
>>> the imports within editing this statement?
>>
>> Done
>
> Thanks
>
>>
>>> ti_install.py
>>> line 244 - nit: can you remove unneeded trailing \ while you are
>>> editing this statement?
>>>
>>
>> Done
>
> thanks
>
>>
>>> varshared.py
>>> line 231 - remove commented code
>>
>> Actually I'd prefer to leave this here, as this will effectively be
>> uncommented when /var/shared is implemented, I've added another comment
>> above the for loop to indicate this.
>
> ok
>
>
> thanks,
> - dermot
>
>
>>
>> New webrev posted :
>>
>>
>> https://cr.opensolaris.org/action/browse/caiman/mattman/7048015.7049157.7049160.1/
>>
>>
>>
>> cheers
>>
>> Matt
>>
>>>
>>>
>>> - Dermot
>>>
>>>
>>> On 09/01/11 18:22, Matt Keenan wrote:
>>>> Hi,
>>>>
>>>> Can I get some eyes to look at the latest revision of the /var dataset
>>>> bugs :
>>>>
>>>> 7048015 AI should create a separate /var and shared area
>>>> 7049157 Text installer should create a separate /var and shared area
>>>> 7049160 GUI installer should create a separate /var and shared area
>>>>
>>>> webrev:
>>>>
>>>> https://cr.opensolaris.org/action/browse/caiman/mattman/7048015.7049157.7049160/
>>>>
>>>>
>>>>
>>>>
>>>> Originally coded to create both a in_be /var dataset and a globally
>>>> shared /var/shared dataset, this has been changed to simply create
>>>> /var in_be.
>>>>
>>>> This code has been through code review before, but as we are so close
>>>> to final build, another scan might be no harm.
>>>>
>>>> All the code required for creating /var/shared is still in place just
>>>> not called.
>>>>
>>>> /var/shared work will be undertaken for s11u1 timeframe.
>>>> _______________________________________________
>>>> 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
