Hi Jack, On 02/27/10 05:03 PM, Jack Schwartz wrote: >> >> Similarly, functions such as >> ai_parse_manifest.py:ai_start_manifest_server aren't really needed; >> the amount of C code to call a Python module function vs. the amount >> of C code to call a Python class method is the same. > > While your point is well taken, I don't want to veer too far away from > the original scope of my project. My intent isn't to optimize AI so > much as to get Driver Update to work with it. I have filed bug > 14926 - auto-installer C-Python interface code can be optimized > to address this in the future.
Works for me. I'm well aware that my eye tends to wander (i.e., introduce scope creep) when I code review! >>>> >>>> install_utils/ManifestServ.py: >>>> General: Many functions seem to have a keep_temp_files parameter. I >>>> don't see a consumer of this parameter; is it obsolete? >>> It is consumed by schema_validate(), set_defaults() and >>> semantic_validate(). >>>> If not, should it be an attribute of the ManifestServ object, >>>> instead of something that must be passed around through multiple >>>> functions? >>> The reason it is passed around is so the user of ManifestServ can >>> choose the stages at which to save the files. > I have changed the code now so that a user of ManifestServ can choose > different files if desired, but can also take defaults set up by > __init__. This, I believe, addresses your original concerns while > maintaining flexibility. That's cool! I was actually fine with the original reasoning, but I'm not opposed to the added flexibility either. - Keith >>>> >>>> 298: Instead of providing two separate methods for creating a >>>> ManifestServ object (__init__ and full_setup()), can you add an >>>> optional parameter to the __init__ function, and perform the >>>> additional setup if that parameter is set to True (defaulting to >>>> whichever is the more common scenario)? The function full_setup can >>>> remain, and could just be called if the parameter indicates it >>>> should be (though it would no longer need to be a staticmethod). >>> Yes, I like this idea. That would remove the need for changing DC >>> and install-tools/ManifestServ.py To achieve this, I'll make the >>> default to do the full setup as it was doing before. >>> >>> Look for an updated review after I go through the rest of the comments. >>> >>> Thanks again for reviewing. >> >> You're welcome. > I'll post a new webrev after I retest. I've made lots of changes... > > Thanks, > Jack >> >> - Keith >> >>> >>> Jack >>>> >>>> >>>> >>>> On 02/ 9/10 06:24 PM, Jack Schwartz wrote: >>>>> Hi everyone. >>>>> >>>>> Here is the code review for AI changes to incorporate Driver Update. >>>>> >>>>> Driver Update will allow the installer to install packages of >>>>> needed drivers in its own boot environment before performing the >>>>> installation, and then install those same packages on the target. >>>>> Packages may be explicitly specified in the manifest. They can >>>>> also be found via search, also initiated via the manifest. >>>>> >>>>> http://cr.opensolaris.org/~schwartz/100209.1/webrev/index.html >>>>> >>>>> This webrev is only of AI changes needed to call into the DDU >>>>> library. The DDU team will be posting a separate webrev with >>>>> their library changes. >>>>> >>>>> Testing is ongoing. Testing done: >>>>> - Validation of correct and incorrect manifest entries. >>>>> - Proper calling of DDU library, for explicit package entries >>>>> - Proper calling of DDU library to complete a search and resulting >>>>> installation. >>>>> - Verification that packages are installed when library is >>>>> correctly called, or else sending feedback to the DDU team about >>>>> found library issues. >>>>> - Verified that ManifestServ and DistroConstructor >>>>> >>>>> Please review and send comments/questions by Tues 2/23 COB. >>>>> >>>>> Thanks to the DDU team for helping me on their end, to get this done. >>>>> >>>>> Thanks, >>>>> Jack >>>>> _______________________________________________ >>>>> caiman-discuss mailing list >>>>> caiman-discuss at opensolaris.org >>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >
