[GitHub] [thrift] emmenlau commented on a change in pull request #2148: THRIFT-5212: Thrift Visual Studio project throw errors when build

[GitBox <gi...@apache.org>]

emmenlau commented on a change in pull request #2148:
URL: https://github.com/apache/thrift/pull/2148#discussion_r429105704



##########
File path: lib/cpp/src/thrift/transport/THttpClient.cpp
##########
@@ -102,7 +102,7 @@ void THttpClient::flush() {
   std::ostringstream h;
   h << "POST " << path_ << " HTTP/1.1" << CRLF << "Host: " << host_ << CRLF
     << "Content-Type: application/x-thrift" << CRLF << "Content-Length: " << len << CRLF
-    << "Accept: application/x-thrift" << CRLF << "User-Agent: Thrift/" << PACKAGE_VERSION
+    << "Accept: application/x-thrift" << CRLF << "User-Agent: Thrift/" /*<< PACKAGE_VERSION*/

Review comment:
       Why is `PACKAGE_VERSION` commented out?

##########
File path: lib/cpp/src/thrift/transport/TSSLSocket.h
##########
@@ -324,7 +324,7 @@ class TSSLSocketFactory {
   std::shared_ptr<AccessManager> access_;
   static concurrency::Mutex mutex_;
   static uint64_t count_;
-  THRIFT_EXPORT static bool manualOpenSSLInitialization_;
+  /*THRIFT_EXPORT*/ static bool manualOpenSSLInitialization_;

Review comment:
       Why is `THRIFT_EXPORT` commented out?

##########
File path: lib/cpp/libthrift.vcxproj
##########
@@ -211,7 +267,8 @@
   </ImportGroup>
   <PropertyGroup Label="UserMacros" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
-    <IncludePath>$(ProjectDir)\src\;$(ProjectDir)\src\thrift\windows\;$(BOOST_ROOT)\include;$(BOOST_ROOT)\;$(OPENSSL_ROOT_DIR)\include\;$(IncludePath)</IncludePath>
+    <IncludePath>$(ProjectDir)\src\;$(ProjectDir)\src\thrift\windows\;C:\Users\Administrator\Desktop\HeHaiPing\boost_1_73_0;C:\ruanjianbao\openssl\include;$(IncludePath)</IncludePath>
+    <LibraryPath>$(BOOST_HOME)\stage\lib;C:\ruanjianbao\openssl\lib;$(LibraryPath)</LibraryPath>

Review comment:
       This path `C:\ruanjianbao\openssl` is probably an artefact? :-)

##########
File path: lib/cpp/src/thrift/transport/THttpServer.cpp
##########
@@ -140,7 +140,7 @@ void THttpServer::flush() {
 std::string THttpServer::getHeader(uint32_t len) {
   std::ostringstream h;
   h << "HTTP/1.1 200 OK" << CRLF << "Date: " << getTimeRFC1123() << CRLF << "Server: Thrift/"
-    << PACKAGE_VERSION << CRLF << "Access-Control-Allow-Origin: *" << CRLF
+    /*<< PACKAGE_VERSION*/ << CRLF << "Access-Control-Allow-Origin: *" << CRLF

Review comment:
       Why is `PACKAGE_VERSION` commented out?

##########
File path: lib/cpp/libthrift.vcxproj
##########
@@ -134,45 +182,53 @@
     <ConfigurationType>StaticLibrary</ConfigurationType>
     <UseDebugLibraries>true</UseDebugLibraries>
     <CharacterSet>MultiByte</CharacterSet>
+    <PlatformToolset>v142</PlatformToolset>

Review comment:
       Is `PlatformToolset` required or can Visual Studio find it automatically? Could you try to remove it (maybe with a text editor), then load the project and see if it builds correctly?
   
   The less constraint the project is, the better, because it can remain compatible with more versions of `PlatformToolset`.

##########
File path: lib/cpp/libthrift.vcxproj
##########
@@ -211,7 +267,8 @@
   </ImportGroup>
   <PropertyGroup Label="UserMacros" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
-    <IncludePath>$(ProjectDir)\src\;$(ProjectDir)\src\thrift\windows\;$(BOOST_ROOT)\include;$(BOOST_ROOT)\;$(OPENSSL_ROOT_DIR)\include\;$(IncludePath)</IncludePath>

Review comment:
       Why is `BOOST_ROOT` and `OPENSSL_ROOT` replaced with `BOOST_HOME`?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org