Thrift
  1. Thrift
  2. THRIFT-1031

Patch to compile Thrift for vc++ 9.0 and 10.0

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8
    • Component/s: C++ - Library
    • Labels:
      None
    • Environment:

      Windows XP 32bit, vc++ 9.0, 10.0

    • Patch Info:
      Patch Available

      Description

      At our company we need clients running on Windows being able to connect to our linux servers running hypertable. The attached patch enables the parts needed by Hypertable to be compiled on Windows using either the VC++ 9.0 or 10.0 compilers.

      Having read previous posts about ports using boost::asio we found these to be too intrusive for our needs. This version uses pthreads_win32 and winsock2 and is as designed to be as un-intrusive as is possible to the original unix code base. It is mostly #defines between unix sockets and winsock2 sockets. We also tried to follow the folder structuring of the C# runtime that has visual studio solutions to be consistent.

      More details are in the README as not all the functionality of the original unix code base is available to windows users. We will add the missing functionality, we just wanted to share what we had as for a Windows based client for us it is sufficient.

      The patch is based on the latest revision in SVN, we would love feedback and any code reviews. If there is any possibility of this being added to the main trunk then that would be much appreciated, however we don't expect that.

      1. PosixThreadFactory.patch
        0.9 kB
        Peace C
      2. thrift_msvc_v0_1.patch
        77 kB
        James Dickson
      3. thrift_msvc_v0_2.patch
        294 kB
        James Dickson
      4. thrift_msvc_v0_3.patch
        97 kB
        James Dickson
      5. thrift_msvc_v0_4.patch
        81 kB
        James Dickson
      6. thrift_msvc_v0_5.patch
        81 kB
        James Dickson
      7. thrift_msvc_v0_6.patch
        83 kB
        James Dickson
      8. thrift_msvc_v0_7.patch
        55 kB
        James Dickson
      9. thrift_msvc.patch
        40 kB
        James Dickson

        Issue Links

          Activity

          Hide
          James Dickson added a comment -

          Patch to add switch for vc++ compilers.

          Show
          James Dickson added a comment - Patch to add switch for vc++ compilers.
          Hide
          Bryan Duxbury added a comment -

          Any CPP people out there who could pass judgment on this patch?

          Show
          Bryan Duxbury added a comment - Any CPP people out there who could pass judgment on this patch?
          Hide
          Christian Lavoie added a comment -

          A few issues:

          1) Config.h is a bad name – ./configure already creates a config.h, and w\e the windows configuration mechanism ends up being, it should be completely and obviously different from what ./configure outputs

          2) I really don't think we wanna sprinkle the code with any more ifdefs than there already are – I'd be more comfortable with slowly creating a set of utility functions that are reimplemented for each major platform group as needed (we've already run in this problem with Darwin's lack of clock_gettime(); and in a more distant fashion with supporting unix sockets).

          Or just using the Apache Portable Runtime, which solves this by making it Someone Else's Problem.

          3) I'm not particularly enthused by having all files including a superset of the needed headers through including them all in to Config.h – if anything we should reduce the (transitive closure of the) number of headers included per cpp file

          Show
          Christian Lavoie added a comment - A few issues: 1) Config.h is a bad name – ./configure already creates a config.h, and w\e the windows configuration mechanism ends up being, it should be completely and obviously different from what ./configure outputs 2) I really don't think we wanna sprinkle the code with any more ifdefs than there already are – I'd be more comfortable with slowly creating a set of utility functions that are reimplemented for each major platform group as needed (we've already run in this problem with Darwin's lack of clock_gettime(); and in a more distant fashion with supporting unix sockets). Or just using the Apache Portable Runtime, which solves this by making it Someone Else's Problem. 3) I'm not particularly enthused by having all files including a superset of the needed headers through including them all in to Config.h – if anything we should reduce the (transitive closure of the) number of headers included per cpp file
          Hide
          James Dickson added a comment -

          Thank you for the feedback, it is greatly appreciated.

          I will look into making those changes and try to get a new patch submitted as soon as possible.

          Show
          James Dickson added a comment - Thank you for the feedback, it is greatly appreciated. I will look into making those changes and try to get a new patch submitted as soon as possible.
          Hide
          Dragan Okiljevic added a comment -

          Working on my project I needed to create Thrift clients and servers both in C++ and Java, and to have C++ interoperability on Windows and *NIX platforms.

          I found a great deal of help in this patch by Mr. Dickson's and I extended it with server functionality.

          The result is published as THRIFT-1123.

          I express my gratitude to Mr. Dickson and I am looking forward for any comments.

          Show
          Dragan Okiljevic added a comment - Working on my project I needed to create Thrift clients and servers both in C++ and Java, and to have C++ interoperability on Windows and *NIX platforms. I found a great deal of help in this patch by Mr. Dickson's and I extended it with server functionality. The result is published as THRIFT-1123 . I express my gratitude to Mr. Dickson and I am looking forward for any comments.
          Hide
          James Dickson added a comment -

          All,

          Thank you once again for all the comments both in this issue and in THRIFT-1123. It has really helped me.

          I have attached a new patch which is what I currently have as regards combining all the comments, however it is by no means complete - it just compiles the client side code at the moment. I have posted it mainly for a (hopefully) quick code review to make sure I am not going down the wrong path.

          To outline the changes:

          • Platform specifics have been removed and replaced by using the Apache Portable Runtime (APR) v1.4.2
          • Config.h has been renamed to win32_config.h and been moved into "build_windows/msvc##/" where ## is the compiler version.
          • By using APR very few headers are now included in this new config header, just enough to ensure POSIX types are available.
          • Tests for client, concurrency, realloc and server have been added, but do not yet compile. (Work in progress)

          To reply to some comments:

          @Christian Lavoie:

          why did you put (std::min) in parentheses in TBufferTransports.cpp ?

          This is because somewhere in a windows header (can't remember which one) min and max have been defined as macros and as far as I am aware cannot be turned off. The parentheses make sure the STL min\max versions get chosen instead. A pain I know!

          @Dragan Okiljevic: Thank you for working on the server side of things, I really appreciate it. However, in switching to using APR I have probably broken your changes. After I have finished the client side of things, I will endeavor to fix the server side for you.

          Again, thank you for any comments and feedback.
          James

          Show
          James Dickson added a comment - All, Thank you once again for all the comments both in this issue and in THRIFT-1123 . It has really helped me. I have attached a new patch which is what I currently have as regards combining all the comments, however it is by no means complete - it just compiles the client side code at the moment. I have posted it mainly for a (hopefully) quick code review to make sure I am not going down the wrong path. To outline the changes: Platform specifics have been removed and replaced by using the Apache Portable Runtime (APR) v1.4.2 Config.h has been renamed to win32_config.h and been moved into "build_windows/msvc##/" where ## is the compiler version. By using APR very few headers are now included in this new config header, just enough to ensure POSIX types are available. Tests for client, concurrency, realloc and server have been added, but do not yet compile. (Work in progress) To reply to some comments: @Christian Lavoie: why did you put (std::min) in parentheses in TBufferTransports.cpp ? This is because somewhere in a windows header (can't remember which one) min and max have been defined as macros and as far as I am aware cannot be turned off. The parentheses make sure the STL min\max versions get chosen instead. A pain I know! @Dragan Okiljevic: Thank you for working on the server side of things, I really appreciate it. However, in switching to using APR I have probably broken your changes. After I have finished the client side of things, I will endeavor to fix the server side for you. Again, thank you for any comments and feedback. James
          Hide
          Dragan Okiljevic added a comment -

          Christian and Roger, thank you for your feedback. The comments, advices and propositions you gave are really of big help.

          James, it's great to see the improved version of your patch. As for the server side, thank you for your intention to fix it in a fashion applied in your new patch, if I can be of any help, I encourage you to contact me at any time.

          Show
          Dragan Okiljevic added a comment - Christian and Roger, thank you for your feedback. The comments, advices and propositions you gave are really of big help. James, it's great to see the improved version of your patch. As for the server side, thank you for your intention to fix it in a fashion applied in your new patch, if I can be of any help, I encourage you to contact me at any time.
          Hide
          Roger Meier added a comment -

          Good job!
          I have one concern, the dependency on apr. I'm very happy to have another runtime dependency for Thrift. We use Thrift on very small systems and do not like to pull in additional dependencies.
          We need just need a little subset of APR. Are there any other alternatives to achieve this?

          Show
          Roger Meier added a comment - Good job! I have one concern, the dependency on apr . I'm very happy to have another runtime dependency for Thrift. We use Thrift on very small systems and do not like to pull in additional dependencies. We need just need a little subset of APR. Are there any other alternatives to achieve this?
          Hide
          David Reiss added a comment -

          I agree that that dependency is not acceptable, especially for such an invasive change to TSocket. My proposed solution would be

          • For ctime_r, only use APR on Windows.
          • For TSocket, instead of commenting out huge swaths of the code, just fork the class into TAprSocket. TSocket would continue to not build on Windows, but TAprSocket would be the alternative. Unix users could switch if they wanted to include the extra dependency.
          Show
          David Reiss added a comment - I agree that that dependency is not acceptable, especially for such an invasive change to TSocket. My proposed solution would be For ctime_r, only use APR on Windows. For TSocket, instead of commenting out huge swaths of the code, just fork the class into TAprSocket. TSocket would continue to not build on Windows, but TAprSocket would be the alternative. Unix users could switch if they wanted to include the extra dependency.
          Hide
          James Dickson added a comment -

          Thank you for the quick feedback.

          @Roger + David: I agree that the extra dependency is not ideal, especially as such a small part of it is used. The alternative I see would be to simply use Winsock2 on Windows as the previous patch did. If that would be the way to go then incorporating previous comments, changes to the previous patch would need to be:

          1. Create a "platform.h" header, which on *nix machines includes "config.h" (generated by ./configure) and header files which are not found on Windows. This new header would be used in code where "config.h" had been included directly previously. (This is based on Roger's comment on 29/Mar/11 19:10, THRIFT-1123 part a.)

          2. (1.) Would not fix situations in which *nix only headers have been included in a file. Replacing these headers with "platform.h" would pull in headers that file would not require, which has been a point of contention previously. However, if this is the only work around then maybe it is acceptable?

          @Roger: As regards part b. of your comment in THRIFT-1123 on 29/Mar/11 19:10, I like that solution and will make the change.

          @Roger: As regards part c. of your comment in THRIFT-1123 on 29/Mar/11 19:10, it is a yes and no answer. Yes in that the Winsock2 equivalent of those functions are named completely differently. For instance fcntl(...) becomes ioctlsocket(...). However, as far as requiring them to be macros then no. Maybe an alternative is to make TSocket a template class where one of the template parameters is an OS socket layer policy. This would mean the implementation of low level socket routines could exist in platform specific folders (along with gettimeofday(...) on Windows) and leave the TSocket class almost the same.

          Just to avoid any confusion in the above statement, TSocket would become:

          
          template< class SOCKET_POLICY >
          class TSocket : public TVirtualTransport<TSocket>, private SOCKET_POLICY {
          
          ...
          
          }
          
          Where SOCKET_POLICY implements the following:
          
          close(...)
          usleep(...)
          poll(...)
          reset(...)
          blocking(...)
          non_blocking(...)
          
          

          If people are happy with the proposal I will start those changes, but would appreciate any comments before hand.

          Show
          James Dickson added a comment - Thank you for the quick feedback. @Roger + David: I agree that the extra dependency is not ideal, especially as such a small part of it is used. The alternative I see would be to simply use Winsock2 on Windows as the previous patch did. If that would be the way to go then incorporating previous comments, changes to the previous patch would need to be: 1. Create a "platform.h" header, which on *nix machines includes "config.h" (generated by ./configure) and header files which are not found on Windows. This new header would be used in code where "config.h" had been included directly previously. (This is based on Roger's comment on 29/Mar/11 19:10, THRIFT-1123 part a.) 2. (1.) Would not fix situations in which *nix only headers have been included in a file. Replacing these headers with "platform.h" would pull in headers that file would not require, which has been a point of contention previously. However, if this is the only work around then maybe it is acceptable? @Roger: As regards part b. of your comment in THRIFT-1123 on 29/Mar/11 19:10, I like that solution and will make the change. @Roger: As regards part c. of your comment in THRIFT-1123 on 29/Mar/11 19:10, it is a yes and no answer. Yes in that the Winsock2 equivalent of those functions are named completely differently. For instance fcntl(...) becomes ioctlsocket(...). However, as far as requiring them to be macros then no. Maybe an alternative is to make TSocket a template class where one of the template parameters is an OS socket layer policy. This would mean the implementation of low level socket routines could exist in platform specific folders (along with gettimeofday(...) on Windows) and leave the TSocket class almost the same. Just to avoid any confusion in the above statement, TSocket would become: template< class SOCKET_POLICY > class TSocket : public TVirtualTransport<TSocket>, private SOCKET_POLICY { ... } Where SOCKET_POLICY implements the following: close(...) usleep(...) poll(...) reset(...) blocking(...) non_blocking(...) If people are happy with the proposal I will start those changes, but would appreciate any comments before hand.
          Hide
          Deepak Muley added a comment -

          Thanks a lot James for starting this patch.

          I also reviewed the makefile project (namke) based patch in THRIFT-591 but I preferred your way of doing it without makefiles.

          I downloaded your patch and works great for creating libthrift.lib. looking forward to have this patch make it to the next release.

          Show
          Deepak Muley added a comment - Thanks a lot James for starting this patch. I also reviewed the makefile project (namke) based patch in THRIFT-591 but I preferred your way of doing it without makefiles. I downloaded your patch and works great for creating libthrift.lib. looking forward to have this patch make it to the next release.
          Hide
          Roger Meier added a comment -

          David's approach, creating a TAprSocket and TAprServerSocket seems to be a good solution with minimal changes to the existing implementation and that's a good thing!

          The template approach is a good solution for all these Socket variants we have. I think it is worth to have that refactoring task as a dedicated issue within jira.

          However, I suggest to implement a TAprSocket and TAprServerSocket and do the template stuff later.

          Show
          Roger Meier added a comment - David's approach, creating a TAprSocket and TAprServerSocket seems to be a good solution with minimal changes to the existing implementation and that's a good thing! The template approach is a good solution for all these Socket variants we have. I think it is worth to have that refactoring task as a dedicated issue within jira. However, I suggest to implement a TAprSocket and TAprServerSocket and do the template stuff later.
          Hide
          Xavier Lepaul added a comment -

          Hi,
          I have a small issue with the patch (I'm using the v0.1): the generated code for enums does not compile. I have a simple enum

          enum AccountType {CHECKING, SAVINGS}

          In the generated AccountType_types.cpp, the following line fails to compile:

          const std::map<int, const char*> _AccountType_VALUES_TO_NAMES(::apache::thrift::TEnumIterator(2, _kAccountTypeValues, _kAccountTypeNames), ::apache::thrift::TEnumIterator(-1, NULL, NULL));

          with the following error:

          1>c:\program files\microsoft visual studio 9.0\vc\include\xutility(764) : error C2039: 'iterator_category' : is not a member of 'apache::thrift::TEnumIterator'
          1>        e:\tools\thrift\thrift-0.6.0-patched\lib\cpp\src\thrift.h(50) : see declaration of 'apache::thrift::TEnumIterator'
          1>        c:\program files\microsoft visual studio 9.0\vc\include\xutility(1598) : see reference to class template instantiation 'std::iterator_traits<_Iter>' being compiled
          1>        with
          1>        [
          1>            _Iter=apache::thrift::TEnumIterator
          1>        ]
          1>        c:\program files\microsoft visual studio 9.0\vc\include\map(120) : see reference to function template instantiation 'void std::_Debug_range<_Iter>(_InIt,_InIt,const wchar_t *,unsigned int)' being compiled
          1>        with
          1>        [
          1>            _Iter=apache::thrift::TEnumIterator,
          1>            _InIt=apache::thrift::TEnumIterator
          1>        ]
          1>        e:\temp\serial_cpp\gen-cpp\accounttype_types.cpp(18) : see reference to function template instantiation 'std::map<_Kty,_Ty>::map<apache::thrift::TEnumIterator>(_Iter,_Iter)' being compiled
          1>        with
          1>        [
          1>            _Kty=int,
          1>            _Ty=const char *,
          1>            _Iter=apache::thrift::TEnumIterator
          1>        ]
          

          Any idea? Everything else is working fine, though. Thanks for the patch!

          Show
          Xavier Lepaul added a comment - Hi, I have a small issue with the patch (I'm using the v0.1): the generated code for enums does not compile. I have a simple enum enum AccountType {CHECKING, SAVINGS} In the generated AccountType_types.cpp, the following line fails to compile: const std::map< int , const char *> _AccountType_VALUES_TO_NAMES(::apache::thrift::TEnumIterator(2, _kAccountTypeValues, _kAccountTypeNames), ::apache::thrift::TEnumIterator(-1, NULL, NULL)); with the following error: 1>c:\program files\microsoft visual studio 9.0\vc\include\xutility(764) : error C2039: 'iterator_category' : is not a member of 'apache::thrift::TEnumIterator' 1> e:\tools\thrift\thrift-0.6.0-patched\lib\cpp\src\thrift.h(50) : see declaration of 'apache::thrift::TEnumIterator' 1> c:\program files\microsoft visual studio 9.0\vc\include\xutility(1598) : see reference to class template instantiation 'std::iterator_traits<_Iter>' being compiled 1> with 1> [ 1> _Iter=apache::thrift::TEnumIterator 1> ] 1> c:\program files\microsoft visual studio 9.0\vc\include\map(120) : see reference to function template instantiation 'void std::_Debug_range<_Iter>(_InIt,_InIt,const wchar_t *,unsigned int)' being compiled 1> with 1> [ 1> _Iter=apache::thrift::TEnumIterator, 1> _InIt=apache::thrift::TEnumIterator 1> ] 1> e:\temp\serial_cpp\gen-cpp\accounttype_types.cpp(18) : see reference to function template instantiation 'std::map<_Kty,_Ty>::map<apache::thrift::TEnumIterator>(_Iter,_Iter)' being compiled 1> with 1> [ 1> _Kty=int, 1> _Ty=const char *, 1> _Iter=apache::thrift::TEnumIterator 1> ] Any idea? Everything else is working fine, though. Thanks for the patch!
          Hide
          Dragan Okiljevic added a comment - - edited

          @Xavier

          I got the same problem while trying to compile Thrift generated C++ code for enums using Visual Studio 2010. The map is supposed to be initialized using two iterator arguments (first, and last), so it will containt key/value pairs for each enumeration value.

          Quick & dirty solution:
          Last time I inspected the code, it seemed that this map is not used, so my code worked when I simply removed it from generated code by commenting it from Thrift generated files. (Worked not only for compilation, but for runtime usage of enums aswell).

          I'm not sure why MSVC doesn't recognize TEnumIterator as an iterator type. This class overloads '*' operator and returns std::pair<int, const char *> which seems to work on *nix, but not MSVC. Am I right?

          Anyway, there is many ways to populate std::map either during initialization or later, so I guess there is one both compatible with MSVC and *nix and Mac OSX compilers.

          I would suggest creation of a new issue for this problem, as it is lossely connected to the main topic of this one.

          Update: discussion on this issue continued at THRIFT-1139

          Show
          Dragan Okiljevic added a comment - - edited @Xavier I got the same problem while trying to compile Thrift generated C++ code for enums using Visual Studio 2010. The map is supposed to be initialized using two iterator arguments (first, and last), so it will containt key/value pairs for each enumeration value. Quick & dirty solution: Last time I inspected the code, it seemed that this map is not used, so my code worked when I simply removed it from generated code by commenting it from Thrift generated files. (Worked not only for compilation, but for runtime usage of enums aswell). I'm not sure why MSVC doesn't recognize TEnumIterator as an iterator type. This class overloads '*' operator and returns std::pair<int, const char *> which seems to work on *nix, but not MSVC. Am I right? Anyway, there is many ways to populate std::map either during initialization or later, so I guess there is one both compatible with MSVC and *nix and Mac OSX compilers. I would suggest creation of a new issue for this problem, as it is lossely connected to the main topic of this one. Update: discussion on this issue continued at THRIFT-1139
          Hide
          Deepak Muley added a comment -

          I just observed that am facing lot of linker errors related to following:

          main.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl apache::thrift::concurrency::ReadWriteMutex::release(void)const " (?release@ReadWriteMutex@concurrency@thrift@apache@@UEBAXXZ)
          main.obj : error LNK2001: unresolved external symbol "public: virtual bool __cdecl apache::thrift::concurrency::ReadWriteMutex::attemptWrite(void)const " (?attemptWrite@ReadWriteMutex@concurrency@thrift@apache@@UEBA_NXZ)
          main.obj : error LNK2001: unresolved external symbol "public: virtual bool __cdecl apache::thrift::concurrency::ReadWriteMutex::attemptRead(void)const " (?attemptRead@ReadWriteMutex@concurrency@thrift@apache@@UEBA_NXZ)
          main.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl apache::thrift::concurrency::ReadWriteMutex::acquireWrite(void)const " (?acquireWrite@ReadWriteMutex@concurrency@thrift@apache@@UEBAXXZ)
          main.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl apache::thrift::concurrency::ReadWriteMutex::acquireRead(void)const " (?acquireRead@ReadWriteMutex@concurrency@thrift@apache@@UEBAXXZ)

          After going thru the libthrift.vcproj, observed that Mutex.cpp and many other .cpp files are excluded from build. Is it intended or its just present on my machine?

          Show
          Deepak Muley added a comment - I just observed that am facing lot of linker errors related to following: main.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl apache::thrift::concurrency::ReadWriteMutex::release(void)const " (?release@ReadWriteMutex@concurrency@thrift@apache@@UEBAXXZ) main.obj : error LNK2001: unresolved external symbol "public: virtual bool __cdecl apache::thrift::concurrency::ReadWriteMutex::attemptWrite(void)const " (?attemptWrite@ReadWriteMutex@concurrency@thrift@apache@@UEBA_NXZ) main.obj : error LNK2001: unresolved external symbol "public: virtual bool __cdecl apache::thrift::concurrency::ReadWriteMutex::attemptRead(void)const " (?attemptRead@ReadWriteMutex@concurrency@thrift@apache@@UEBA_NXZ) main.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl apache::thrift::concurrency::ReadWriteMutex::acquireWrite(void)const " (?acquireWrite@ReadWriteMutex@concurrency@thrift@apache@@UEBAXXZ) main.obj : error LNK2001: unresolved external symbol "public: virtual void __cdecl apache::thrift::concurrency::ReadWriteMutex::acquireRead(void)const " (?acquireRead@ReadWriteMutex@concurrency@thrift@apache@@UEBAXXZ) After going thru the libthrift.vcproj, observed that Mutex.cpp and many other .cpp files are excluded from build. Is it intended or its just present on my machine?
          Hide
          Deepak Muley added a comment -

          Found the reason and solution for the above linker error in THRIFT-1123

          Show
          Deepak Muley added a comment - Found the reason and solution for the above linker error in THRIFT-1123
          Hide
          Dragan Okiljevic added a comment - - edited

          @Deepak Muley
          You may find the following comment: https://issues.apache.org/jira/browse/THRIFT-1123?focusedCommentId=13021475&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13021475 useful for both THRIFT-1123 and (possibly) THRIFT-1031. It describes two additional minor interventions needed to make your MSVC 2008/2010 Thrift project work like a charm.

          Show
          Dragan Okiljevic added a comment - - edited @Deepak Muley You may find the following comment: https://issues.apache.org/jira/browse/THRIFT-1123?focusedCommentId=13021475&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13021475 useful for both THRIFT-1123 and (possibly) THRIFT-1031 . It describes two additional minor interventions needed to make your MSVC 2008/2010 Thrift project work like a charm.
          Hide
          Deepak Muley added a comment -

          @Dragan Okiljevic
          Thank you.

          Show
          Deepak Muley added a comment - @Dragan Okiljevic Thank you.
          Hide
          Carl Steinbach added a comment -

          This patch depends on pthreads_win32 library which carries an LGPL license. The ASF License FAQ lists the LGPL in the prohibited category, though it's unclear to me if this prohibition is limited to creating a derivative work from LGPL'd code, or if it also applies to runtime dependencies against standalone LGPL libraries. Does anyone know the answer?

          Show
          Carl Steinbach added a comment - This patch depends on pthreads_win32 library which carries an LGPL license. The ASF License FAQ lists the LGPL in the prohibited category, though it's unclear to me if this prohibition is limited to creating a derivative work from LGPL'd code, or if it also applies to runtime dependencies against standalone LGPL libraries. Does anyone know the answer?
          Hide
          alexandre parenteau added a comment -

          I have created a fork of latest thrift 0.6.1 on github, which comprises part of this patch, and also uses THRIFT-923 (non-blocking server with libevent). If interested, please refer to https://github.com/aubonbeurre/thrift/blob/alex-0.6.1/README.non.blocking.Windows.

          Show
          alexandre parenteau added a comment - I have created a fork of latest thrift 0.6.1 on github, which comprises part of this patch, and also uses THRIFT-923 (non-blocking server with libevent). If interested, please refer to https://github.com/aubonbeurre/thrift/blob/alex-0.6.1/README.non.blocking.Windows .
          Hide
          Deepak Muley added a comment -

          Carl, Did you get any answer (privately) to your LGPL license related question, am curious to know the answer as well.

          Show
          Deepak Muley added a comment - Carl, Did you get any answer (privately) to your LGPL license related question, am curious to know the answer as well.
          Hide
          James Dickson added a comment -

          Hi all,

          Please accept my apologies for the long radio silence, other tasks have somewhat occupied my time lately.

          However, over the past week I have been working on a newer version of the patch, one which I hope addresses all concerns and requests. I greatly appreciate all the feedback you have given me.

          This patch leaves the original code base completely unchanged. Where classes are completely incompatible with windows I have rewritten the incompatible parts and branched the implementation thus maintaining the same interface. It is not complete, there are still some Transport classes needing to still be ported, but the majority has been.

          I have also ported the compiler to compile under MSVC, as far as I can tell it works in it's entirety, however I would really appreciate testing of the various other languages as I have mainly concentrated on the C++ side of code generation. I have only given a cursory run of the other language generators.

          As to why I ported the compiler, this is so I can generate the testing code so that I can port all the test cases and make sure the library is equal to it's linux counterpart.

          From the readme:

          Using Thrift with C++
          =====================

          You need to define an enviroment variable called THIRD_PARTY. The project
          assumes that you have extracted the dependancies into their default structure
          into the path defined by THIRD_PARTY.

          e.g. $(THIRD_PARTY)/boost/boost_1_47_0/

          Thrift is divided into two libraries.

          libthrift
          The core Thrift library contains all the core Thrift code. It requires
          boost shared pointers and pthreads_win32.

          libthriftnb
          This library contains the Thrift nonblocking server, which uses libevent.
          To link this library you will also need to link libevent.

          You MUST apply this patch to make generated code compile.
          https://issues.apache.org/jira/browse/THRIFT-1139

          Linking Against Thrift
          ======================

          You need to link your project that uses thrift against all the thrift
          dependancies; in the case of libthrift, pthreads_win32, boost and for
          libthriftnb, libevent.

          In the project properties you must also set HAVE_CONFIG_H as force include
          the config header: "windows/confg.h"

          Dependencies
          ============

          boost shared pointers
          http://www.boost.org/libs/smart_ptr/smart_ptr.htm

          libevent (for libthriftnb only)
          http://monkey.org/~provos/libevent/

          pthreads win32
          http://sources.redhat.com/pthreads-win32/

          Known issues
          ============

          • Endianess has not been fully tested, may not work with doubles.
          • Currently does not support the non-blocking connect path.
          • Only supports the creation of clients, server sockets are not yet ported.
          • Does not support named pipes. (Supported in unix through unix domain sockets).

          TODO
          ====

          • Port remaining classes in libthrift:
          • PosixThreadFactory
          • TFDTransport
          • TFileTransport
          • THttpClient
          • THttpServer
          • TSimpleFileTransport
          • TSSLSocket
          • TServerSocket
          • Port remaing classes in libthriftnb:
          • TNonblockingServer
          • Port test cases. (Not even started this. Run test cases in release mode?)
          • Autolink libraries depending on debug\release build.
          • Auto versioning.
          Show
          James Dickson added a comment - Hi all, Please accept my apologies for the long radio silence, other tasks have somewhat occupied my time lately. However, over the past week I have been working on a newer version of the patch, one which I hope addresses all concerns and requests. I greatly appreciate all the feedback you have given me. This patch leaves the original code base completely unchanged. Where classes are completely incompatible with windows I have rewritten the incompatible parts and branched the implementation thus maintaining the same interface. It is not complete, there are still some Transport classes needing to still be ported, but the majority has been. I have also ported the compiler to compile under MSVC, as far as I can tell it works in it's entirety, however I would really appreciate testing of the various other languages as I have mainly concentrated on the C++ side of code generation. I have only given a cursory run of the other language generators. As to why I ported the compiler, this is so I can generate the testing code so that I can port all the test cases and make sure the library is equal to it's linux counterpart. From the readme: Using Thrift with C++ ===================== You need to define an enviroment variable called THIRD_PARTY. The project assumes that you have extracted the dependancies into their default structure into the path defined by THIRD_PARTY. e.g. $(THIRD_PARTY)/boost/boost_1_47_0/ Thrift is divided into two libraries. libthrift The core Thrift library contains all the core Thrift code. It requires boost shared pointers and pthreads_win32. libthriftnb This library contains the Thrift nonblocking server, which uses libevent. To link this library you will also need to link libevent. You MUST apply this patch to make generated code compile. https://issues.apache.org/jira/browse/THRIFT-1139 Linking Against Thrift ====================== You need to link your project that uses thrift against all the thrift dependancies; in the case of libthrift, pthreads_win32, boost and for libthriftnb, libevent. In the project properties you must also set HAVE_CONFIG_H as force include the config header: "windows/confg.h" Dependencies ============ boost shared pointers http://www.boost.org/libs/smart_ptr/smart_ptr.htm libevent (for libthriftnb only) http://monkey.org/~provos/libevent/ pthreads win32 http://sources.redhat.com/pthreads-win32/ Known issues ============ Endianess has not been fully tested, may not work with doubles. Currently does not support the non-blocking connect path. Only supports the creation of clients, server sockets are not yet ported. Does not support named pipes. (Supported in unix through unix domain sockets). TODO ==== Port remaining classes in libthrift: PosixThreadFactory TFDTransport TFileTransport THttpClient THttpServer TSimpleFileTransport TSSLSocket TServerSocket Port remaing classes in libthriftnb: TNonblockingServer Port test cases. (Not even started this. Run test cases in release mode?) Autolink libraries depending on debug\release build. Auto versioning.
          Hide
          Jake Farrell added a comment -

          The introduction of the duplicate set of t_*_generators in the v0_2 patch just add the potential for patches to be missed when new bugs arise within the generators. This functionality should be added into the current generators as an option flag to modify the resulted generator output where necessary. If I misunderstood your comments and this is just for debugging and will not be in the final patch can you please provide it as a separate patch noted as just for debug.

          Show
          Jake Farrell added a comment - The introduction of the duplicate set of t_*_generators in the v0_2 patch just add the potential for patches to be missed when new bugs arise within the generators. This functionality should be added into the current generators as an option flag to modify the resulted generator output where necessary. If I misunderstood your comments and this is just for debugging and will not be in the final patch can you please provide it as a separate patch noted as just for debug.
          Hide
          James Dickson added a comment -

          With version 0.2 of the patch, the py and go generates make a call to chmod. I have attached a new version of the patch with the changes applied directly to the generators removing the need to have duplicates. It simply does not call chmod when compiling under msvc.

          Show
          James Dickson added a comment - With version 0.2 of the patch, the py and go generates make a call to chmod. I have attached a new version of the patch with the changes applied directly to the generators removing the need to have duplicates. It simply does not call chmod when compiling under msvc.
          Hide
          James Dickson added a comment -

          Version 0_4 adds some missing headers and enables code to be compiled with iterator debugging enabled.

          Show
          James Dickson added a comment - Version 0_4 adds some missing headers and enables code to be compiled with iterator debugging enabled.
          Hide
          James Dickson added a comment -

          Attached a new version (0_5) with some bug fixes that I have encountered during testing today.

          Show
          James Dickson added a comment - Attached a new version (0_5) with some bug fixes that I have encountered during testing today.
          Hide
          alexandre parenteau added a comment -

          @James: I reviewed the patch, and have those remarks to share:

          Overall:
          ========

          Pros:

          • This is a nice improvement to have a 0.8 patch for Windows
          • Almost everything in thrift is ported (however see also Cons)
          • IMHO this could be the base patch for the upcoming Windows support inside thrift

          Cons:

          • the server part is missing (as you mentioned), and/or especially async servers is incomplete (libevent and pthread based, very important for high-performance thrift serving)
          • the new socket-windows-only code is troubling to me (see below for more details, and possible alternatives)
          • it lacks a 64 bits target (should be trivial to add)

          Details:
          ========

          Those observations come mostly from my own experiment of maintaining a Windows port: see https://github.com/aubonbeurre/thrift/blob/winthriftnb-0.8.x-1/README.non.blocking.Windows

          My focus has been on supporting async server/client, so in a way I feel it is very complimentary to this patch.

          I was hoping you could review my observations, and see if somehow you could blend the two together!

          TSocket:

          • It is not immediately obvious to me why creating a different socket class for Windows is necessary. On the github link above, TSocket has only a couple of minor changes, and run/compile fine on Windows.
          • HOWEVER: Much more importantly, I don't think it will work for multi-thread servers to use errno (see http://msdn.microsoft.com/en-us/library/ms737828(v=vs.85).aspx and http://msdn.microsoft.com/en-us/library/windows/apps/ms737828(v=vs.85).aspx)
          • My own approach was to re-define errno to WSAGetLastError (only internally to thrift), and all the errno.h codes: It works as long as errno is not getting used inside any thrift .h (or it would force clients linking against thrift to do the same).
          • So taking together those approaches (slight changes to the existing TSocket, and override of errno) seem better to me, even if they require special attention to never use errno inside .h. Also errors getting thrown by thrift have now the WinSock error codes, and it works in multiple threads (like the pool thread).

          lib\cpp\src\concurrency\PosixThreadFactory.cpp:

          • In the port mentioned above, you'll find the minor tweaks to have it compile and run on Windows (it was taken from another JIRA patch, perhaps even this one!)

          lib\cpp\src\server\TNonblockingServer.cpp:

          Lately Roger integrated a set of patches of mine which make the compilation on Windows happen without any changes. However see in the code above the file win32-config.h, which defines SOCKOPT_CAST_T and AF_LOCAL to make this work.

          TWinsockSingleton:

          • I'm not sure this is necessary to have thrift initialize WinSock (especially since libevent might already do this, dunno). I choose to make it a documentation point.

          Thanks, I feel overall the windows port is getting closer and closer.

          Show
          alexandre parenteau added a comment - @James: I reviewed the patch, and have those remarks to share: Overall: ======== Pros: This is a nice improvement to have a 0.8 patch for Windows Almost everything in thrift is ported (however see also Cons) IMHO this could be the base patch for the upcoming Windows support inside thrift Cons: the server part is missing (as you mentioned), and/or especially async servers is incomplete (libevent and pthread based, very important for high-performance thrift serving) the new socket-windows-only code is troubling to me (see below for more details, and possible alternatives) it lacks a 64 bits target (should be trivial to add) Details: ======== Those observations come mostly from my own experiment of maintaining a Windows port: see https://github.com/aubonbeurre/thrift/blob/winthriftnb-0.8.x-1/README.non.blocking.Windows My focus has been on supporting async server/client, so in a way I feel it is very complimentary to this patch. I was hoping you could review my observations, and see if somehow you could blend the two together! TSocket: It is not immediately obvious to me why creating a different socket class for Windows is necessary. On the github link above, TSocket has only a couple of minor changes, and run/compile fine on Windows. HOWEVER: Much more importantly, I don't think it will work for multi-thread servers to use errno (see http://msdn.microsoft.com/en-us/library/ms737828(v=vs.85).aspx and http://msdn.microsoft.com/en-us/library/windows/apps/ms737828(v=vs.85).aspx ) My own approach was to re-define errno to WSAGetLastError (only internally to thrift), and all the errno.h codes: It works as long as errno is not getting used inside any thrift .h (or it would force clients linking against thrift to do the same). So taking together those approaches (slight changes to the existing TSocket, and override of errno) seem better to me, even if they require special attention to never use errno inside .h. Also errors getting thrown by thrift have now the WinSock error codes, and it works in multiple threads (like the pool thread). lib\cpp\src\concurrency\PosixThreadFactory.cpp: In the port mentioned above, you'll find the minor tweaks to have it compile and run on Windows (it was taken from another JIRA patch, perhaps even this one!) lib\cpp\src\server\TNonblockingServer.cpp: Lately Roger integrated a set of patches of mine which make the compilation on Windows happen without any changes. However see in the code above the file win32-config.h, which defines SOCKOPT_CAST_T and AF_LOCAL to make this work. TWinsockSingleton: I'm not sure this is necessary to have thrift initialize WinSock (especially since libevent might already do this, dunno). I choose to make it a documentation point. Thanks, I feel overall the windows port is getting closer and closer.
          Hide
          James Dickson added a comment -

          @alexandre

          Thank you for your comments, I mostly agree, however to make some further points:

          PosixThreadFactory & TNonblockingServer - I will take a look at what you have and integrate those today (16th Sept).

          TSocket:

          I have taken a look at what you have, and whilst it is indeed minimal, the main reason for re-implementing it for Windows was in response to David's proposal of creating TAprSocket, except I wasn't really a fan of using APR as it is yet another dependency to include. By using #'defs as you have done I kind of feel it is going down the route of trying to fit a square peg into a round hole (maybe I am being pedantic!).

          I think so long as the windows implementation of the TSocket interface fulfills the obligations and behavior of that interface then my preference would be to have an all windows implementation. Whilst what I have currently is merely a duplication with the non windows code ported over, eventually I would like to implement an entire windows version.

          Errno:
          Note that a windows specific implementation would also fix the errno issue as it would be possible to use WSAGetLastError directly as well as the windows error codes without the special care currently required in your version.

          I have also taken a look at your implementation of "fcntl" and it doesn't seem to follow the specification here, mainly throwing an integer value of -99 isn't specified: http://pubs.opengroup.org/onlinepubs/009695399/functions/fcntl.html

          I'd really like this patch to be integrated into the main trunk, so if there is anything more I can do to make this happen please let me know.

          Show
          James Dickson added a comment - @alexandre Thank you for your comments, I mostly agree, however to make some further points: PosixThreadFactory & TNonblockingServer - I will take a look at what you have and integrate those today (16th Sept). TSocket: I have taken a look at what you have, and whilst it is indeed minimal, the main reason for re-implementing it for Windows was in response to David's proposal of creating TAprSocket, except I wasn't really a fan of using APR as it is yet another dependency to include. By using #'defs as you have done I kind of feel it is going down the route of trying to fit a square peg into a round hole (maybe I am being pedantic!). I think so long as the windows implementation of the TSocket interface fulfills the obligations and behavior of that interface then my preference would be to have an all windows implementation. Whilst what I have currently is merely a duplication with the non windows code ported over, eventually I would like to implement an entire windows version. Errno: Note that a windows specific implementation would also fix the errno issue as it would be possible to use WSAGetLastError directly as well as the windows error codes without the special care currently required in your version. I have also taken a look at your implementation of "fcntl" and it doesn't seem to follow the specification here, mainly throwing an integer value of -99 isn't specified: http://pubs.opengroup.org/onlinepubs/009695399/functions/fcntl.html I'd really like this patch to be integrated into the main trunk, so if there is anything more I can do to make this happen please let me know.
          Hide
          Roger Meier added a comment -

          Just comitted the first portion of your patch related to the compiler!

          I would prefer the TSocket aproach from alexandre, he made good progress on making it more portable.
          There are just a few #ifdef's and it will be much easier to manage the code base with one implementation.

          I'm looking forward to bring this into the next release!

          Show
          Roger Meier added a comment - Just comitted the first portion of your patch related to the compiler! I would prefer the TSocket aproach from alexandre, he made good progress on making it more portable. There are just a few #ifdef's and it will be much easier to manage the code base with one implementation. I'm looking forward to bring this into the next release!
          Hide
          Hudson added a comment -

          Integrated in Thrift #265 (See https://builds.apache.org/job/Thrift/265/)
          THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0 (partial)
          no chmod on windows for go and py compiler
          Patch: James Dickson

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1171520
          Files :

          • /thrift/trunk/compiler/cpp/src/generate/t_go_generator.cc
          • /thrift/trunk/compiler/cpp/src/generate/t_py_generator.cc
          Show
          Hudson added a comment - Integrated in Thrift #265 (See https://builds.apache.org/job/Thrift/265/ ) THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0 (partial) no chmod on windows for go and py compiler Patch: James Dickson roger : http://svn.apache.org/viewvc/?view=rev&rev=1171520 Files : /thrift/trunk/compiler/cpp/src/generate/t_go_generator.cc /thrift/trunk/compiler/cpp/src/generate/t_py_generator.cc
          Hide
          James Dickson added a comment -

          I have attached a new version (0_6) which includes suggestions from Alexandre.

          The following have now been ported:

          TNonblockingServer
          PosixThreadFactory
          TServerSocket

          I have also added the error code fixes for multi threading as Alexandre pointed out. In fact it is from Alexandre's patch, except instead of putting it into the global config include I have put it in a separate header which is only force included by TSocket and TServerSocket.

          Let me know what you think. I will continue pushing on to complete the porting of the remaining classes in the mean time.

          Show
          James Dickson added a comment - I have attached a new version (0_6) which includes suggestions from Alexandre. The following have now been ported: TNonblockingServer PosixThreadFactory TServerSocket I have also added the error code fixes for multi threading as Alexandre pointed out. In fact it is from Alexandre's patch, except instead of putting it into the global config include I have put it in a separate header which is only force included by TSocket and TServerSocket. Let me know what you think. I will continue pushing on to complete the porting of the remaining classes in the mean time.
          Hide
          Roger Meier added a comment -

          0_6 is committed!
          Thank you James, Alexandre and all other contributors to make this happen!!

          Show
          Roger Meier added a comment - 0_6 is committed! Thank you James, Alexandre and all other contributors to make this happen!!
          Hide
          Hudson added a comment -

          Integrated in Thrift #266 (See https://builds.apache.org/job/Thrift/266/)
          THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0
          Patch: James Dickson and Alexandre Parenteau

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1171777
          Files :

          • /thrift/trunk/compiler/cpp/src/windows
          • /thrift/trunk/compiler/cpp/src/windows/config.h
          • /thrift/trunk/compiler/cpp/src/windows/version.h
          • /thrift/trunk/lib/cpp/README_WINDOWS
          • /thrift/trunk/lib/cpp/libthrift.vcxproj
          • /thrift/trunk/lib/cpp/libthrift.vcxproj.filters
          • /thrift/trunk/lib/cpp/libthriftnb.vcxproj
          • /thrift/trunk/lib/cpp/libthriftnb.vcxproj.filters
          • /thrift/trunk/lib/cpp/src/concurrency/PosixThreadFactory.cpp
          • /thrift/trunk/lib/cpp/src/server/TNonblockingServer.h
          • /thrift/trunk/lib/cpp/src/transport/TServerSocket.cpp
          • /thrift/trunk/lib/cpp/src/transport/TSocket.cpp
          • /thrift/trunk/lib/cpp/src/windows
          • /thrift/trunk/lib/cpp/src/windows/Fcntl.cpp
          • /thrift/trunk/lib/cpp/src/windows/Fcntl.h
          • /thrift/trunk/lib/cpp/src/windows/GetTimeOfDay.cpp
          • /thrift/trunk/lib/cpp/src/windows/GetTimeOfDay.h
          • /thrift/trunk/lib/cpp/src/windows/Operators.h
          • /thrift/trunk/lib/cpp/src/windows/SocketPair.cpp
          • /thrift/trunk/lib/cpp/src/windows/SocketPair.h
          • /thrift/trunk/lib/cpp/src/windows/StdAfx.cpp
          • /thrift/trunk/lib/cpp/src/windows/StdAfx.h
          • /thrift/trunk/lib/cpp/src/windows/TWinsockSingleton.cpp
          • /thrift/trunk/lib/cpp/src/windows/TWinsockSingleton.h
          • /thrift/trunk/lib/cpp/src/windows/TargetVersion.h
          • /thrift/trunk/lib/cpp/src/windows/config.h
          • /thrift/trunk/lib/cpp/src/windows/force_inc.h
          • /thrift/trunk/lib/cpp/src/windows/tr1
          • /thrift/trunk/lib/cpp/src/windows/tr1/functional
          • /thrift/trunk/lib/cpp/thrift.sln
          Show
          Hudson added a comment - Integrated in Thrift #266 (See https://builds.apache.org/job/Thrift/266/ ) THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0 Patch: James Dickson and Alexandre Parenteau roger : http://svn.apache.org/viewvc/?view=rev&rev=1171777 Files : /thrift/trunk/compiler/cpp/src/windows /thrift/trunk/compiler/cpp/src/windows/config.h /thrift/trunk/compiler/cpp/src/windows/version.h /thrift/trunk/lib/cpp/README_WINDOWS /thrift/trunk/lib/cpp/libthrift.vcxproj /thrift/trunk/lib/cpp/libthrift.vcxproj.filters /thrift/trunk/lib/cpp/libthriftnb.vcxproj /thrift/trunk/lib/cpp/libthriftnb.vcxproj.filters /thrift/trunk/lib/cpp/src/concurrency/PosixThreadFactory.cpp /thrift/trunk/lib/cpp/src/server/TNonblockingServer.h /thrift/trunk/lib/cpp/src/transport/TServerSocket.cpp /thrift/trunk/lib/cpp/src/transport/TSocket.cpp /thrift/trunk/lib/cpp/src/windows /thrift/trunk/lib/cpp/src/windows/Fcntl.cpp /thrift/trunk/lib/cpp/src/windows/Fcntl.h /thrift/trunk/lib/cpp/src/windows/GetTimeOfDay.cpp /thrift/trunk/lib/cpp/src/windows/GetTimeOfDay.h /thrift/trunk/lib/cpp/src/windows/Operators.h /thrift/trunk/lib/cpp/src/windows/SocketPair.cpp /thrift/trunk/lib/cpp/src/windows/SocketPair.h /thrift/trunk/lib/cpp/src/windows/StdAfx.cpp /thrift/trunk/lib/cpp/src/windows/StdAfx.h /thrift/trunk/lib/cpp/src/windows/TWinsockSingleton.cpp /thrift/trunk/lib/cpp/src/windows/TWinsockSingleton.h /thrift/trunk/lib/cpp/src/windows/TargetVersion.h /thrift/trunk/lib/cpp/src/windows/config.h /thrift/trunk/lib/cpp/src/windows/force_inc.h /thrift/trunk/lib/cpp/src/windows/tr1 /thrift/trunk/lib/cpp/src/windows/tr1/functional /thrift/trunk/lib/cpp/thrift.sln
          Hide
          alexandre parenteau added a comment - - edited

          Hi, thanks all for making this happen!

          I had a chance finally to compile/run it (using the Calculator), and here are my comments/questions for @James:

          1- overall definitively this looks good: I guess there is no WIN64 target, because there is no pthread for WIN64?

          2- Cosmetic: something needs to define "#define HAVE_GETTIMEOFDAY" (windows/config.h?)

          3- Cosmetic: #define __BYTE_ORDER: this is not necessary anymore, another patch of mine previously fixed the auto detection on win32 using boost. The README may also reflect that (double should be fine).

          4- Cosmetic: typedef __kernel_size_t size_t, typedef __kernel_ssize_t ssize_t: I found this easier version: typedef ptrdiff_t ssize_t; (don't think size_t needs to be defined AFAIK)

          5- Cosmetic: #define NOMINMAX (before Windows.h, because std::min is used in thrift)

          6- Cosmetic warning: #pragma warning(disable: 4996) and #pragma warning(disable: 4250) could be nice ("POSIX name for this item is deprecated", "inherits via dominance")

          7- config.h: here is my biggest concern: you decided (honorably!) to re-use config.h (and define HAVE_CONFIG_H): I find it in practice annoying because it assumes "config.h" is at the same level of "Thrift.h". This forces in turn to add <thrift folder>/windows to the include path. What happens is when mixing with other libraries who also use config.h, it may take precedence over thrift's config.h. Ideally, I would prefer if not using HAVE_CONFIG_H, and do something like this instead:

          #ifdef _WIN32
          #include "windows/config.h"
          $endif

          When included from Thrift.h, this guarantees to pick the right config.h (and not Python's one for example).

          Now what happens for the source code .cpp which does not include Thrift.h? Well since you are using already force-compilation, you could add windows/config.h to the list for forced headers. I found this to work in practice.

          Again, thanks a lot for the patch!

          Show
          alexandre parenteau added a comment - - edited Hi, thanks all for making this happen! I had a chance finally to compile/run it (using the Calculator), and here are my comments/questions for @James: 1- overall definitively this looks good: I guess there is no WIN64 target, because there is no pthread for WIN64? 2- Cosmetic: something needs to define "#define HAVE_GETTIMEOFDAY" (windows/config.h?) 3- Cosmetic: #define __BYTE_ORDER: this is not necessary anymore, another patch of mine previously fixed the auto detection on win32 using boost. The README may also reflect that (double should be fine). 4- Cosmetic: typedef __kernel_size_t size_t, typedef __kernel_ssize_t ssize_t: I found this easier version: typedef ptrdiff_t ssize_t; (don't think size_t needs to be defined AFAIK) 5- Cosmetic: #define NOMINMAX (before Windows.h, because std::min is used in thrift) 6- Cosmetic warning: #pragma warning(disable: 4996) and #pragma warning(disable: 4250) could be nice ("POSIX name for this item is deprecated", "inherits via dominance") 7- config.h: here is my biggest concern: you decided (honorably!) to re-use config.h (and define HAVE_CONFIG_H): I find it in practice annoying because it assumes "config.h" is at the same level of "Thrift.h". This forces in turn to add <thrift folder>/windows to the include path. What happens is when mixing with other libraries who also use config.h, it may take precedence over thrift's config.h. Ideally, I would prefer if not using HAVE_CONFIG_H, and do something like this instead: #ifdef _WIN32 #include "windows/config.h" $endif When included from Thrift.h, this guarantees to pick the right config.h (and not Python's one for example). Now what happens for the source code .cpp which does not include Thrift.h? Well since you are using already force-compilation, you could add windows/config.h to the list for forced headers. I found this to work in practice. Again, thanks a lot for the patch!
          Hide
          James Dickson added a comment - - edited

          New version added - 0_7.
          @alexandre: 1. Yep, not sure what else to do short so if you have any suggestions please let me know. 2-7 are done, Please see the changes in 0_7.

          Also a couple of bug fixes with the non-blocking path. On windows the connect method returns EWOULDBLOCK instead of EINPROGRESS when non blocking is set on the socket.

          Show
          James Dickson added a comment - - edited New version added - 0_7. @alexandre: 1. Yep, not sure what else to do short so if you have any suggestions please let me know. 2-7 are done, Please see the changes in 0_7. Also a couple of bug fixes with the non-blocking path. On windows the connect method returns EWOULDBLOCK instead of EINPROGRESS when non blocking is set on the socket.
          Hide
          Aurélien Revol added a comment - - edited

          Hi all,

          Thanks for the Windows port, it's great work! My last obstacle to using it, though, is that I have to build it in VC++ 9.0; unfortunately, starting from thrift_msvc_v0_2.patch, the project files are VC++ 10.0 only. Is this a deliberate and necessary choice, or is there a way for me to undo this?

          Another question: a port to MinGW/MSYS would be great, since the rest of the project is based on the autotools, and we have native Win32 code here. Do you think these sources are compatible with MinGW? How much effort would be needed make it work with the autotools scripts?

          Show
          Aurélien Revol added a comment - - edited Hi all, Thanks for the Windows port, it's great work! My last obstacle to using it, though, is that I have to build it in VC++ 9.0 ; unfortunately, starting from thrift_msvc_v0_2.patch , the project files are VC++ 10.0 only . Is this a deliberate and necessary choice, or is there a way for me to undo this? Another question: a port to MinGW/MSYS would be great, since the rest of the project is based on the autotools, and we have native Win32 code here. Do you think these sources are compatible with MinGW? How much effort would be needed make it work with the autotools scripts?
          Hide
          Roger Meier added a comment -

          Thanks! just committed patch 0_7

          Show
          Roger Meier added a comment - Thanks! just committed patch 0_7
          Hide
          Hudson added a comment -

          Integrated in Thrift #275 (See https://builds.apache.org/job/Thrift/275/)
          THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0
          => some more improvements

          Patch: James Dickson

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1174801
          Files :

          • /thrift/trunk/compiler/cpp/compiler.sln
          • /thrift/trunk/compiler/cpp/compiler.vcxproj
          • /thrift/trunk/compiler/cpp/compiler.vcxproj.filters
          • /thrift/trunk/lib/cpp/libthrift.vcxproj
          • /thrift/trunk/lib/cpp/src/Thrift.h
          • /thrift/trunk/lib/cpp/src/transport/TSocket.cpp
          • /thrift/trunk/lib/cpp/src/windows/TargetVersion.h
          • /thrift/trunk/lib/cpp/src/windows/config.h
          • /thrift/trunk/lib/cpp/src/windows/force_inc.h
          Show
          Hudson added a comment - Integrated in Thrift #275 (See https://builds.apache.org/job/Thrift/275/ ) THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0 => some more improvements Patch: James Dickson roger : http://svn.apache.org/viewvc/?view=rev&rev=1174801 Files : /thrift/trunk/compiler/cpp/compiler.sln /thrift/trunk/compiler/cpp/compiler.vcxproj /thrift/trunk/compiler/cpp/compiler.vcxproj.filters /thrift/trunk/lib/cpp/libthrift.vcxproj /thrift/trunk/lib/cpp/src/Thrift.h /thrift/trunk/lib/cpp/src/transport/TSocket.cpp /thrift/trunk/lib/cpp/src/windows/TargetVersion.h /thrift/trunk/lib/cpp/src/windows/config.h /thrift/trunk/lib/cpp/src/windows/force_inc.h
          Hide
          alexandre parenteau added a comment -

          @james: another cosmetic change: wouldn't be better to use WSAPoll on >= Vista?

          #if WINVER <= 0x0502
          #define poll(fds, nfds, timeout) \
              poll_win32(fds, nfds, timeout)
          ...
          #else
          #   define poll(fds, nfds, timeout) \
              WSAPoll(fds, nfds, timeout)
          #endif // WINVER
          

          This is based on one the original windows port attempts, I updated it to match your latest version.

          Show
          alexandre parenteau added a comment - @james: another cosmetic change: wouldn't be better to use WSAPoll on >= Vista? # if WINVER <= 0x0502 #define poll(fds, nfds, timeout) \ poll_win32(fds, nfds, timeout) ... # else # define poll(fds, nfds, timeout) \ WSAPoll(fds, nfds, timeout) #endif // WINVER This is based on one the original windows port attempts, I updated it to match your latest version.
          Hide
          alexandre parenteau added a comment -

          For the problem of a WIN64 port, as well as the pthread_win32 license, I submitted a patch to replace pthread_win32 by boost: THRIFT-1361

          Show
          alexandre parenteau added a comment - For the problem of a WIN64 port, as well as the pthread_win32 license, I submitted a patch to replace pthread_win32 by boost: THRIFT-1361
          Hide
          James Dickson added a comment -

          @alexandre: I will add that change back, I forgot to after testing it on XP. As regards the POSIX and 64bit issue I came across this: http://locklessinc.com/articles/pthreads_on_windows/ The implementation seems quite good and wondered what your thoughts were? It has a licence that I believe would be compatible.

          Show
          James Dickson added a comment - @alexandre: I will add that change back, I forgot to after testing it on XP. As regards the POSIX and 64bit issue I came across this: http://locklessinc.com/articles/pthreads_on_windows/ The implementation seems quite good and wondered what your thoughts were? It has a licence that I believe would be compatible.
          Hide
          alexandre parenteau added a comment -

          @james: thanks for the link, it looks very nice! Unfortunately it does not compile with Thrift at the moment, I wrote a comment on the page.

          Show
          alexandre parenteau added a comment - @james: thanks for the link, it looks very nice! Unfortunately it does not compile with Thrift at the moment, I wrote a comment on the page.
          Hide
          Peace C added a comment - - edited

          Terrific work gents! I'm very glad to see James' and Alexandre's patches merged. Adding MSVC support will really speed up Thrift adoption.

          I'm able to build and run a Windows sample project that I've created based on Alexandre's sample in his fork over on github. There is one issue when running against the trunk (SVN 1177082). Using a ThreadManager causes an exception in PosixThreadFactory.cpp (pthread_attr_setschedpolicy @ line 121).

          Here's a code snippit:

          boost::shared_ptr<TProtocolFactory> protocolFactory(new TBinaryProtocolFactory());
          boost::shared_ptr<MyHandler> handler(new MyHandler());
          boost::shared_ptr<TProcessor> processor(new MyProcessor(handler));

          boost::shared_ptr threadManager = ThreadManager::newSimpleThreadManager(NumThreads);
          boost::shared_ptr threadFactory = boost::shared_ptr (new PosixThreadFactory());
          threadManager->threadFactory(threadFactory);
          threadManager->start(); // <-- exception here

          TNonblockingServer server(processor, protocolFactory, Port, threadManager);
          server.serve();

          The exception occurs within the threadManager->start() call. This worked with Alexandre's fork but I haven't been able to determine why it's failing with the newly merged code in the trunk (SVN 1177082). Am I doing something wrong or does the current thrift code not support using ThreadManager this way?

          Show
          Peace C added a comment - - edited Terrific work gents! I'm very glad to see James' and Alexandre's patches merged. Adding MSVC support will really speed up Thrift adoption. I'm able to build and run a Windows sample project that I've created based on Alexandre's sample in his fork over on github. There is one issue when running against the trunk (SVN 1177082). Using a ThreadManager causes an exception in PosixThreadFactory.cpp (pthread_attr_setschedpolicy @ line 121). Here's a code snippit: boost::shared_ptr<TProtocolFactory> protocolFactory(new TBinaryProtocolFactory()); boost::shared_ptr<MyHandler> handler(new MyHandler()); boost::shared_ptr<TProcessor> processor(new MyProcessor(handler)); boost::shared_ptr threadManager = ThreadManager::newSimpleThreadManager(NumThreads); boost::shared_ptr threadFactory = boost::shared_ptr (new PosixThreadFactory()); threadManager->threadFactory(threadFactory); threadManager->start(); // <-- exception here TNonblockingServer server(processor, protocolFactory, Port, threadManager); server.serve(); The exception occurs within the threadManager->start() call. This worked with Alexandre's fork but I haven't been able to determine why it's failing with the newly merged code in the trunk (SVN 1177082). Am I doing something wrong or does the current thrift code not support using ThreadManager this way?
          Hide
          alexandre parenteau added a comment -

          @Peace: I think this is missing inside the trunk:

          > #ifdef _WIN32
          > //WIN32 Pthread implementation doesn't seem to support sheduling policies other then PosixThreadFactory::OTHER - runtime error
          > policy_ = PosixThreadFactory::OTHER;
          #endif
          if (pthread_attr_setschedpolicy(&thread_attr, policy_) != 0) {
          

          This is the only difference I notice between the trunk and my fork, and it seems to be consistent with your problem description.

          Show
          alexandre parenteau added a comment - @Peace: I think this is missing inside the trunk: > #ifdef _WIN32 > //WIN32 Pthread implementation doesn't seem to support sheduling policies other then PosixThreadFactory::OTHER - runtime error > policy_ = PosixThreadFactory::OTHER; #endif if (pthread_attr_setschedpolicy(&thread_attr, policy_) != 0) { This is the only difference I notice between the trunk and my fork, and it seems to be consistent with your problem description.
          Hide
          Peace C added a comment - - edited

          @alexandre: Yes that fixes the threading exception. Thank you!

          Another difference is around line 252, a float cast assigned to stepsperquanta:

          #ifdef HAVE_SCHED_GET_PRIORITY_MAX
              max_priority = sched_get_priority_max(pthread_policy);
          #endif
              int quanta = (HIGHEST - LOWEST) + 1;
              float stepsperquanta = (float)(max_priority - min_priority) / quanta;
          

          Not sure how important that is but I put it in and the sample runs well.

          Show
          Peace C added a comment - - edited @alexandre: Yes that fixes the threading exception. Thank you! Another difference is around line 252, a float cast assigned to stepsperquanta: #ifdef HAVE_SCHED_GET_PRIORITY_MAX max_priority = sched_get_priority_max(pthread_policy); #endif int quanta = (HIGHEST - LOWEST) + 1; float stepsperquanta = ( float )(max_priority - min_priority) / quanta; Not sure how important that is but I put it in and the sample runs well.
          Hide
          Peace C added a comment - - edited

          Uploaded "PosixThreadFactory.patch" with the changes in the above comments.

          I just noticed that the patch doesn't contain the full path name for PosixThreadFactory.cpp (sorry about that). It should be
          lib/cpp/src/concurrency/PosixThreadFactory.cpp

          Show
          Peace C added a comment - - edited Uploaded "PosixThreadFactory.patch" with the changes in the above comments. I just noticed that the patch doesn't contain the full path name for PosixThreadFactory.cpp (sorry about that). It should be lib/cpp/src/concurrency/PosixThreadFactory.cpp
          Hide
          Roger Meier added a comment -

          Thanks Peace! committed.

          Show
          Roger Meier added a comment - Thanks Peace! committed.
          Hide
          Hudson added a comment -

          Integrated in Thrift #287 (See https://builds.apache.org/job/Thrift/287/)
          THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0
          Patch: Peace

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1177817
          Files :

          • /thrift/trunk/lib/cpp/src/concurrency/PosixThreadFactory.cpp
          Show
          Hudson added a comment - Integrated in Thrift #287 (See https://builds.apache.org/job/Thrift/287/ ) THRIFT-1031 Patch to compile Thrift for vc++ 9.0 and 10.0 Patch: Peace roger : http://svn.apache.org/viewvc/?view=rev&rev=1177817 Files : /thrift/trunk/lib/cpp/src/concurrency/PosixThreadFactory.cpp
          Hide
          Jens Geyer added a comment -

          The Thrift compiler project is still VC2010 only?

          Show
          Jens Geyer added a comment - The Thrift compiler project is still VC2010 only?
          Hide
          James Dickson added a comment -

          I have only supplied 2010 projects as this is the only system we use at work. I can't really spend the required time to fully support older versions. However, having said that, I have recently come across this project: http://industriousone.com/premake which is similar to cmake in some ways, but I feel is better and would enable multiple project support if we switched to it.

          Let me know what you all think.

          James

          Show
          James Dickson added a comment - I have only supplied 2010 projects as this is the only system we use at work. I can't really spend the required time to fully support older versions. However, having said that, I have recently come across this project: http://industriousone.com/premake which is similar to cmake in some ways, but I feel is better and would enable multiple project support if we switched to it. Let me know what you all think. James
          Hide
          Roger Meier added a comment -

          I like the premake approach.
          Is it really possible to generate all these project files correctly?
          Latest release is from November 2010(http://industriousone.com/topic/premake-43-released).
          Is it widely used?
          How active is the development and community of premake?

          -roger

          Show
          Roger Meier added a comment - I like the premake approach. Is it really possible to generate all these project files correctly? Latest release is from November 2010( http://industriousone.com/topic/premake-43-released ). Is it widely used? How active is the development and community of premake? -roger
          Hide
          Aurélien Revol added a comment -

          @James: Thanks for the suggestion! If there is no practical way to somehow reuse the existing Makefiles in MS Visual Studio (I suppose nmake is too badly broken for that?), the premake approach does look reasonable. Still I'm curious too about what makes you feel that premake is better than CMake?

          @Roger: here are some stats about both premake and CMake if that can be of any help:
          http://www.ohloh.net/p/premake
          http://www.ohloh.net/p/cmake
          Those make me agree with your concern that premake looks riskier to engage with.

          The issue I see with CMake, contrarily to premake, is that its documentation does not clearly state that MSVC 2010 is actually supported. So I guess the only way to find out would be to try.

          Aurelien

          Show
          Aurélien Revol added a comment - @James: Thanks for the suggestion! If there is no practical way to somehow reuse the existing Makefiles in MS Visual Studio (I suppose nmake is too badly broken for that?), the premake approach does look reasonable. Still I'm curious too about what makes you feel that premake is better than CMake? @Roger: here are some stats about both premake and CMake if that can be of any help: http://www.ohloh.net/p/premake http://www.ohloh.net/p/cmake Those make me agree with your concern that premake looks riskier to engage with. The issue I see with CMake, contrarily to premake, is that its documentation does not clearly state that MSVC 2010 is actually supported. So I guess the only way to find out would be to try. Aurelien
          Hide
          Jake Farrell added a comment -

          What is the introduction of a new make tool for? can whatever is trying to be accomplished not be achieved with autotools currently?

          Show
          Jake Farrell added a comment - What is the introduction of a new make tool for? can whatever is trying to be accomplished not be achieved with autotools currently?
          Hide
          Aurélien Revol added a comment -

          That's my point. Using premake or CMake would still duplicate the effort that is put in the autotools for the Linux version.

          Instead I was wondering if using Microsoft's version of make, a.k.a. nmake (not the nmake from AT&T of course) wouldn't allow compiling with Visual C++ thanks to Makefiles generated by the autotools.

          Or could gnu make from MSys or Cygwin be used for the same purpose of driving Visual C++'s command-line compiler?

          In both cases I don't know if generating Visual Studio project files from Makefiles would be possible.

          Show
          Aurélien Revol added a comment - That's my point. Using premake or CMake would still duplicate the effort that is put in the autotools for the Linux version. Instead I was wondering if using Microsoft's version of make, a.k.a. nmake (not the nmake from AT&T of course) wouldn't allow compiling with Visual C++ thanks to Makefiles generated by the autotools. Or could gnu make from MSys or Cygwin be used for the same purpose of driving Visual C++'s command-line compiler? In both cases I don't know if generating Visual Studio project files from Makefiles would be possible.
          Hide
          Roger Meier added a comment -

          cmake was already discussed here THRIFT-797

          I close this issue now. Please create additional tickets for other improvements on this.

          Show
          Roger Meier added a comment - cmake was already discussed here THRIFT-797 I close this issue now. Please create additional tickets for other improvements on this.

            People

            • Assignee:
              Roger Meier
              Reporter:
              James Dickson
            • Votes:
              5 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development