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/> 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
