arrow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From w...@apache.org
Subject arrow git commit: ARROW-699: [C++] Resolve Arrow and Arrow IPC build issues on Windows;
Date Thu, 30 Mar 2017 19:13:44 GMT
Repository: arrow
Updated Branches:
  refs/heads/master 47fad3f42 -> 15b874e47


ARROW-699: [C++] Resolve Arrow and Arrow IPC build issues on Windows;

Resolve Arrow and Arrow IPC build issues on Windows; Running unit tests in Appveyor.

Changes description:

- Current file.cc implementation ( https://github.com/apache/arrow/compare/master...MaxRis:ARROW-699?expand=1#diff-1b2fb57add5bb8f21e28a707f24462b0L161
) assumes that input file name is encoded with utf-16 inside std::string. But unit tests are
passing just utf-8 compatible C-strings.
Util method Utf8ToUtf16 introduced ( https://github.com/apache/arrow/compare/master...MaxRis:ARROW-699?expand=1#diff-1b2fb57add5bb8f21e28a707f24462b0R156
) to convert utf-8 to utf-16 (std::wstring).

- io-file-test has FIleIsClosed method which uses method _close(FILE_HANDLE) to test if file
handle valid or invalid. Result is interpreted as file was closed or not. MSVC C runtime implementation
by default crashes application if input param is invalid. To overwrite this behavior it's
needed to set custom hander (https://github.com/apache/arrow/compare/master...MaxRis:ARROW-699?expand=1#diff-05724c5d85bf64720fa85ef3012e470dR61).
More info here: https://msdn.microsoft.com/en-us/library/ksazx244.aspx

- Message and FileWriter classes keeps their internal implementation as private member of
unique_ptr of FORWARD declared type, for example:

```
class MessageImpl;
std::unique_ptr<MessageImpl> impl_;
```

MSVC compiler requires constructor and destructor of Message class be defined. Currently,
they are defined by default, and because of this, compiler places auto generated code into
the .hpp file, which is not visible for others libs during the linking (We got unresolved
linking issues). The solution is to define destructors explicitly. More there http://stackoverflow.com/a/42158611/2266412
and there http://stackoverflow.com/a/6089065/2266412

Author: Max Risuhin <risuhin.max@gmail.com>

Closes #449 from MaxRis/ARROW-699 and squashes the following commits:

2d5383f [Max Risuhin] ARROW-699: [C++] Resolve Arrow and Arrow IPC build issues on Windows;
Running unit tests in Appveyor.


Project: http://git-wip-us.apache.org/repos/asf/arrow/repo
Commit: http://git-wip-us.apache.org/repos/asf/arrow/commit/15b874e4
Tree: http://git-wip-us.apache.org/repos/asf/arrow/tree/15b874e4
Diff: http://git-wip-us.apache.org/repos/asf/arrow/diff/15b874e4

Branch: refs/heads/master
Commit: 15b874e47e3975c5240290ec7ed105bf8d1b56bc
Parents: 47fad3f
Author: Max Risuhin <risuhin.max@gmail.com>
Authored: Thu Mar 30 15:13:39 2017 -0400
Committer: Wes McKinney <wes.mckinney@twosigma.com>
Committed: Thu Mar 30 15:13:39 2017 -0400

----------------------------------------------------------------------
 appveyor.yml                       | 12 ++++++----
 cpp/CMakeLists.txt                 | 11 ++++++++-
 cpp/cmake_modules/BuildUtils.cmake |  2 ++
 cpp/src/arrow/io/file.cc           | 42 +++++++++++++++++++--------------
 cpp/src/arrow/io/io-file-test.cc   | 23 ++++++++++++++----
 cpp/src/arrow/io/io-hdfs-test.cc   |  5 ++--
 cpp/src/arrow/io/test-common.h     | 10 ++++----
 cpp/src/arrow/ipc/CMakeLists.txt   | 33 +++++++++++++++++++-------
 cpp/src/arrow/ipc/metadata.cc      |  2 ++
 cpp/src/arrow/ipc/metadata.h       |  1 +
 cpp/src/arrow/ipc/writer.cc        |  4 ++++
 cpp/src/arrow/ipc/writer.h         |  4 +++-
 12 files changed, 106 insertions(+), 43 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/appveyor.yml
----------------------------------------------------------------------
diff --git a/appveyor.yml b/appveyor.yml
index 17362c9..9f35949 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -23,8 +23,8 @@ environment:
     - GENERATOR: Visual Studio 14 2015 Win64
     # - GENERATOR: Visual Studio 14 2015
   MSVC_DEFAULT_OPTIONS: ON
-  BOOST_ROOT: C:\Libraries\boost_1_59_0
-  BOOST_LIBRARYDIR: C:\Libraries\boost_1_59_0\lib64-msvc-14.0
+  BOOST_ROOT: C:\Libraries\boost_1_63_0
+  BOOST_LIBRARYDIR: C:\Libraries\boost_1_63_0\lib64-msvc-14.0
 
 build_script:
  - cd cpp
@@ -32,8 +32,10 @@ build_script:
  - cd build
  # A lot of features are still deactivated as they do not build on Windows
  #  * gbenchmark doesn't build with MSVC
- - cmake -G "%GENERATOR%" -DARROW_BOOST_USE_SHARED=OFF -DARROW_IPC=OFF -DARROW_HDFS=OFF -DARROW_BUILD_BENCHMARKS=OFF
-DARROW_JEMALLOC=OFF ..
- - cmake --build . --config Debug
+ - cmake -G "%GENERATOR%" -DARROW_BOOST_USE_SHARED=OFF -DARROW_BUILD_BENCHMARKS=OFF -DARROW_JEMALLOC=OFF
-DCMAKE_BUILD_TYPE=Release ..
+ - cmake --build . --config Release
 
 # test_script:
-#  - ctest -VV
+ - ctest -VV
+
+

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index e11de1b..aa8ea31 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -500,7 +500,11 @@ if(ARROW_BUILD_TESTS)
     set(GFLAGS_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/gflags_ep-prefix/src/gflags_ep")
     set(GFLAGS_HOME "${GFLAGS_PREFIX}")
     set(GFLAGS_INCLUDE_DIR "${GFLAGS_PREFIX}/include")
-    set(GFLAGS_STATIC_LIB "${GFLAGS_PREFIX}/lib/libgflags.a")
+    if(MSVC)
+      set(GFLAGS_STATIC_LIB "${GFLAGS_PREFIX}/lib/gflags_static.lib")
+    else()
+      set(GFLAGS_STATIC_LIB "${GFLAGS_PREFIX}/lib/libgflags.a")
+    endif()
     set(GFLAGS_VENDORED 1)
     set(GFLAGS_CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
                           -DCMAKE_INSTALL_PREFIX=${GFLAGS_PREFIX}
@@ -536,6 +540,11 @@ if(ARROW_BUILD_TESTS)
   include_directories(SYSTEM ${GFLAGS_INCLUDE_DIR})
   ADD_THIRDPARTY_LIB(gflags
     STATIC_LIB ${GFLAGS_STATIC_LIB})
+  if(MSVC)
+    set_target_properties(gflags
+      PROPERTIES
+      IMPORTED_LINK_INTERFACE_LIBRARIES "shlwapi.lib")
+  endif()
 
   if(GFLAGS_VENDORED)
     add_dependencies(gflags gflags_ep)

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/cmake_modules/BuildUtils.cmake
----------------------------------------------------------------------
diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake
index c993041..43d9840 100644
--- a/cpp/cmake_modules/BuildUtils.cmake
+++ b/cpp/cmake_modules/BuildUtils.cmake
@@ -125,6 +125,8 @@ function(ADD_ARROW_LIB LIB_NAME)
     set_target_properties(${LIB_NAME}_shared
       PROPERTIES
       LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}"
+      RUNTIME_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}"
+      PDB_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}"
       LINK_FLAGS "${ARG_SHARED_LINK_FLAGS}"
       OUTPUT_NAME ${LIB_NAME}
       VERSION "${ARROW_ABI_VERSION}"

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/io/file.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/file.cc b/cpp/src/arrow/io/file.cc
index 7c14238..0aa2c92 100644
--- a/cpp/src/arrow/io/file.cc
+++ b/cpp/src/arrow/io/file.cc
@@ -152,20 +152,30 @@ static inline int64_t lseek64_compat(int fd, int64_t pos, int whence)
{
 #endif
 }
 
+#if defined(_MSC_VER)
+static inline Status ConvertToUtf16(const std::string& input, std::wstring* result) {
+  if (result == nullptr) { return Status::Invalid("Pointer to result is not valid"); }
+
+  if (input.empty()) {
+    *result = std::wstring();
+    return Status::OK();
+  }
+
+  std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> utf16_converter;
+  *result = utf16_converter.from_bytes(input);
+  return Status::OK();
+}
+#endif
+
 static inline Status FileOpenReadable(const std::string& filename, int* fd) {
   int ret;
   errno_t errno_actual = 0;
 #if defined(_MSC_VER)
-  // https://msdn.microsoft.com/en-us/library/w64k0ytk.aspx
-
-  // See GH #209. Here we are assuming that the filename has been encoded in
-  // utf-16le so that unicode filenames can be supported
-  const int nwchars = static_cast<int>(filename.size()) / sizeof(wchar_t);
-  std::vector<wchar_t> wpath(nwchars + 1);
-  memcpy(wpath.data(), filename.data(), filename.size());
-  memcpy(wpath.data() + nwchars, L"\0", sizeof(wchar_t));
+  std::wstring wide_filename;
+  RETURN_NOT_OK(ConvertToUtf16(filename, &wide_filename));
 
-  errno_actual = _wsopen_s(fd, wpath.data(), _O_RDONLY | _O_BINARY, _SH_DENYNO, _S_IREAD);
+  errno_actual =
+      _wsopen_s(fd, wide_filename.c_str(), _O_RDONLY | _O_BINARY, _SH_DENYNO, _S_IREAD);
   ret = *fd;
 #else
   ret = *fd = open(filename.c_str(), O_RDONLY | O_BINARY);
@@ -181,16 +191,12 @@ static inline Status FileOpenWriteable(
   errno_t errno_actual = 0;
 
 #if defined(_MSC_VER)
-  // https://msdn.microsoft.com/en-us/library/w64k0ytk.aspx
-  // Same story with wchar_t as above
-  const int nwchars = static_cast<int>(filename.size()) / sizeof(wchar_t);
-  std::vector<wchar_t> wpath(nwchars + 1);
-  memcpy(wpath.data(), filename.data(), filename.size());
-  memcpy(wpath.data() + nwchars, L"\0", sizeof(wchar_t));
+  std::wstring wide_filename;
+  RETURN_NOT_OK(ConvertToUtf16(filename, &wide_filename));
 
   int oflag = _O_CREAT | _O_BINARY;
-  int sh_flag = _S_IWRITE;
-  if (!write_only) { sh_flag |= _S_IREAD; }
+  int pmode = _S_IWRITE;
+  if (!write_only) { pmode |= _S_IREAD; }
 
   if (truncate) { oflag |= _O_TRUNC; }
 
@@ -200,7 +206,7 @@ static inline Status FileOpenWriteable(
     oflag |= _O_RDWR;
   }
 
-  errno_actual = _wsopen_s(fd, wpath.data(), oflag, _SH_DENYNO, sh_flag);
+  errno_actual = _wsopen_s(fd, wide_filename.c_str(), oflag, _SH_DENYNO, pmode);
   ret = *fd;
 
 #else

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/io/io-file-test.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/io-file-test.cc b/cpp/src/arrow/io/io-file-test.cc
index 5810c82..348be17 100644
--- a/cpp/src/arrow/io/io-file-test.cc
+++ b/cpp/src/arrow/io/io-file-test.cc
@@ -41,14 +41,29 @@ static bool FileExists(const std::string& path) {
   return std::ifstream(path.c_str()).good();
 }
 
+#if defined(_MSC_VER)
+void InvalidParamHandler(const wchar_t* expr, const wchar_t* func,
+    const wchar_t* source_file, unsigned int source_line, uintptr_t reserved) {
+  wprintf(L"Invalid parameter in funcion %s. Source: %s line %d expression %s", func,
+      source_file, source_line, expr);
+}
+#endif
+
 static bool FileIsClosed(int fd) {
-#ifdef _MSC_VER
-  // Close file a second time, this should set errno to EBADF
-  close(fd);
+#if defined(_MSC_VER)
+  // Disables default behavior on wrong params which causes the application to crash
+  // https://msdn.microsoft.com/en-us/library/ksazx244.aspx
+  _set_invalid_parameter_handler(InvalidParamHandler);
+
+  // Disables possible assertion alert box on invalid input arguments
+  _CrtSetReportMode(_CRT_ASSERT, 0);
+
+  int ret = static_cast<int>(_close(fd));
+  return (ret == -1);
 #else
   if (-1 != fcntl(fd, F_GETFD)) { return false; }
-#endif
   return errno == EBADF;
+#endif
 }
 
 class FileTestFixture : public ::testing::Test {

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/io/io-hdfs-test.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/io-hdfs-test.cc b/cpp/src/arrow/io/io-hdfs-test.cc
index 648d4ba..f3140be 100644
--- a/cpp/src/arrow/io/io-hdfs-test.cc
+++ b/cpp/src/arrow/io/io-hdfs-test.cc
@@ -78,8 +78,9 @@ class TestHdfsClient : public ::testing::Test {
     LibHdfsShim* driver_shim;
 
     client_ = nullptr;
-    scratch_dir_ =
-        boost::filesystem::unique_path("/tmp/arrow-hdfs/scratch-%%%%").string();
+    scratch_dir_ = boost::filesystem::unique_path(
+        boost::filesystem::temp_directory_path() / "arrow-hdfs/scratch-%%%%")
+                       .string();
 
     loaded_driver_ = false;
 

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/io/test-common.h
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/io/test-common.h b/cpp/src/arrow/io/test-common.h
index 4c11476..db5bcc1 100644
--- a/cpp/src/arrow/io/test-common.h
+++ b/cpp/src/arrow/io/test-common.h
@@ -69,13 +69,15 @@ class MemoryMapFixture {
 
   void CreateFile(const std::string path, int64_t size) {
     FILE* file = fopen(path.c_str(), "w");
-    if (file != nullptr) { tmp_files_.push_back(path); }
+    if (file != nullptr) {
+      tmp_files_.push_back(path);
 #ifdef _MSC_VER
-    _chsize(fileno(file), static_cast<size_t>(size));
+      _chsize(fileno(file), static_cast<size_t>(size));
 #else
-    ftruncate(fileno(file), static_cast<size_t>(size));
+      ftruncate(fileno(file), static_cast<size_t>(size));
 #endif
-    fclose(file);
+      fclose(file);
+    }
   }
 
   Status InitMemoryMap(

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/ipc/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/ipc/CMakeLists.txt b/cpp/src/arrow/ipc/CMakeLists.txt
index 030cba9..31a04df 100644
--- a/cpp/src/arrow/ipc/CMakeLists.txt
+++ b/cpp/src/arrow/ipc/CMakeLists.txt
@@ -85,6 +85,15 @@ if (ARROW_BUILD_TESTS)
       dl)
     set_target_properties(json-integration-test
       PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
+  elseif (MSVC)
+    target_link_libraries(json-integration-test
+      arrow_ipc_static
+      arrow_io_static
+      arrow_static
+      gflags
+      gtest
+      ${BOOST_FILESYSTEM_LIBRARY}
+      ${BOOST_SYSTEM_LIBRARY})
   else()
     target_link_libraries(json-integration-test
       arrow_ipc_static
@@ -156,14 +165,22 @@ install(
   FILES "${CMAKE_CURRENT_BINARY_DIR}/arrow-ipc.pc"
   DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig/")
 
-
-set(UTIL_LINK_LIBS
-  arrow_ipc_static
-  arrow_io_static
-  arrow_static
-  ${BOOST_FILESYSTEM_LIBRARY}
-  ${BOOST_SYSTEM_LIBRARY}
-  dl)
+if(MSVC)
+  set(UTIL_LINK_LIBS
+    arrow_ipc_static
+    arrow_io_static
+    arrow_static
+    ${BOOST_FILESYSTEM_LIBRARY}
+    ${BOOST_SYSTEM_LIBRARY})
+else()
+  set(UTIL_LINK_LIBS
+    arrow_ipc_static
+    arrow_io_static
+    arrow_static
+    ${BOOST_FILESYSTEM_LIBRARY}
+    ${BOOST_SYSTEM_LIBRARY}
+    dl)
+endif()
 
 if (ARROW_BUILD_UTILITIES)
   add_executable(file-to-stream file-to-stream.cc)

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/ipc/metadata.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/ipc/metadata.cc b/cpp/src/arrow/ipc/metadata.cc
index 36ba4b2..6d9fabd 100644
--- a/cpp/src/arrow/ipc/metadata.cc
+++ b/cpp/src/arrow/ipc/metadata.cc
@@ -767,6 +767,8 @@ Message::Message(const std::shared_ptr<Buffer>& buffer, int64_t
offset) {
   impl_.reset(new MessageImpl(buffer, offset));
 }
 
+Message::~Message() {}
+
 Status Message::Open(const std::shared_ptr<Buffer>& buffer, int64_t offset,
     std::shared_ptr<Message>* out) {
   // ctor is private

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/ipc/metadata.h
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/ipc/metadata.h b/cpp/src/arrow/ipc/metadata.h
index f60fb77..798abdc 100644
--- a/cpp/src/arrow/ipc/metadata.h
+++ b/cpp/src/arrow/ipc/metadata.h
@@ -140,6 +140,7 @@ struct ARROW_EXPORT BufferMetadata {
 
 class ARROW_EXPORT Message {
  public:
+  ~Message();
   enum Type { NONE, SCHEMA, DICTIONARY_BATCH, RECORD_BATCH };
 
   static Status Open(const std::shared_ptr<Buffer>& buffer, int64_t offset,

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/ipc/writer.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/ipc/writer.cc b/cpp/src/arrow/ipc/writer.cc
index db5f082..0a19f69 100644
--- a/cpp/src/arrow/ipc/writer.cc
+++ b/cpp/src/arrow/ipc/writer.cc
@@ -662,6 +662,8 @@ Status StreamWriter::WriteRecordBatch(const RecordBatch& batch, bool
allow_64bit
   return impl_->WriteRecordBatch(batch, allow_64bit);
 }
 
+StreamWriter::~StreamWriter() {}
+
 void StreamWriter::set_memory_pool(MemoryPool* pool) {
   impl_->set_memory_pool(pool);
 }
@@ -718,6 +720,8 @@ FileWriter::FileWriter() {
   impl_.reset(new FileWriterImpl());
 }
 
+FileWriter::~FileWriter() {}
+
 Status FileWriter::Open(io::OutputStream* sink, const std::shared_ptr<Schema>&
schema,
     std::shared_ptr<FileWriter>* out) {
   *out = std::shared_ptr<FileWriter>(new FileWriter());  // ctor is private

http://git-wip-us.apache.org/repos/asf/arrow/blob/15b874e4/cpp/src/arrow/ipc/writer.h
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/ipc/writer.h b/cpp/src/arrow/ipc/writer.h
index 25b5ad6..c572157 100644
--- a/cpp/src/arrow/ipc/writer.h
+++ b/cpp/src/arrow/ipc/writer.h
@@ -82,7 +82,7 @@ Status GetRecordBatchSize(const RecordBatch& batch, int64_t* size);
 
 class ARROW_EXPORT StreamWriter {
  public:
-  virtual ~StreamWriter() = default;
+  virtual ~StreamWriter();
 
   static Status Open(io::OutputStream* sink, const std::shared_ptr<Schema>& schema,
       std::shared_ptr<StreamWriter>* out);
@@ -105,6 +105,8 @@ class ARROW_EXPORT StreamWriter {
 
 class ARROW_EXPORT FileWriter : public StreamWriter {
  public:
+  virtual ~FileWriter();
+
   static Status Open(io::OutputStream* sink, const std::shared_ptr<Schema>& schema,
       std::shared_ptr<FileWriter>* out);
 


Mime
View raw message