Paul, Thanks for your comments, I missed out on few of your comments last time, and those are addressed in this update.
http://cr.opensolaris.org/~wittyman/wxwidgets/ I request all to review the updated code changes at the link above. Mayuresh Paul Cunningham wrote: > Mayuresh, > > More comments below from quick skip through ... > > Paul > > Mayuresh Nirhali wrote: > >> Thanks for your valuable comments. >> and I apologize for not having sent the updated webrev sooner. >> >> I have addressed your comments and the modified webrev is living at, >> http://cr.opensolaris.org/~wittyman/wxwidgets/ >> >> Apart from your comments, there are some minor modifications in the >> 'depend' file. >> I request everyone on this list to review this code change. >> > > >> Paul Cunningham wrote: >> >>> Mayuresh Nirhali wrote: >>> >>>> I am integrating wxWidgets shared libraries into SFW. >>>> Please note that, wxWidgets already exists in SFW, but in static form >>>> (for pgAdmin3). >>>> This code review is to expose the wxGTK libraries as a shared >>>> component, so that other GUI tools can also use them (e.g. audacity, >>>> filezilla). >>>> >>>> I would really appreciate a code review from the community for my >>>> changes. >>>> >>>> The webrev lives at, >>>> http://cr.opensolaris.org/~wittyman/wxwidgets/ >>>> >>>> and the bug report is at, >>>> http://bugs.opensolaris.org/view_bug.do?bug_id=6739008 >>>> >>>> References: >>>> wxWidgets: A cross-platform GUI toolkit >>>> http://www.wxwidgets.org/ >>>> >>> ==== Start of Comments === >>> >>> 1. usr/src/cmd/postgres/pgadmin/Makefile.sfw >>> & usr/src/lib/wxwidgets/Makefile.sfw >>> As you are changing these you might want to use the >>> predefined 'configure --prefix=...' value from >>> Makefile.master and you could also set up the >>> configure options using CONFIGURE_OPTIONS method, >>> eg., something like ... >>> CONFIGURE_OPTIONS += --bindir=$(CFGPREFIX)/pgadmin3/bin >>> CONFIGURE_OPTIONS += --libexecdir=/usr/pgadmin3/bin >>> ... >>> ... >>> ./configure $(CONFIGURE_OPTIONS) >>> There should be various examples of doing it this >>> way in the gate now, eg. cups, etc. >>> >>> You also might want to use 'env - ' rather than 'env ' >>> > > You might want to apply 'Roland Mainz' comments ... > - Use "env - ..." and not "env ..." in the Makefiles to > make sure "configure" & "make" only see the env > variables they should really get(and not pick-up > any random env variable) > - Use either $(SHELL) or /usr/bin/bash for "configure" > calls (so we know which one is used and "configure" > doesn't pick one itself). > > (done in wxwidgets/Makefile.sfw) > > >>> 2. usr/src/cmd/postgres/pgadmin/Makefile.sfw >>> Shouldn't the 'CPPFLAGS=...' line be before the >>> './configure' line ? >>> >>> 3. usr/src/lib/wxwidgets/man/wx-config.1 >>> & usr/src/lib/wxwidgets/man/wxrc.1 >>> Do these need the sun stability stuff added in someway; >>> either in this files or via sunman-stability script? >>> > > In usr/src/lib/wxwidgets/install-sfw I think you need to change the > '_install N ..' to '_install M ..' so you get the sunman-stability stuff > added. > > >>> 4. usr/src/lib/wxwidgets/install-sfw >>> Again as you are changing this file you might want to >>> look at 'Roland Mainz' comments on other people's >>> install-sfw scripts and apply those comments - look >>> back through sfwnv-discuss for them. >>> >>> And do all these files really need the write permissions >>> bit set? (and in prototype* files) >>> > > Still got write bit set on /usr files? 644 > > and in .. > usr/src/pkgdefs/SUNWwxwidgets-devel/prototype_com > > > >>> 5. usr/src/pkgdefs/SUNWwxwidgets-devel/Makefile >>> & usr/src/pkgdefs/SUNWwxwidgets/Makefile >>> You could remove the null DATAFILES= line >>> >>> 6. usr/src/pkgdefs/SUNWwxwidgets-devel/pkginfo.tmpl >>> & usr/src/pkgdefs/SUNWwxwidgets/pkginfo.tmpl >>> Its more normal to put the version number at the end >>> of the DESC= line (not that it really matters) >>> >>> 7. everything else looks okay to me >>> >>> ==== End of Comments ===== >>> >>> > >
