Hi Angela, The new webrev looks good to me.
Thanks, Srirama angela said the following on Wednesday 18 March 2009 01:55 PM: > 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 >>> >
