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: > > 1. In Targetdirs, please add the include dir entry in alphabetical order. Oops. modified it. > > 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)"? > - 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? > - Please use CCC/CXX with "-norunpath" option just to make sure RPATH > is not set to the workspace build dir. Thanks, modified according to your suggestion. > > 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? > > 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 ... > - Just curious to know why is 'const' keyword being removed in the patch. Because there are definition conflict which will cause compile failure. Such as: Error: Cannot assign void(*)(const int) to extern "C" void(*)(int). xmlrpc_c::clientXmlTransport_libwww::clientXmlTransport_libwww(const std::string,const std::string ) /export/home2/hl150050/sfwnv/usr/src/lib/libxmlrpc-c/xmlrpc-c-1.06.31/src/cpp/libxmlrpc_client++.a(client.o) > > 5. In depend file, I guess you could add "SUNWopensslr" instead of > "SUNWopenssl-libraries" as ssl/crypto libraries are now shipped as > part of "SUNWopensslr". Yes, you are right. Done. > > 6. Please make sure all the files in your webrev has comments of the > format "<bugid> <synopsis>" As your comment. Thanks, Angela > > Thanks, > Srirama > > angela said the following on Friday 06 March 2009 04:27 PM: >> Hi, All >> >> I am porting "xmlrpc-c", programming libraries and related tools to >> help user write an XML-RPC server or client in C or C++. >> >> Can you please have a review on my code. The webrev is at: >> http://cr.opensolaris.org/~angelali/xmlrpc-c/ >> >> Thanks, >> Angela >> >> _______________________________________________ >> sfwnv-discuss mailing list >> sfwnv-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
