From a188252edae6f6808c1a6f3ef89c5e531064bc90 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Thu, 26 Nov 2020 15:24:34 -0500 Subject: [PATCH 01/25] ARROW-10746: [C++] Bump gtest version and use GTEST_SKIP tests --- ci/conda_env_cpp.yml | 4 ++-- cpp/src/arrow/io/hdfs_test.cc | 7 +++--- cpp/src/arrow/util/compression_test.cc | 33 +++++++++----------------- cpp/src/parquet/encoding_test.cc | 15 +++++------- cpp/thirdparty/versions.txt | 2 +- 5 files changed, 23 insertions(+), 38 deletions(-) diff --git a/ci/conda_env_cpp.yml b/ci/conda_env_cpp.yml index 870d85166a4..390eb7dcdd5 100644 --- a/ci/conda_env_cpp.yml +++ b/ci/conda_env_cpp.yml @@ -24,9 +24,9 @@ c-ares cmake gflags glog -gmock>=1.8.1 +gmock>=1.10.0 grpc-cpp>=1.27.3 -gtest=1.8.1 +gtest=1.10.0 libprotobuf libutf8proc lz4-c diff --git a/cpp/src/arrow/io/hdfs_test.cc b/cpp/src/arrow/io/hdfs_test.cc index ac7100745fd..2ebf950808f 100644 --- a/cpp/src/arrow/io/hdfs_test.cc +++ b/cpp/src/arrow/io/hdfs_test.cc @@ -136,10 +136,9 @@ class TestHadoopFileSystem : public ::testing::Test { std::shared_ptr client_; }; -#define SKIP_IF_NO_DRIVER() \ - if (!this->loaded_driver_) { \ - std::cout << "Driver not loaded, skipping" << std::endl; \ - return; \ +#define SKIP_IF_NO_DRIVER() \ + if (!this->loaded_driver_) { \ + GTEST_SKIP() << "Driver not loaded, skipping"; \ } TEST_F(TestHadoopFileSystem, ConnectsAgain) { diff --git a/cpp/src/arrow/util/compression_test.cc b/cpp/src/arrow/util/compression_test.cc index 8b184f2dc41..ef34701d4e5 100644 --- a/cpp/src/arrow/util/compression_test.cc +++ b/cpp/src/arrow/util/compression_test.cc @@ -349,8 +349,7 @@ TEST(TestCodecMisc, GetCompressionType) { TEST_P(CodecTest, CodecRoundtrip) { const auto compression = GetCompression(); if (compression == Compression::BZ2) { - // SKIP: BZ2 doesn't support one-shot compression - return; + GTEST_SKIP() << "BZ2 does not support one-shot compression"; } int sizes[] = {0, 10000, 100000}; @@ -429,17 +428,14 @@ TEST_P(CodecTest, OutputBufferIsSmall) { TEST_P(CodecTest, StreamingCompressor) { if (GetCompression() == Compression::SNAPPY) { - // SKIP: snappy doesn't support streaming compression - return; + GTEST_SKIP() << "snappy doesn't support streaming compression"; } if (GetCompression() == Compression::BZ2) { - // SKIP: BZ2 doesn't support one-shot decompression - return; + GTEST_SKIP() << "Z2 doesn't support one-shot decompression"; } if (GetCompression() == Compression::LZ4 || GetCompression() == Compression::LZ4_HADOOP) { - // SKIP: LZ4 raw format doesn't support streaming compression. - return; + GTEST_SKIP() << "LZ4 raw format doesn't support streaming compression."; } int sizes[] = {0, 10, 100000}; @@ -456,17 +452,14 @@ TEST_P(CodecTest, StreamingCompressor) { TEST_P(CodecTest, StreamingDecompressor) { if (GetCompression() == Compression::SNAPPY) { - // SKIP: snappy doesn't support streaming decompression - return; + GTEST_SKIP() << "snappy doesn't support streaming decompression."; } if (GetCompression() == Compression::BZ2) { - // SKIP: BZ2 doesn't support one-shot compression - return; + GTEST_SKIP() << "Z2 doesn't support one-shot compression"; } if (GetCompression() == Compression::LZ4 || GetCompression() == Compression::LZ4_HADOOP) { - // SKIP: LZ4 raw format doesn't support streaming decompression. - return; + GTEST_SKIP() << "LZ4 raw format doesn't support streaming decompression."; } int sizes[] = {0, 10, 100000}; @@ -483,13 +476,11 @@ TEST_P(CodecTest, StreamingDecompressor) { TEST_P(CodecTest, StreamingRoundtrip) { if (GetCompression() == Compression::SNAPPY) { - // SKIP: snappy doesn't support streaming decompression - return; + GTEST_SKIP() << "snappy doesn't support streaming decompression"; } if (GetCompression() == Compression::LZ4 || GetCompression() == Compression::LZ4_HADOOP) { - // SKIP: LZ4 raw format doesn't support streaming compression. - return; + GTEST_SKIP() << "LZ4 raw format doesn't support streaming compression."; } int sizes[] = {0, 10, 100000}; @@ -506,13 +497,11 @@ TEST_P(CodecTest, StreamingRoundtrip) { TEST_P(CodecTest, StreamingDecompressorReuse) { if (GetCompression() == Compression::SNAPPY) { - // SKIP: snappy doesn't support streaming decompression - return; + GTEST_SKIP() << "snappy doesn't support streaming decompression"; } if (GetCompression() == Compression::LZ4 || GetCompression() == Compression::LZ4_HADOOP) { - // SKIP: LZ4 raw format doesn't support streaming decompression. - return; + GTEST_SKIP() << "LZ4 raw format doesn't support streaming decompression."; } auto codec = MakeCodec(); diff --git a/cpp/src/parquet/encoding_test.cc b/cpp/src/parquet/encoding_test.cc index 7e440fa9a03..6dfc956e037 100644 --- a/cpp/src/parquet/encoding_test.cc +++ b/cpp/src/parquet/encoding_test.cc @@ -43,13 +43,6 @@ using arrow::default_memory_pool; using arrow::MemoryPool; using arrow::internal::checked_cast; -// TODO(hatemhelal): investigate whether this can be replaced with GTEST_SKIP in a future -// gtest release that contains https://github.com/google/googletest/pull/1544 -#define SKIP_TEST_IF(condition) \ - if (condition) { \ - return; \ - } - namespace parquet { namespace test { @@ -495,7 +488,9 @@ class TestArrowBuilderDecoding : public ::testing::Test { void CheckDecodeArrowNonNullUsingDenseBuilder() { for (auto np : null_probabilities_) { InitTestCase(np); - SKIP_TEST_IF(null_count_ > 0) + if (null_count_ > 0) { + GTEST_SKIP(); + } typename EncodingTraits::Accumulator acc; acc.builder.reset(new ::arrow::BinaryBuilder); auto actual_num_values = decoder_->DecodeArrowNonNull(num_values_, &acc); @@ -508,7 +503,9 @@ class TestArrowBuilderDecoding : public ::testing::Test { void CheckDecodeArrowNonNullUsingDictBuilder() { for (auto np : null_probabilities_) { InitTestCase(np); - SKIP_TEST_IF(null_count_ > 0) + if (null_count_ > 0) { + GTEST_SKIP(); + } auto builder = CreateDictBuilder(); auto actual_num_values = decoder_->DecodeArrowNonNull(num_values_, builder.get()); CheckDict(actual_num_values, *builder); diff --git a/cpp/thirdparty/versions.txt b/cpp/thirdparty/versions.txt index 969de58ea79..3a682e5d420 100644 --- a/cpp/thirdparty/versions.txt +++ b/cpp/thirdparty/versions.txt @@ -36,7 +36,7 @@ ARROW_GBENCHMARK_BUILD_VERSION=v1.5.2 ARROW_GFLAGS_BUILD_VERSION=v2.2.2 ARROW_GLOG_BUILD_VERSION=v0.4.0 ARROW_GRPC_BUILD_VERSION=v1.29.1 -ARROW_GTEST_BUILD_VERSION=1.8.1 +ARROW_GTEST_BUILD_VERSION=1.10.0 ARROW_JEMALLOC_BUILD_VERSION=5.2.1 ARROW_LZ4_BUILD_VERSION=v1.9.2 ARROW_MIMALLOC_BUILD_VERSION=v1.6.4 From 3bdb750114972fcafe9547f37526bd2e62f7d352 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Mon, 30 Nov 2020 21:53:35 -0500 Subject: [PATCH 02/25] @kou patch --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index df03c3129a5..bdf15e4bc04 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1603,21 +1603,17 @@ macro(build_gtest) set(GTEST_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/googletest_ep-prefix") set(GTEST_INCLUDE_DIR "${GTEST_PREFIX}/include") - set(_GTEST_RUNTIME_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY}) +set(_GTEST_LIBRARY_DIR "${GTEST_PREFIX}/lib") if(MSVC) set(_GTEST_IMPORTED_TYPE IMPORTED_IMPLIB) set(_GTEST_LIBRARY_SUFFIX "${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_IMPORT_LIBRARY_SUFFIX}") - # Use the import libraries from the EP - set(_GTEST_LIBRARY_DIR "${GTEST_PREFIX}/lib") else() set(_GTEST_IMPORTED_TYPE IMPORTED_LOCATION) set(_GTEST_LIBRARY_SUFFIX "${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_SHARED_LIBRARY_SUFFIX}") - # Library and runtime same on non-Windows - set(_GTEST_LIBRARY_DIR "${_GTEST_RUNTIME_DIR}") endif() set(GTEST_SHARED_LIB @@ -1632,6 +1628,7 @@ macro(build_gtest) ${EP_COMMON_TOOLCHAIN} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} "-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}" + -DCMAKE_INSTALL_LIBDIR=lib -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS} -DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS}) @@ -1649,19 +1646,6 @@ macro(build_gtest) "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE}=${_GTEST_RUNTIME_DIR}") endif() - if(MSVC) - if(NOT ("${CMAKE_GENERATOR}" STREQUAL "Ninja")) - set(_GTEST_RUNTIME_DIR ${_GTEST_RUNTIME_DIR}/${CMAKE_BUILD_TYPE}) - endif() - set(GTEST_CMAKE_ARGS - ${GTEST_CMAKE_ARGS} "-DCMAKE_RUNTIME_OUTPUT_DIRECTORY=${_GTEST_RUNTIME_DIR}" - "-DCMAKE_RUNTIME_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE}=${_GTEST_RUNTIME_DIR}") - else() - list( - APPEND GTEST_CMAKE_ARGS "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY=${_GTEST_RUNTIME_DIR}" - "-DCMAKE_RUNTIME_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE}=${_GTEST_RUNTIME_DIR}") - endif() - add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1) if(MSVC AND NOT ARROW_USE_STATIC_CRT) From 846987c1ceb21f35047e57d3578516258d6febe5 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Tue, 1 Dec 2020 00:21:18 -0500 Subject: [PATCH 03/25] @kou feedback: googletest version check --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index bdf15e4bc04..60bdf13ad69 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1682,7 +1682,7 @@ set(_GTEST_LIBRARY_DIR "${GTEST_PREFIX}/lib") endmacro() if(ARROW_TESTING) - resolve_dependency(GTest) + resolve_dependency(GTest REQUIRED_VERSION 1.10.0) if(NOT GTEST_VENDORED) # TODO(wesm): This logic does not work correctly with the MSVC static libraries From 6967027b5e239cc042cbed7d9676411820abb267 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Tue, 1 Dec 2020 00:56:19 -0500 Subject: [PATCH 04/25] @kou template patch --- cpp/src/parquet/statistics_test.cc | 33 ++++++++++++++++-------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/cpp/src/parquet/statistics_test.cc b/cpp/src/parquet/statistics_test.cc index 0828f36b3a8..eef23b0263b 100644 --- a/cpp/src/parquet/statistics_test.cc +++ b/cpp/src/parquet/statistics_test.cc @@ -609,8 +609,7 @@ static const int NUM_VALUES = 10; template class TestStatisticsSortOrder : public ::testing::Test { public: - typedef typename TestType::c_type T; - + using c_type = typename TestType::c_type; void AddNodes(std::string name) { fields_.push_back(schema::PrimitiveNode::Make( name, Repetition::REQUIRED, TestType::type_num, ConvertedType::NONE)); @@ -670,7 +669,7 @@ class TestStatisticsSortOrder : public ::testing::Test { } protected: - std::vector values_; + std::vector values_; std::vector values_buf_; std::vector fields_; std::shared_ptr schema_; @@ -700,13 +699,17 @@ void TestStatisticsSortOrder::SetValues() { // Write UINT32 min/max values stats_[0] - .set_min(std::string(reinterpret_cast(&values_[5]), sizeof(T))) - .set_max(std::string(reinterpret_cast(&values_[4]), sizeof(T))); + .set_min( + std::string(reinterpret_cast(&values_[5]), sizeof(c_type))) + .set_max( + std::string(reinterpret_cast(&values_[4]), sizeof(c_type))); // Write INT32 min/max values stats_[1] - .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(T))) - .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(T))); + .set_min( + std::string(reinterpret_cast(&values_[0]), sizeof(c_type))) + .set_max( + std::string(reinterpret_cast(&values_[9]), sizeof(c_type))); } // TYPE::INT64 @@ -728,13 +731,13 @@ void TestStatisticsSortOrder::SetValues() { // Write UINT64 min/max values stats_[0] - .set_min(std::string(reinterpret_cast(&values_[5]), sizeof(T))) - .set_max(std::string(reinterpret_cast(&values_[4]), sizeof(T))); + .set_min(std::string(reinterpret_cast(&values_[5]), sizeof(c_type))) + .set_max(std::string(reinterpret_cast(&values_[4]), sizeof(c_type))); // Write INT64 min/max values stats_[1] - .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(T))) - .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(T))); + .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(c_type))) + .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(c_type))); } // TYPE::FLOAT @@ -747,8 +750,8 @@ void TestStatisticsSortOrder::SetValues() { // Write Float min/max values stats_[0] - .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(T))) - .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(T))); + .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(c_type))) + .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(c_type))); } // TYPE::DOUBLE @@ -761,8 +764,8 @@ void TestStatisticsSortOrder::SetValues() { // Write Double min/max values stats_[0] - .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(T))) - .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(T))); + .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(c_type))) + .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(c_type))); } // TYPE::ByteArray From b127ed610763346c17c139c7a44f69a49cc258a2 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Tue, 1 Dec 2020 00:57:30 -0500 Subject: [PATCH 05/25] linting fix --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 2 +- cpp/src/parquet/statistics_test.cc | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 60bdf13ad69..9cb6d9e310c 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1603,7 +1603,7 @@ macro(build_gtest) set(GTEST_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/googletest_ep-prefix") set(GTEST_INCLUDE_DIR "${GTEST_PREFIX}/include") -set(_GTEST_LIBRARY_DIR "${GTEST_PREFIX}/lib") + set(_GTEST_LIBRARY_DIR "${GTEST_PREFIX}/lib") if(MSVC) set(_GTEST_IMPORTED_TYPE IMPORTED_IMPLIB) diff --git a/cpp/src/parquet/statistics_test.cc b/cpp/src/parquet/statistics_test.cc index eef23b0263b..c9f2e347750 100644 --- a/cpp/src/parquet/statistics_test.cc +++ b/cpp/src/parquet/statistics_test.cc @@ -699,17 +699,13 @@ void TestStatisticsSortOrder::SetValues() { // Write UINT32 min/max values stats_[0] - .set_min( - std::string(reinterpret_cast(&values_[5]), sizeof(c_type))) - .set_max( - std::string(reinterpret_cast(&values_[4]), sizeof(c_type))); + .set_min(std::string(reinterpret_cast(&values_[5]), sizeof(c_type))) + .set_max(std::string(reinterpret_cast(&values_[4]), sizeof(c_type))); // Write INT32 min/max values stats_[1] - .set_min( - std::string(reinterpret_cast(&values_[0]), sizeof(c_type))) - .set_max( - std::string(reinterpret_cast(&values_[9]), sizeof(c_type))); + .set_min(std::string(reinterpret_cast(&values_[0]), sizeof(c_type))) + .set_max(std::string(reinterpret_cast(&values_[9]), sizeof(c_type))); } // TYPE::INT64 From 826e032e0ceabe37d2712b692f9268684e1fa752 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Tue, 1 Dec 2020 15:14:03 -0500 Subject: [PATCH 06/25] patch column_writer_test --- cpp/src/parquet/column_writer_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/src/parquet/column_writer_test.cc b/cpp/src/parquet/column_writer_test.cc index 6694a59f443..a914669a9ff 100644 --- a/cpp/src/parquet/column_writer_test.cc +++ b/cpp/src/parquet/column_writer_test.cc @@ -64,7 +64,6 @@ const int64_t DICTIONARY_PAGE_SIZE = 1024 * 1024; template class TestPrimitiveWriter : public PrimitiveTypedTest { public: - typedef typename TestType::c_type T; void SetUp() { this->SetupValuesOut(SMALL_SIZE); From d0818a4640bf2d86eebb418290d98d75cda796ba Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Wed, 2 Dec 2020 00:30:16 -0500 Subject: [PATCH 07/25] more template fixes --- cpp/src/parquet/column_scanner_test.cc | 6 +-- cpp/src/parquet/encoding_test.cc | 52 +++++++++++++------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/cpp/src/parquet/column_scanner_test.cc b/cpp/src/parquet/column_scanner_test.cc index 1d57c9be6b5..ea54319babe 100644 --- a/cpp/src/parquet/column_scanner_test.cc +++ b/cpp/src/parquet/column_scanner_test.cc @@ -48,7 +48,7 @@ void InitDictValues(int num_values, int dict_per_page, std::vector& template class TestFlatScanner : public ::testing::Test { public: - typedef typename Type::c_type T; + using c_type = typename Type::c_type; void InitScanner(const ColumnDescriptor* d) { std::unique_ptr pager(new test::MockPageReader(pages_)); @@ -57,7 +57,7 @@ class TestFlatScanner : public ::testing::Test { void CheckResults(int batch_size, const ColumnDescriptor* d) { TypedScanner* scanner = reinterpret_cast*>(scanner_.get()); - T val; + c_type val; bool is_null = false; int16_t def_level; int16_t rep_level; @@ -131,7 +131,7 @@ class TestFlatScanner : public ::testing::Test { int num_values_; std::vector> pages_; std::shared_ptr scanner_; - std::vector values_; + std::vector values_; std::vector def_levels_; std::vector rep_levels_; std::vector data_buffer_; // For BA and FLBA diff --git a/cpp/src/parquet/encoding_test.cc b/cpp/src/parquet/encoding_test.cc index 6dfc956e037..d857696eb2f 100644 --- a/cpp/src/parquet/encoding_test.cc +++ b/cpp/src/parquet/encoding_test.cc @@ -175,7 +175,7 @@ std::shared_ptr ExampleDescr() { template class TestEncodingBase : public ::testing::Test { public: - typedef typename Type::c_type T; + using c_type = typename Type::c_type; static constexpr int TYPE = Type::type_num; void SetUp() { @@ -188,11 +188,11 @@ class TestEncodingBase : public ::testing::Test { void InitData(int nvalues, int repeats) { num_values_ = nvalues * repeats; - input_bytes_.resize(num_values_ * sizeof(T)); - output_bytes_.resize(num_values_ * sizeof(T)); - draws_ = reinterpret_cast(input_bytes_.data()); - decode_buf_ = reinterpret_cast(output_bytes_.data()); - GenerateData(nvalues, draws_, &data_buffer_); + input_bytes_.resize(num_values_ * sizeof(c_type)); + output_bytes_.resize(num_values_ * sizeof(c_type)); + draws_ = reinterpret_cast(input_bytes_.data()); + decode_buf_ = reinterpret_cast(output_bytes_.data()); + GenerateData(nvalues, draws_, &data_buffer_); // add some repeated values for (int j = 1; j < repeats; ++j) { @@ -230,8 +230,8 @@ class TestEncodingBase : public ::testing::Test { int num_values_; int type_length_; - T* draws_; - T* decode_buf_; + c_type* draws_; + c_type* decode_buf_; std::vector input_bytes_; std::vector output_bytes_; std::vector data_buffer_; @@ -255,7 +255,7 @@ class TestEncodingBase : public ::testing::Test { template class TestPlainEncoding : public TestEncodingBase { public: - typedef typename Type::c_type T; + using c_type = typename Type::c_type; static constexpr int TYPE = Type::type_num; virtual void CheckRoundtrip() { @@ -268,7 +268,7 @@ class TestPlainEncoding : public TestEncodingBase { static_cast(encode_buffer_->size())); int values_decoded = decoder->Decode(decode_buf_, num_values_); ASSERT_EQ(num_values_, values_decoded); - ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); + ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); } void CheckRoundtripSpaced(const uint8_t* valid_bits, int64_t valid_bits_offset) { @@ -288,7 +288,7 @@ class TestPlainEncoding : public TestEncodingBase { auto values_decoded = decoder->DecodeSpaced(decode_buf_, num_values_, null_count, valid_bits, valid_bits_offset); ASSERT_EQ(num_values_, values_decoded); - ASSERT_NO_FATAL_FAILURE(VerifyResultsSpaced(decode_buf_, draws_, num_values_, + ASSERT_NO_FATAL_FAILURE(VerifyResultsSpaced(decode_buf_, draws_, num_values_, valid_bits, valid_bits_offset)); } @@ -330,7 +330,7 @@ typedef ::testing::Types class TestDictionaryEncoding : public TestEncodingBase { public: - typedef typename Type::c_type T; + using c_type = typename Type::c_type; static constexpr int TYPE = Type::type_num; void CheckRoundtrip() { @@ -371,14 +371,14 @@ class TestDictionaryEncoding : public TestEncodingBase { // TODO(wesm): The DictionaryDecoder must stay alive because the decoded // values' data is owned by a buffer inside the DictionaryEncoder. We // should revisit when data lifetime is reviewed more generally. - ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); + ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); // Also test spaced decoding decoder->SetData(num_values_, indices->data(), static_cast(indices->size())); values_decoded = decoder->DecodeSpaced(decode_buf_, num_values_, 0, valid_bits.data(), 0); ASSERT_EQ(num_values_, values_decoded); - ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); + ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); } protected: @@ -1019,7 +1019,7 @@ TEST_F(DictEncoding, CheckDecodeIndicesNoNulls) { template class TestByteStreamSplitEncoding : public TestEncodingBase { public: - typedef typename Type::c_type T; + using c_type = typename Type::c_type; static constexpr int TYPE = Type::type_num; void CheckRoundtrip() override { @@ -1034,7 +1034,7 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { static_cast(encode_buffer_->size())); int values_decoded = decoder->Decode(decode_buf_, num_values_); ASSERT_EQ(num_values_, values_decoded); - ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); + ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, draws_, num_values_)); } { @@ -1046,14 +1046,14 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { for (int i = 0; i < num_values_; i += step) { int num_decoded = decoder->Decode(decode_buf_, step); ASSERT_EQ(num_decoded, std::min(step, remaining)); - ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, &draws_[i], num_decoded)); + ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, &draws_[i], num_decoded)); remaining -= num_decoded; } } { std::vector valid_bits(::arrow::BitUtil::BytesForBits(num_values_), 0); - std::vector expected_filtered_output; + std::vector expected_filtered_output; const int every_nth = 5; expected_filtered_output.reserve((num_values_ + every_nth - 1) / every_nth); ::arrow::internal::BitmapWriter writer{valid_bits.data(), 0, num_values_}; @@ -1075,7 +1075,7 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { int values_decoded = decoder->Decode(decode_buf_, num_values_); ASSERT_EQ(expected_size, values_decoded); ASSERT_NO_FATAL_FAILURE( - VerifyResults(decode_buf_, expected_filtered_output.data(), expected_size)); + VerifyResults(decode_buf_, expected_filtered_output.data(), expected_size)); } } @@ -1086,11 +1086,11 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { USING_BASE_MEMBERS(); void CheckDecode(const uint8_t* encoded_data, const int64_t encoded_data_size, - const T* expected_decoded_data, const int num_elements) { + const c_type* expected_decoded_data, const int num_elements) { std::unique_ptr> decoder = MakeTypedDecoder(Encoding::BYTE_STREAM_SPLIT); decoder->SetData(num_elements, encoded_data, static_cast(encoded_data_size)); - std::vector decoded_data(num_elements); + std::vector decoded_data(num_elements); int num_decoded_elements = decoder->Decode(decoded_data.data(), num_elements); ASSERT_EQ(num_elements, num_decoded_elements); for (size_t i = 0U; i < decoded_data.size(); ++i) { @@ -1099,7 +1099,7 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { ASSERT_EQ(0, decoder->values_left()); } - void CheckEncode(const T* data, const int num_elements, + void CheckEncode(const c_type* data, const int num_elements, const uint8_t* expected_encoded_data, const int64_t encoded_data_size) { std::unique_ptr> encoder = @@ -1114,11 +1114,11 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { } }; -template -static std::vector ToLittleEndian(const std::vector& input) { - std::vector data(input.size()); +template +static std::vector ToLittleEndian(const std::vector& input) { + std::vector data(input.size()); std::transform(input.begin(), input.end(), data.begin(), - [](const T& value) { return ::arrow::BitUtil::ToLittleEndian(value); }); + [](const c_type& value) { return ::arrow::BitUtil::ToLittleEndian(value); }); return data; } From b6cc50bf391b7277e65f7cfe65f0662bcbc18a43 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Wed, 2 Dec 2020 01:06:35 -0500 Subject: [PATCH 08/25] linting --- cpp/src/parquet/encoding_test.cc | 14 ++++++++------ cpp/src/parquet/statistics_test.cc | 14 +++++++------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/cpp/src/parquet/encoding_test.cc b/cpp/src/parquet/encoding_test.cc index d857696eb2f..41a3a5c0149 100644 --- a/cpp/src/parquet/encoding_test.cc +++ b/cpp/src/parquet/encoding_test.cc @@ -289,7 +289,7 @@ class TestPlainEncoding : public TestEncodingBase { valid_bits, valid_bits_offset); ASSERT_EQ(num_values_, values_decoded); ASSERT_NO_FATAL_FAILURE(VerifyResultsSpaced(decode_buf_, draws_, num_values_, - valid_bits, valid_bits_offset)); + valid_bits, valid_bits_offset)); } protected: @@ -1046,7 +1046,8 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { for (int i = 0; i < num_values_; i += step) { int num_decoded = decoder->Decode(decode_buf_, step); ASSERT_EQ(num_decoded, std::min(step, remaining)); - ASSERT_NO_FATAL_FAILURE(VerifyResults(decode_buf_, &draws_[i], num_decoded)); + ASSERT_NO_FATAL_FAILURE( + VerifyResults(decode_buf_, &draws_[i], num_decoded)); remaining -= num_decoded; } } @@ -1074,8 +1075,8 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { static_cast(encode_buffer_->size())); int values_decoded = decoder->Decode(decode_buf_, num_values_); ASSERT_EQ(expected_size, values_decoded); - ASSERT_NO_FATAL_FAILURE( - VerifyResults(decode_buf_, expected_filtered_output.data(), expected_size)); + ASSERT_NO_FATAL_FAILURE(VerifyResults( + decode_buf_, expected_filtered_output.data(), expected_size)); } } @@ -1117,8 +1118,9 @@ class TestByteStreamSplitEncoding : public TestEncodingBase { template static std::vector ToLittleEndian(const std::vector& input) { std::vector data(input.size()); - std::transform(input.begin(), input.end(), data.begin(), - [](const c_type& value) { return ::arrow::BitUtil::ToLittleEndian(value); }); + std::transform(input.begin(), input.end(), data.begin(), [](const c_type& value) { + return ::arrow::BitUtil::ToLittleEndian(value); + }); return data; } diff --git a/cpp/src/parquet/statistics_test.cc b/cpp/src/parquet/statistics_test.cc index c9f2e347750..e984f7c5f81 100644 --- a/cpp/src/parquet/statistics_test.cc +++ b/cpp/src/parquet/statistics_test.cc @@ -254,13 +254,13 @@ TEST(Comparison, UnknownSortOrder) { template class TestStatistics : public PrimitiveTypedTest { public: - using T = typename TestType::c_type; + using c_type = typename TestType::c_type; - std::vector GetDeepCopy( - const std::vector&); // allocates new memory for FLBA/ByteArray + std::vector GetDeepCopy( + const std::vector&); // allocates new memory for FLBA/ByteArray - T* GetValuesPointer(std::vector&); - void DeepFree(std::vector&); + c_type* GetValuesPointer(std::vector&); + void DeepFree(std::vector&); void TestMinMaxEncode() { this->GenerateData(1000); @@ -358,8 +358,8 @@ class TestStatistics : public PrimitiveTypedTest { batch_num_values - batch_null_count, 1); auto beg = this->values_.begin() + i * num_values / 2; auto end = beg + batch_num_values; - std::vector batch = GetDeepCopy(std::vector(beg, end)); - T* batch_values_ptr = GetValuesPointer(batch); + std::vector batch = GetDeepCopy(std::vector(beg, end)); + c_type* batch_values_ptr = GetValuesPointer(batch); column_writer->WriteBatch(batch_num_values, definition_levels.data(), nullptr, batch_values_ptr); DeepFree(batch); From ae3fb17aec64b7cd972b876700ddc3869071924b Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Wed, 2 Dec 2020 17:52:48 -0500 Subject: [PATCH 09/25] Windows template fix? --- cpp/src/parquet/statistics_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/parquet/statistics_test.cc b/cpp/src/parquet/statistics_test.cc index e984f7c5f81..8e1d958a205 100644 --- a/cpp/src/parquet/statistics_test.cc +++ b/cpp/src/parquet/statistics_test.cc @@ -482,10 +482,10 @@ void TestStatistics::TestMinMaxEncode() { ASSERT_EQ(statistics1->max(), statistics2->max()); } -using TestTypes = ::testing::Types; -TYPED_TEST_SUITE(TestStatistics, TestTypes); +TYPED_TEST_SUITE(TestStatistics, Types); TYPED_TEST(TestStatistics, MinMaxEncode) { this->SetUpSchema(Repetition::REQUIRED); From c4d813b61d5545281b64c99b0ffebb66c3d0845d Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Wed, 2 Dec 2020 17:59:58 -0500 Subject: [PATCH 10/25] review comments --- cpp/src/parquet/encoding_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/parquet/encoding_test.cc b/cpp/src/parquet/encoding_test.cc index 41a3a5c0149..3397ceda6b2 100644 --- a/cpp/src/parquet/encoding_test.cc +++ b/cpp/src/parquet/encoding_test.cc @@ -489,7 +489,7 @@ class TestArrowBuilderDecoding : public ::testing::Test { for (auto np : null_probabilities_) { InitTestCase(np); if (null_count_ > 0) { - GTEST_SKIP(); + continue; } typename EncodingTraits::Accumulator acc; acc.builder.reset(new ::arrow::BinaryBuilder); @@ -504,7 +504,7 @@ class TestArrowBuilderDecoding : public ::testing::Test { for (auto np : null_probabilities_) { InitTestCase(np); if (null_count_ > 0) { - GTEST_SKIP(); + continue; } auto builder = CreateDictBuilder(); auto actual_num_values = decoder_->DecodeArrowNonNull(num_values_, builder.get()); From 3ac08a2934d3737cd13994fa764dcaa475e50b90 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Wed, 2 Dec 2020 18:12:25 -0500 Subject: [PATCH 11/25] linting --- cpp/src/parquet/column_writer_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/src/parquet/column_writer_test.cc b/cpp/src/parquet/column_writer_test.cc index a914669a9ff..752d5c4184c 100644 --- a/cpp/src/parquet/column_writer_test.cc +++ b/cpp/src/parquet/column_writer_test.cc @@ -64,7 +64,6 @@ const int64_t DICTIONARY_PAGE_SIZE = 1024 * 1024; template class TestPrimitiveWriter : public PrimitiveTypedTest { public: - void SetUp() { this->SetupValuesOut(SMALL_SIZE); writer_properties_ = default_writer_properties(); From 4c2385528930448e26495f5958e302fbbe136c65 Mon Sep 17 00:00:00 2001 From: Andrew Wieteska Date: Wed, 2 Dec 2020 18:32:51 -0500 Subject: [PATCH 12/25] more linting --- cpp/src/parquet/statistics_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/parquet/statistics_test.cc b/cpp/src/parquet/statistics_test.cc index 8e1d958a205..57d9e8cbe31 100644 --- a/cpp/src/parquet/statistics_test.cc +++ b/cpp/src/parquet/statistics_test.cc @@ -482,8 +482,8 @@ void TestStatistics::TestMinMaxEncode() { ASSERT_EQ(statistics1->max(), statistics2->max()); } -using Types = ::testing::Types; +using Types = ::testing::Types; TYPED_TEST_SUITE(TestStatistics, Types); From 63e421ef74d2634792d2c0586502555ee8b0c6ef Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Sun, 6 Dec 2020 17:14:05 +0900 Subject: [PATCH 13/25] Use static rpath on macOS --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 22 ++++++--------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 9cb6d9e310c..c113e520d84 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1625,27 +1625,17 @@ macro(build_gtest) "${_GTEST_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_LIBRARY_SUFFIX}" ) set(GTEST_CMAKE_ARGS - ${EP_COMMON_TOOLCHAIN} - -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} "-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}" - -DCMAKE_INSTALL_LIBDIR=lib + ${EP_COMMON_TOOLCHAIN} -DBUILD_SHARED_LIBS=ON + -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS} - -DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS}) + -DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS} + -DCMAKE_INSTALL_LIBDIR=lib + -DCMAKE_INSTALL_NAME_DIR=$/lib + -DCMAKE_MACOSX_RPATH=OFF) set(GMOCK_INCLUDE_DIR "${GTEST_PREFIX}/include") - if(APPLE) - set(GTEST_CMAKE_ARGS ${GTEST_CMAKE_ARGS} "-DCMAKE_MACOSX_RPATH:BOOL=ON") - endif() - - if(CMAKE_GENERATOR STREQUAL "Xcode") - # Xcode projects support multi-configuration builds. This forces the gtest build - # to use the same output directory as a single-configuration Makefile driven build. - list( - APPEND GTEST_CMAKE_ARGS "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY=${_GTEST_LIBRARY_DIR}" - "-DCMAKE_LIBRARY_OUTPUT_DIRECTORY_${CMAKE_BUILD_TYPE}=${_GTEST_RUNTIME_DIR}") - endif() - add_definitions(-DGTEST_LINKED_AS_SHARED_LIBRARY=1) if(MSVC AND NOT ARROW_USE_STATIC_CRT) From 361e3e81f778ab25df272cb867d9760843f47f62 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Sun, 6 Dec 2020 18:28:31 +0900 Subject: [PATCH 14/25] Ensure using UTF-8 for source encoding with Visual C++ --- cpp/cmake_modules/SetupCxxFlags.cmake | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake index a5cd95bd7ab..5f6544ffaf9 100644 --- a/cpp/cmake_modules/SetupCxxFlags.cmake +++ b/cpp/cmake_modules/SetupCxxFlags.cmake @@ -177,6 +177,10 @@ if(WIN32) # Support large object code set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /bigobj") + + # We may use UTF-8 in source code such as + # cpp/src/arrow/compute/kernels/scalar_string_test.cc + set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /utf-8") else() # MinGW check_cxx_compiler_flag(-Wa,-mbig-obj CXX_SUPPORTS_BIG_OBJ) From efca6b9c17b25ccf127f91887de3db2bc2ed647b Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Sun, 6 Dec 2020 18:28:57 +0900 Subject: [PATCH 15/25] Fix type name conflict with Visual C++ --- cpp/src/parquet/file_serialize_test.cc | 2 -- cpp/src/parquet/test_util.h | 20 ++++++++++---------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/cpp/src/parquet/file_serialize_test.cc b/cpp/src/parquet/file_serialize_test.cc index ad9e02e3655..90e074f1153 100644 --- a/cpp/src/parquet/file_serialize_test.cc +++ b/cpp/src/parquet/file_serialize_test.cc @@ -40,8 +40,6 @@ namespace test { template class TestSerialize : public PrimitiveTypedTest { public: - typedef typename TestType::c_type T; - void SetUp() { num_columns_ = 4; num_rowgroups_ = 4; diff --git a/cpp/src/parquet/test_util.h b/cpp/src/parquet/test_util.h index 737ecc2fdde..e8bfd613e64 100644 --- a/cpp/src/parquet/test_util.h +++ b/cpp/src/parquet/test_util.h @@ -204,7 +204,7 @@ class MockPageReader : public PageReader { template class DataPageBuilder { public: - typedef typename Type::c_type T; + using c_type = typename Type::c_type; // This class writes data and metadata to the passed inputs explicit DataPageBuilder(ArrowOutputStream* sink) @@ -235,7 +235,7 @@ class DataPageBuilder { have_rep_levels_ = true; } - void AppendValues(const ColumnDescriptor* d, const std::vector& values, + void AppendValues(const ColumnDescriptor* d, const std::vector& values, Encoding::type encoding = Encoding::PLAIN) { std::shared_ptr values_sink = EncodeValues( encoding, false, values.data(), static_cast(values.size()), d); @@ -610,7 +610,7 @@ inline std::string TestColumnName(int i) { template class PrimitiveTypedTest : public ::testing::Test { public: - typedef typename TestType::c_type T; + using c_type = typename TestType::c_type; void SetUpSchema(Repetition::type repetition, int num_columns = 1) { std::vector fields; @@ -633,19 +633,19 @@ class PrimitiveTypedTest : public ::testing::Test { SchemaDescriptor schema_; // Input buffers - std::vector values_; + std::vector values_; std::vector def_levels_; std::vector buffer_; // Pointer to the values, needed as we cannot use std::vector::data() - T* values_ptr_; + c_type* values_ptr_; std::vector bool_buffer_; // Output buffers - std::vector values_out_; + std::vector values_out_; std::vector bool_buffer_out_; - T* values_out_ptr_; + c_type* values_out_ptr_; }; template @@ -654,7 +654,7 @@ inline void PrimitiveTypedTest::SyncValuesOut() {} template <> inline void PrimitiveTypedTest::SyncValuesOut() { std::vector::const_iterator source_iterator = bool_buffer_out_.begin(); - std::vector::iterator destination_iterator = values_out_.begin(); + std::vector::iterator destination_iterator = values_out_.begin(); while (source_iterator != bool_buffer_out_.end()) { *destination_iterator++ = *source_iterator++ != 0; } @@ -685,7 +685,7 @@ inline void PrimitiveTypedTest::GenerateData(int64_t num_values) { def_levels_.resize(num_values); values_.resize(num_values); - InitValues(static_cast(num_values), values_, buffer_); + InitValues(static_cast(num_values), values_, buffer_); values_ptr_ = values_.data(); std::fill(def_levels_.begin(), def_levels_.end(), 1); @@ -696,7 +696,7 @@ inline void PrimitiveTypedTest::GenerateData(int64_t num_values) { def_levels_.resize(num_values); values_.resize(num_values); - InitValues(static_cast(num_values), values_, buffer_); + InitValues(static_cast(num_values), values_, buffer_); bool_buffer_.resize(num_values); std::copy(values_.begin(), values_.end(), bool_buffer_.begin()); values_ptr_ = reinterpret_cast(bool_buffer_.data()); From f499ffb427641778bbd679286781b59e1b9bb955 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 06:26:50 +0900 Subject: [PATCH 16/25] Disable C5105 with Visual C++ --- cpp/cmake_modules/SetupCxxFlags.cmake | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake index 5f6544ffaf9..402d18f19d6 100644 --- a/cpp/cmake_modules/SetupCxxFlags.cmake +++ b/cpp/cmake_modules/SetupCxxFlags.cmake @@ -159,6 +159,23 @@ if(WIN32) set(CXX_COMMON_FLAGS "/W3 /EHsc") endif() + # Disable C5105 (macro expansion producing 'defined' has undefined + # behavior) warning because there are codes that produce this + # warning in Windows Kits. e.g.: + # + # #define _CRT_INTERNAL_NONSTDC_NAMES \ + # ( \ + # ( defined _CRT_DECLARE_NONSTDC_NAMES && _CRT_DECLARE_NONSTDC_NAMES) || \ + # (!defined _CRT_DECLARE_NONSTDC_NAMES && !__STDC__ ) \ + # ) + # + # See also: + # * C5105: https://docs.microsoft.com/en-US/cpp/error-messages/compiler-warnings/c5105 + # * Related reports: + # * https://developercommunity.visualstudio.com/content/problem/387684/c5105-with-stdioh-and-experimentalpreprocessor.html + # * https://developercommunity.visualstudio.com/content/problem/1249671/stdc17-generates-warning-compiling-windowsh.html + set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd5105") + if(ARROW_USE_STATIC_CRT) foreach(c_flag CMAKE_CXX_FLAGS From ed98d6e80a13b673c97101584f7db73c7d944156 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 06:36:08 +0900 Subject: [PATCH 17/25] Copy GoogleTest libraries to build output directory on Windows --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 27 ++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index c113e520d84..5ca6ea3d6ad 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1625,7 +1625,6 @@ macro(build_gtest) "${_GTEST_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_LIBRARY_SUFFIX}" ) set(GTEST_CMAKE_ARGS - "-DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}" ${EP_COMMON_TOOLCHAIN} -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} @@ -1633,6 +1632,7 @@ macro(build_gtest) -DCMAKE_CXX_FLAGS_${UPPERCASE_BUILD_TYPE}=${GTEST_CMAKE_CXX_FLAGS} -DCMAKE_INSTALL_LIBDIR=lib -DCMAKE_INSTALL_NAME_DIR=$/lib + -DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX} -DCMAKE_MACOSX_RPATH=OFF) set(GMOCK_INCLUDE_DIR "${GTEST_PREFIX}/include") @@ -1647,6 +1647,31 @@ macro(build_gtest) BUILD_BYPRODUCTS ${GTEST_SHARED_LIB} ${GTEST_MAIN_SHARED_LIB} ${GMOCK_SHARED_LIB} CMAKE_ARGS ${GTEST_CMAKE_ARGS} ${EP_LOG_OPTIONS}) + if(WIN32) + # Copy the built shared libraries to the same directory as our + # test programs because Windows doesn't provided rpath (run-time + # search path) feature. We need to put these shared libraries to + # the same directory as our test programs or add + # _GTEST_LIBRARY_DIR to PATH when we run our test programs. We + # choose the former because the latter may be forgotten. + externalproject_add_step(googletest_ep copy + COMMAND ${CMAKE_COMMAND} + -E + copy + ${GTEST_SHARED_LIB} + ${BUILD_OUTPUT_ROOT_DIRECTORY} + COMMAND ${CMAKE_COMMAND} + -E + copy + ${GMOCK_SHARED_LIB} + ${BUILD_OUTPUT_ROOT_DIRECTORY} + COMMAND ${CMAKE_COMMAND} + -E + copy + ${GTEST_MAIN_SHARED_LIB} + ${BUILD_OUTPUT_ROOT_DIRECTORY} + DEPENDEES install) + endif() # The include directory must exist before it is referenced by a target. file(MAKE_DIRECTORY "${GTEST_INCLUDE_DIR}") From b6710fbdbdea2a41090a3254aebbbb250d935edb Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 08:59:17 +0900 Subject: [PATCH 18/25] Copy .dll not .lib --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 5ca6ea3d6ad..12aec40ed32 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1654,21 +1654,34 @@ macro(build_gtest) # the same directory as our test programs or add # _GTEST_LIBRARY_DIR to PATH when we run our test programs. We # choose the former because the latter may be forgotten. + set(_GTEST_RUNTIME_DIR "${GTEST_PREFIX}/bin") + set(_GTEST_RUNTIME_SUFFIX + "${CMAKE_GTEST_DEBUG_EXTENSION}${CMAKE_SHARED_LIBRARY_SUFFIX}") + set( + _GTEST_RUNTIME_LIB + "${_GTEST_RUNTIME_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest${_GTEST_RUNTIME_SUFFIX}") + set( + _GMOCK_RUNTIME_LIB + "${_GTEST_RUNTIME_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gmock${_GTEST_RUNTIME_SUFFIX}") + set( + _GTEST_MAIN_RUNTIME_LIB + "${_GTEST_RUNTIME_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_RUNTIME_SUFFIX}" + ) externalproject_add_step(googletest_ep copy COMMAND ${CMAKE_COMMAND} -E copy - ${GTEST_SHARED_LIB} + ${_GTEST_RUNTIME_LIB} ${BUILD_OUTPUT_ROOT_DIRECTORY} COMMAND ${CMAKE_COMMAND} -E copy - ${GMOCK_SHARED_LIB} + ${_GMOCK_RUNTIME_LIB} ${BUILD_OUTPUT_ROOT_DIRECTORY} COMMAND ${CMAKE_COMMAND} -E copy - ${GTEST_MAIN_SHARED_LIB} + ${_GTEST_MAIN_RUNTIME_LIB} ${BUILD_OUTPUT_ROOT_DIRECTORY} DEPENDEES install) endif() From b2da86205dcfabd88b8296403d703ba71e536da1 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 09:15:05 +0900 Subject: [PATCH 19/25] Ensure creating build output root directory --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 12aec40ed32..9d6fff12d32 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1668,6 +1668,8 @@ macro(build_gtest) "${_GTEST_RUNTIME_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_RUNTIME_SUFFIX}" ) externalproject_add_step(googletest_ep copy + COMMAND ${CMAKE_COMMAND} -E make_directory + ${BUILD_OUTPUT_ROOT_DIRECTORY} COMMAND ${CMAKE_COMMAND} -E copy From 5cbcc26b088be53042e95a3f405a297876b94087 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 09:38:59 +0900 Subject: [PATCH 20/25] Add a missing empty line --- cpp/src/parquet/statistics_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/src/parquet/statistics_test.cc b/cpp/src/parquet/statistics_test.cc index 57d9e8cbe31..2ebce7d7fbe 100644 --- a/cpp/src/parquet/statistics_test.cc +++ b/cpp/src/parquet/statistics_test.cc @@ -610,6 +610,7 @@ template class TestStatisticsSortOrder : public ::testing::Test { public: using c_type = typename TestType::c_type; + void AddNodes(std::string name) { fields_.push_back(schema::PrimitiveNode::Make( name, Repetition::REQUIRED, TestType::type_num, ConvertedType::NONE)); From d68e11e26ab8daffd32c625b149895df0789bd0d Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 10:45:28 +0900 Subject: [PATCH 21/25] Add support multi-config generator on Windows --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 9d6fff12d32..e5ce639c0f6 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1667,24 +1667,31 @@ macro(build_gtest) _GTEST_MAIN_RUNTIME_LIB "${_GTEST_RUNTIME_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_RUNTIME_SUFFIX}" ) + # Want to use GENERATOR_IS_MULTI_CONFIG here but it requires CMake + # 3.9 or later. + if(MSVC) + set(_GTEST_RUNTIME_OUTPUT_DIR "${BUILD_OUTPUT_ROOT_DIRECTORY}/${CMAKE_BUILD_TYPE}") + else() + set(_GTEST_RUNTIME_OUTPUT_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY}) + endif() externalproject_add_step(googletest_ep copy COMMAND ${CMAKE_COMMAND} -E make_directory - ${BUILD_OUTPUT_ROOT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${CMAKE_COMMAND} -E copy ${_GTEST_RUNTIME_LIB} - ${BUILD_OUTPUT_ROOT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${CMAKE_COMMAND} -E copy ${_GMOCK_RUNTIME_LIB} - ${BUILD_OUTPUT_ROOT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} COMMAND ${CMAKE_COMMAND} -E copy ${_GTEST_MAIN_RUNTIME_LIB} - ${BUILD_OUTPUT_ROOT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} DEPENDEES install) endif() From 23c5744fd5c33b373f7c9cc2d08bbf7fe1470f16 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 10:48:27 +0900 Subject: [PATCH 22/25] Fix a typo --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index e5ce639c0f6..7e5bb9fcd43 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1676,22 +1676,22 @@ macro(build_gtest) endif() externalproject_add_step(googletest_ep copy COMMAND ${CMAKE_COMMAND} -E make_directory - ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIR} COMMAND ${CMAKE_COMMAND} -E copy ${_GTEST_RUNTIME_LIB} - ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIR} COMMAND ${CMAKE_COMMAND} -E copy ${_GMOCK_RUNTIME_LIB} - ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIR} COMMAND ${CMAKE_COMMAND} -E copy ${_GTEST_MAIN_RUNTIME_LIB} - ${_GTEST_RUNTIME_OUTPUT_DIRECTORY} + ${_GTEST_RUNTIME_OUTPUT_DIR} DEPENDEES install) endif() From f3f04e865633d2ceb5ad805e49b311fcbea08084 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 10:57:25 +0900 Subject: [PATCH 23/25] Use GENERATOR_IS_MULTI_CONFIG --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 7e5bb9fcd43..3db68a83e82 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1667,9 +1667,12 @@ macro(build_gtest) _GTEST_MAIN_RUNTIME_LIB "${_GTEST_RUNTIME_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${_GTEST_RUNTIME_SUFFIX}" ) - # Want to use GENERATOR_IS_MULTI_CONFIG here but it requires CMake - # 3.9 or later. - if(MSVC) + if(CMAKE_VERSION VERSION_LESS 3.9) + message( + FATAL_ERROR + "Building GoogleTest from source on Windows requires at least CMake 3.9") + endif() + if(GENERATOR_IS_MULTI_CONFIG) set(_GTEST_RUNTIME_OUTPUT_DIR "${BUILD_OUTPUT_ROOT_DIRECTORY}/${CMAKE_BUILD_TYPE}") else() set(_GTEST_RUNTIME_OUTPUT_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY}) From 49b20ebe6cdac9d25f0e8d3322d36954fb8a2223 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 11:41:41 +0900 Subject: [PATCH 24/25] Debug --- ci/scripts/cpp_test.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/scripts/cpp_test.sh b/ci/scripts/cpp_test.sh index d7e239b7c07..d8901c819c5 100755 --- a/ci/scripts/cpp_test.sh +++ b/ci/scripts/cpp_test.sh @@ -71,6 +71,8 @@ esac pushd ${build_dir} +ls -R + if ! which python > /dev/null 2>&1; then export PYTHON=python3 fi From e201ab51965f6dc646383f6fbc4e999d880fb0f6 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 7 Dec 2020 12:04:05 +0900 Subject: [PATCH 25/25] Fix property usage --- ci/scripts/cpp_test.sh | 2 -- cpp/cmake_modules/ThirdpartyToolchain.cmake | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ci/scripts/cpp_test.sh b/ci/scripts/cpp_test.sh index d8901c819c5..d7e239b7c07 100755 --- a/ci/scripts/cpp_test.sh +++ b/ci/scripts/cpp_test.sh @@ -71,8 +71,6 @@ esac pushd ${build_dir} -ls -R - if ! which python > /dev/null 2>&1; then export PYTHON=python3 fi diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 3db68a83e82..47e6be27309 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -1672,7 +1672,8 @@ macro(build_gtest) FATAL_ERROR "Building GoogleTest from source on Windows requires at least CMake 3.9") endif() - if(GENERATOR_IS_MULTI_CONFIG) + get_property(_GENERATOR_IS_MULTI_CONFIG GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG) + if(_GENERATOR_IS_MULTI_CONFIG) set(_GTEST_RUNTIME_OUTPUT_DIR "${BUILD_OUTPUT_ROOT_DIRECTORY}/${CMAKE_BUILD_TYPE}") else() set(_GTEST_RUNTIME_OUTPUT_DIR ${BUILD_OUTPUT_ROOT_DIRECTORY})