Thanks Srirama and Paul's review, and I have finished modification for xmlrpc according to their comments, please review it the newer version again.
The webrev is still: http://cr.opensolaris.org/~angelali/xmlrpc-c/ Thanks, Angela angela ??: > 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 >
