From 34b107d2234b40c73fe6347e1a5594770296a320 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 29 Jul 2024 15:48:19 +0900 Subject: [PATCH 1/2] GH-43400: [C++] Ensure using bundled GoogleTest when we use bundled GoogleTest If we use bundled GoogleTest and system other dependencies such as Boost, our include path options may be: * `-isystem /opt/homebrew/include` (for Boost) * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) With this order, GoogleTest headers in `/opt/homebrew/include/` are used with bundled GoogleTest. It may cause link errors. This change introduces a new CMake target `arrow::GTest::gtest_headers` that has include paths for bundled GoogleTest. And it's always used as the first link library of all test program. With this change, our include path options are: * `-isystem build_dir/_deps/googletest-src/googletest` (for bundled GoogleTest) * `-isystem build_dir/_deps/googletest-src/googlemock` (for bundled GoogleTest) * `-isystem /opt/homebrew/include` (for Boost) With this order, we can always use our bundled GoogleTest. `arrow::GTest::gtest_headers` is defined only when we use bundled GoogleTest. So this doesn't change the system GoogleTest case. --- cpp/cmake_modules/BuildUtils.cmake | 5 +++++ cpp/cmake_modules/ThirdpartyToolchain.cmake | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/cpp/cmake_modules/BuildUtils.cmake b/cpp/cmake_modules/BuildUtils.cmake index e7523add272..692efa78376 100644 --- a/cpp/cmake_modules/BuildUtils.cmake +++ b/cpp/cmake_modules/BuildUtils.cmake @@ -721,6 +721,11 @@ function(ADD_TEST_CASE REL_TEST_NAME) "${EXECUTABLE_OUTPUT_PATH};$ENV{CONDA_PREFIX}/lib") endif() + # Ensure using bundled GoogleTest when we use bundled GoogleTest. + # ARROW_GTEST_GTEST_HEADERS is defined only when we use bundled + # GoogleTest. + target_link_libraries(${TEST_NAME} PRIVATE ${ARROW_GTEST_GTEST_HEADERS}) + if(ARG_STATIC_LINK_LIBS) # Customize link libraries target_link_libraries(${TEST_NAME} PRIVATE ${ARG_STATIC_LINK_LIBS}) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 1c8c40d6f9c..92bd80014e8 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -2306,6 +2306,10 @@ function(build_gtest) install(DIRECTORY "${googletest_SOURCE_DIR}/googlemock/include/" "${googletest_SOURCE_DIR}/googletest/include/" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}") + add_library(arrow::GTest::gtest_headers INTERFACE IMPORTED) + target_include_directories(arrow::GTest::gtest_headers + INTERFACE "${googletest_SOURCE_DIR}/googlemock/include/" + "${googletest_SOURCE_DIR}/googletest/include/") install(TARGETS gmock gmock_main gtest gtest_main EXPORT arrow_testing_targets RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}" @@ -2350,12 +2354,14 @@ if(ARROW_TESTING) string(APPEND ARROW_TESTING_PC_LIBS " $") endif() + set(ARROW_GTEST_GTEST_HEADERS) set(ARROW_GTEST_GMOCK GTest::gmock) set(ARROW_GTEST_GTEST GTest::gtest) set(ARROW_GTEST_GTEST_MAIN GTest::gtest_main) else() string(APPEND ARROW_TESTING_PC_CFLAGS " -I\${includedir}/arrow-gtest") string(APPEND ARROW_TESTING_PC_LIBS " -larrow_gtest") + set(ARROW_GTEST_GTEST_HEADERS arrow::GTest::gtest_headers) set(ARROW_GTEST_GMOCK arrow::GTest::gmock) set(ARROW_GTEST_GTEST arrow::GTest::gtest) set(ARROW_GTEST_GTEST_MAIN arrow::GTest::gtest_main) From 8aef1702bc0822702abee0f036ca8d06de4afee8 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 29 Jul 2024 15:59:44 +0900 Subject: [PATCH 2/2] Remove workaround --- dev/tasks/java-jars/github.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/dev/tasks/java-jars/github.yml b/dev/tasks/java-jars/github.yml index 58c1cedb114..77e8867652e 100644 --- a/dev/tasks/java-jars/github.yml +++ b/dev/tasks/java-jars/github.yml @@ -138,11 +138,6 @@ jobs: # used on test We uninstall Homebrew's Protobuf to ensure using # bundled Protobuf. brew uninstall protobuf - # We want to use the bundled googletest for static linking. Since - # both BUNDLED and brew options are enabled, it could cause a conflict - # when there is a version mismatch. - # We uninstall googletest to ensure using the bundled googletest. - brew uninstall googletest brew bundle --file=arrow/java/Brewfile - name: Build C++ libraries