From 2d5383f2a0e5a5ec737855400d5237a065fe0043 Mon Sep 17 00:00:00 2001 From: Max Risuhin Date: Thu, 30 Mar 2017 21:23:24 +0300 Subject: [PATCH] ARROW-699: [C++] Resolve Arrow and Arrow IPC build issues on Windows; Running unit tests in Appveyor. --- 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(-) diff --git a/appveyor.yml b/appveyor.yml index 17362c993d0..9f3594907d1 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 + + diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index e4c18ca86e4..c3ee6f9b1f8 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) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index c9930418185..43d984045eb 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}" diff --git a/cpp/src/arrow/io/file.cc b/cpp/src/arrow/io/file.cc index 7c14238e8fd..0aa2c92a072 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> 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(filename.size()) / sizeof(wchar_t); - std::vector 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(filename.size()) / sizeof(wchar_t); - std::vector 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 diff --git a/cpp/src/arrow/io/io-file-test.cc b/cpp/src/arrow/io/io-file-test.cc index 5810c820f6d..348be17d893 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(_close(fd)); + return (ret == -1); #else if (-1 != fcntl(fd, F_GETFD)) { return false; } -#endif return errno == EBADF; +#endif } class FileTestFixture : public ::testing::Test { diff --git a/cpp/src/arrow/io/io-hdfs-test.cc b/cpp/src/arrow/io/io-hdfs-test.cc index 648d4baac9b..f3140be0b2d 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; diff --git a/cpp/src/arrow/io/test-common.h b/cpp/src/arrow/io/test-common.h index 4c114760e9a..db5bcc1b4f4 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)); + _chsize(fileno(file), static_cast(size)); #else - ftruncate(fileno(file), static_cast(size)); + ftruncate(fileno(file), static_cast(size)); #endif - fclose(file); + fclose(file); + } } Status InitMemoryMap( diff --git a/cpp/src/arrow/ipc/CMakeLists.txt b/cpp/src/arrow/ipc/CMakeLists.txt index 030cba93f5f..31a04dfc078 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) diff --git a/cpp/src/arrow/ipc/metadata.cc b/cpp/src/arrow/ipc/metadata.cc index 85dc8b321c4..5b38cf38e0e 100644 --- a/cpp/src/arrow/ipc/metadata.cc +++ b/cpp/src/arrow/ipc/metadata.cc @@ -768,6 +768,8 @@ Message::Message(const std::shared_ptr& buffer, int64_t offset) { impl_.reset(new MessageImpl(buffer, offset)); } +Message::~Message() {} + Status Message::Open(const std::shared_ptr& buffer, int64_t offset, std::shared_ptr* out) { // ctor is private diff --git a/cpp/src/arrow/ipc/metadata.h b/cpp/src/arrow/ipc/metadata.h index f60fb770c36..798abdcdf9d 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, int64_t offset, diff --git a/cpp/src/arrow/ipc/writer.cc b/cpp/src/arrow/ipc/writer.cc index 92e61941937..d1a851ac950 100644 --- a/cpp/src/arrow/ipc/writer.cc +++ b/cpp/src/arrow/ipc/writer.cc @@ -663,6 +663,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); } @@ -719,6 +721,8 @@ FileWriter::FileWriter() { impl_.reset(new FileWriterImpl()); } +FileWriter::~FileWriter() {} + Status FileWriter::Open(io::OutputStream* sink, const std::shared_ptr& schema, std::shared_ptr* out) { *out = std::shared_ptr(new FileWriter()); // ctor is private diff --git a/cpp/src/arrow/ipc/writer.h b/cpp/src/arrow/ipc/writer.h index 25b5ad62726..c572157b465 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, std::shared_ptr* 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, std::shared_ptr* out);