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
