Hi Angela, Just one comment.
In Makefile.sfw, I see that you have included a new target "$(ROOT)/usr/include/curl/curlbuild.h" as below: > 49 all64: *$(ROOT)/usr/include/curl/curlbuild.h* $(VER64)/config.status > 50 (cd $(VER64); env - \ > 51 CC="$(CC64)" \ > 52 CXX="$(CCC64)" \ > 53 CFLAGS="$(CFLAGS64)" \ > 54 CXXFLAGS="$(CXXFLAGS64)" \ > 55 LD_LIBRARY_PATH="$(ROOT)/usr/lib/$(MACH64)" \ > 56 CFLAGS_PERSONAL="$(CFLAGS64)" \ > 57 LADD="-m64 -norunpath" \ > 58 "ROOT=$(ROOT)" \ > 59 $(GMAKE)) > 60 > 61 install: all > 62 (PKGVERS=$(VER) \ > 63 $(SHELL) ./install-sfw) > 64 (PKGVERS=$(VER64) \ > 65 MACH64=$(MACH64) $(SHELL) ./install-sfw-64) > 66 > * 67 $(ROOT)/usr/include/curl/curlbuild.h: > 68 (cd ../curl; $(MAKE) -f Makefile.sfw install)* Is this to make sure that curl gets built before your component (xmlrpc-c) during a nightly build ? If yes, then you could remove this target (those which are marked in bold) from your Makefile.sfw and add the dependency in 'usr/src/lib/Makefile' by adding "libxmlrpc-c: curl" entry at the end of Makefile. This is to make sure that your component gets built only after curl gets built during a nightly build. It is advisable to specify such dependencies in higher level Makefile (i.e either usr/src/lib/Makefile or usr/src/cmd/Makefile). Rest of the changes looks good to me. Thanks, Srirama angela said the following on Friday 13 March 2009 03:32 PM: > Thanks Srirama. > Still webrev: http://cr.opensolaris.org/~angelali/xmlrpc-c/ > > Srirama Sharma ??: >> Hi Angela, >> >> Please see my reply in line. >> >> angela said the following on Wednesday 11 March 2009 01:02 PM: >>> Thanks Srirama for your review. >>> >>> The new webrev is generated: >>> http://cr.opensolaris.org/~angelali/xmlrpc-c/ >>> >>> Thanks, >>> Angela >>> >>> Srirama Sharma ??: >>>> Hi Angela, >>>> >>>> Below are few comments: >>>> >>>> 2. In Makefile.sfw, >>>> - you may want to invoke configure as "$(SHELL) ./configure >>>> $(CONFIGURE_OPTIONS)" >>> If use $(SHELL), that is ksh93, configure will fail. So do you think >>> it is ok if use "$(SH) ./configure $(CONFIGURE_OPTIONS)"? >> >> Its okay with me. >> >>>> - Please make sure you have same env set before invoking configure >>>> as well as before invoking gmake. >>> I used "env -" before configure and gmake. I suppose this will >>> accomplish your concern, right? >> >> What I suggested was to have same set of env's while running >> configure as well as while doing a gmake. >> >> The CFLAGS and CXXFLAGS are not being set for 32 bit target >> "$(VER)/config.status". you could add the lines marked in bold. >> 57 $(VER)/config.status: $(VER)/configure >> 58 (cd $(VER); env - \ >> 59 CC="$(CC)" \ >> 60 CXX="$(CCC)" \ >> * "CFLAGS=$(CFLAGS)" \ >> "CXXFLAGS=$(CXXFLAGS)" \ >> * 61 MAKE=$(GMAKE) \ >> 62 $(SH) ./configure $(CONFIGURE_OPTIONS)) >> >> Also, you could set all above env variables (CC, CXX, CFLAGS, >> CXXFLAGS) in targets all32 and all64 before invoking gmake. > Got what you mean. Modified as your comment. >> >> >>>> 3. "usr/src/lib/libxmlrpc-c/curl.fix" is probably not required as >>>> the fix for CR6770465 is now present in the sfw-gate. Also >>>> "usr/src/lib/libxmlrpc-c/curlbuild.h is not required. you shouldn't >>>> be having a separate copy of curlbuild.h in your product dir. >>>> Instead you should be using it from the proto area of your workspace. >>> It is hard to let source including proto area. >>> Actually it works fine if the build server is no less that snv_109, >>> which contains fix for CR6770465, >>> So I'd remove these two files, do you think ok? >> >> Currently all sfw public build machines have snv_107. So I don't >> think the build would be successful on them. >> >> Hence I had suggested you to get the curl headers from the proto area. >> In your configure, if you have something like CURL_CFLAGS then you >> could set that to "'-I ${ROOT}/usr/include " and then make use of it >> later in your Makefiles. >> >> (You could refer to similar changes that I have made to configure.in >> at below link: >> http://cr.opensolaris.org/~srirama/openwsman/usr/src/cmd/openwsman/openwsman.patch.html >> >> ) > Ok, as your comment. >> >> >>>> 4. In usr/src/lib/libxmlrpc-c/ltmain.patch, >>>> - Code changes as below may not be accepted by the community if you >>>> intend to give this back upstream. >>>> >>>> 38 -#include <sys/unistd.h> >>>> 39 +#include <unistd.h> >>>> >>>> Instead you could do something like below: >>>> >>>> #ifdef (__sun) >>>> #include <unistd.h> >>>> #else >>>> #include <sys/unistd.h> >>>> #endif >>> Thanks, modified for every .cpp files in patch. >>> #ifdef __sun >>> ... >> Using "#ifdef (__sun)" would be appropriate in cases where you are >> checking for Solaris OS specific differences. >> For Eg: In above case where you are including "unistd.h". > I used "#ifdef (__sun)" and met compile failure: > Error: Identifier expected instead of "(". > I checked in the workspace, and found some codes using "#ifdef __sun". > > Just found that the following can work: > #if defined (__sun) >> >> In places where you are removing 'const' keyword, it is more a >> compiler specific change and not OS specific change. So I would >> suggest you to use either >> "#if defined (__SUNPRO_C) or #if defined (__SUNPRO_CC)" macros which >> are meant to check if the compiler being used is Sun Studio. >> >> Something like this : >> 20 static void * 21 +#ifdef (__SUNPRO_CC)* >> 22 +sigterm(int signalClass) { >> 23 +#else >> 24 sigterm(int const signalClass) { >> 25 +#endif > Used "#if defined (__SUNPRO_CC)" as your comment. >> >> >> Thanks, >> Srirama > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090318/0b4abd98/attachment.html>
