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. >> 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 ) >> 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". 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 Thanks, Srirama -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090312/423ee953/attachment.html>
