Hi Harshal, looking at updated webrev, I can see that you addressed 1st part of code review comments, but not the 2nd part about slightly reorganizing the code. If you just overlooked it, I am fine with the current webrev, as the 2nd change I was proposing was cosmetic and you addressed the one which I think is important from debugging and observability point of view. So please send me your changes produced by 'hg bundle' and I will push them - assuming that there are no objections from other people and you answer my private question sent in separate email :-)
Thank you, Jan Harshal wrote: > Hello, > > I have made the changes to the patch as suggested by Jan. > > http://cr.opensolaris.org/~herch/rev02/webrev/ > <http://cr.opensolaris.org/%7Eherch/rev02/webrev/> > > > > The patch is tested as follows, > > Test -1 > 1. apply the patch > 2. rebuild the AI image > 3. keep the AI manifest to default value - babel_install > Result - No issues. > > Test-2 > 1. same as 1 and 2 above > 3. Modify AI manifest so that only minimal required packages are > included, remove babel_install > Result - No issues. > > > Please review the patch and let me know what could be the next step > for me to get this thing in caiman's next release. > > > Harshal > > On Fri, Sep 4, 2009 at 4:30 PM, Jan Damborsky <Jan.Damborsky at sun.com > <mailto:Jan.Damborsky at sun.com>> wrote: > > Hi Harshal, > > > Harshal wrote: > > Hi, > > Please review the webrev for the patch which is suppose to fix > bug 5558 > (http://defect.opensolaris.org/bz/show_bug.cgi?id=5558). The > webrev location is, > http://cr.opensolaris.org/~herch/rev01/webrev/ > <http://cr.opensolaris.org/%7Eherch/rev01/webrev/> > <http://cr.opensolaris.org/%7Eherch/rev01/webrev/> > > > The fix looks good, I have just couple of minor comments: > > * I think we should record the case when ICT is skipped > * I might recommend to slightly simplify the fix for > better readability > > the code might be changed in following way: > > 1447 browserInstallstatus = > os.path.exists('/usr/lib/firefox/browserconfig.properties') > 1448 if browserInstallstatus == True: > > [...] > > 1478 else: > 1479 return 0 > > -> > > if not os.path.exists('/usr/lib/firefox/browserconfig.properties'): > info_msg('Skipping fix_browser_home_page() ICT task as > /usr/lib/firefox/browserconfig.properties doesn't exist') > return 0 > > [...] > > Also please use bug number and synopsis as commit message - > e.g. in your case > > $ hg commit -m "5558 RFE: Install-finish > ICT_FIX_BROWSER_HOME_PAGE_FAILED must be soft" > > If the changes are already committed in your local workspace, you > could > > $ hg recommit -m "5558 RFE: Install-finish > ICT_FIX_BROWSER_HOME_PAGE_FAILED must be soft" > > > > > > We were in looking to install the just bare minimal > installation of opensolaris so that we can decrease the > provisioning time for new systems where don't really require > firefox or other bells and whistles of a Desktop Environment. > Caiman installer by default looks for the presence of the > firefox before it completes the installation. Because of this > we were unable to drop firefox from our AI manifest which also > meant bringing in lot of GUI libs which we will never use. > > So in order to address the issue, I wrote a patch which will > let the installation go ahead and succeed even if firefox is > not present. However if the firefox is present it will do what > it used to (set the homepage). > > To test this patch, I followed steps, > > Test -1 > 1. apply the patch > 2. rebuild the AI image > 3. keep the AI manifest to default value - babel_install > Result - No issues. > > Test-2 > 1. same as 1 and 2 above > 3. Modify AI manifest so that only minimal required packages > are included, remove babel_install > Result - No issues. > > > Please let me know what could be the next step to get this > thing into main caiman release. > > > If other agrees, I might recommend following procedure (assuming > that you already > went through the required paper work - you filed Sun Contributor > Agreement). > > [1] Process all code review comments and do the suggested changes > > [2] Retest using the same test procedures > > [3] Resend the updated webrev for another round of code review - > make sure > that 'hg pbchk' is fine with modifications. > > [4] As soon as people who review the changes are fine with the > final state, > I could take the modifications from you and integrate > - you would send me file created by means of 'hg bundle' as Dave > suggested > - I would 'hg unbundle' and push the change into our gate > > [5] I would change state of bug 5558 to RESOLVED-FIXINSOURCE and > update it with > the information about changeset which delivered the fix. > > Jan > >
