You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "alexandre parenteau (JIRA)" <ji...@apache.org> on 2011/09/23 02:01:26 UTC
[jira] [Issue Comment Edited] (THRIFT-1031) Patch to compile Thrift
for vc++ 9.0 and 10.0
[ https://issues.apache.org/jira/browse/THRIFT-1031?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13113043#comment-13113043 ]
alexandre parenteau edited comment on THRIFT-1031 at 9/22/11 11:59 PM:
-----------------------------------------------------------------------
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!
was (Author: aubonbeurre):
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? Well since you are using already force-compilation, you could add windows/config.h to the list for forced headers.
Again, thanks a lot for the patch!
> Patch to compile Thrift for vc++ 9.0 and 10.0
> ---------------------------------------------
>
> Key: THRIFT-1031
> URL: https://issues.apache.org/jira/browse/THRIFT-1031
> Project: Thrift
> Issue Type: Improvement
> Components: C++ - Library
> Environment: Windows XP 32bit, vc++ 9.0, 10.0
> Reporter: James Dickson
> Assignee: Roger Meier
> Priority: Trivial
> Fix For: 0.8
>
> Attachments: thrift_msvc.patch, thrift_msvc_v0_1.patch, thrift_msvc_v0_2.patch, thrift_msvc_v0_3.patch, thrift_msvc_v0_4.patch, thrift_msvc_v0_5.patch, thrift_msvc_v0_6.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira