Thanks Srirama's feedback. Here is new webrev: http://cr.opensolaris.org/~angelali/xmlrpc-c/
Thanks, Angela Srirama Sharma ??: > 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 ? Yes, that is my point. > > 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). Thanks for refer this dependency specify method. Modified as your comment. > > 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 >>
