Skip to content

Commit

Permalink
apacheGH-43400: [C++] Ensure using bundled GoogleTest when we use bun…
Browse files Browse the repository at this point in the history
…dled GoogleTest (apache#43465)

### Rationale for this change

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.

### What changes are included in this PR?

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.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* GitHub Issue: apache#43400

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Jacob Wujciak-Jens <[email protected]>
  • Loading branch information
kou authored Jul 30, 2024
1 parent 96a6c45 commit a4a5562
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 5 deletions.
5 changes: 5 additions & 0 deletions cpp/cmake_modules/BuildUtils.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down
6 changes: 6 additions & 0 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down Expand Up @@ -2350,12 +2354,14 @@ if(ARROW_TESTING)

string(APPEND ARROW_TESTING_PC_LIBS " $<TARGET_FILE:GTest::gtest>")
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)
Expand Down
5 changes: 0 additions & 5 deletions dev/tasks/java-jars/github.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a4a5562

Please sign in to comment.