GH-14917: [C++] Error out when GTest is compiled with a C++ standard lower than 17

[HaochengLIU]

Rationale for this change

On MacOS, the system GTest from brew uses C++14 while arrow is default to C++17.
The build will fail at the linking stage unless users explicity uses bundled GTest.

Users are often confused by unclear linking errors.

What changes are included in this PR?

At CMake configuration time, the CMake code will automatically detect and
compare the C++ standard used by GTest and arrow. If a mismatch is detected,
CMake will error out and privide meaningful guidance. It will apply both to
linux and MacOS.

Are these changes tested?

Covered by Arrow github test worker and tested on local host.

Are there any user-facing changes?

Yes, now users are given better guidance when building Arrow against the system
GTest.

• Closes: [C++] Issues with GTest when building Arrow C++ on MacOS #14917
• Closes: [C++] Compiling failed on Mac M1 #14779

[github-actions]

• Closes: [C++] Issues with GTest when building Arrow C++ on MacOS #14917

[github-actions]

⚠️ GitHub issue #14917 has been automatically assigned in GitHub to PR creator.

[HaochengLIU]

Hi @AlenkaF , after some thoughts, IMO the correct thing here is to fix things at the building level rather than asking every Arrow user to add an extra flag manually in the doc. I have richful experience with CMake thus I go ahead and touch the toolchain file.

I've tested the build settings from #14779 as well as few build configurations with test enabled and they all build smoothly. Looking forward to your thoughts.

Thanks.

[AlenkaF]

Thank you for the thoughtful contribution @HaochengLIU !

Hi @AlenkaF , after some thoughts, IMO the correct thing here is to fix things at the building level rather than asking every Arrow user to add an extra flag manually in the doc. I have richful experience with CMake thus I go ahead and touch the toolchain file.

That would for sure be useful for all that have this issue. My only thought here is that this doesn't seem to be an issue for every MacOS user. For this reason I suggested to first add this to the docs.

The change in the PR looks good to me though (will still wait for the CI and local test to approve it).

@kou what do you think about this change?

[kou]

We should not change *_SOURCE in SetupCxxFlags.cmake.
We should process *_SOURCE in ThirdpartyToolchain.cmake.

This problem is caused by C++ standard difference, right?
For example, this problem is caused when Apache Arrow C++ uses C++17 and system GoogleTest uses C++14.

Can we detect whether system GoogleTest uses different C++ standard than Apache Arrow C++? If we can detect it, we can fallback to bundled GoogleTest automatically when system GoogleTest uses different C++ standard.

[HaochengLIU]

Hi Sutou, Good point. I will check CMake and get back to you.

[HaochengLIU]

After checking the upstream FindGTest.cmake as well as the pkgconfig from GoogleTest on my host, here is my finding:

In order to detect what C++ standard is used by GoogleTest, there is no simple way to do so except by importing it first to my best knowledge. Moreover, once a target is imported, you can not rename or delete it(See Ben's - one of the main CMake developers - comment here). Thus there is no way to fall back to bundled mode.

My new proposed fix is at CMake configuration time, the CMake code will automatically detect and
compare the C++ standard used by GTest and arrow. If a mismatch is detected,
CMake will error out and provide meaningful guidance. With respect to The principle of least astonishment, I prefer to let the users add these flags by themselves.

// sample output from my host
CMake Error at cmake_modules/ThirdpartyToolchain.cmake:2238 (message):
  SYSTEM GTest is built with c++14 but project is built

  with a different standard(17).  Use bundled GTest via passing in CMake flag

  -DGTest_SOURCE="BUNDLED"
Call Stack (most recent call first):
  CMakeLists.txt:505 (include)


-- Configuring incomplete, errors occurred!

Looking forward to your thoughts. @kou .

[kou]

Can we use try_compile() https://cmake.org/cmake/help/latest/command/try_compile.html for this like we did for gRPC

arrow/cpp/src/arrow/flight/CMakeLists.txt

Lines 119 to 123 in ddd0a33

	try_compile(HAS_GRPC_VERSION ${CMAKE_CURRENT_BINARY_DIR}/try_compile
	SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/try_compile/${TEST_FILE}"
	CMAKE_FLAGS "-DINCLUDE_DIRECTORIES=${CURRENT_INCLUDE_DIRECTORIES}"
	LINK_LIBRARIES gRPC::grpc++ Threads::Threads
	OUTPUT_VARIABLE TLS_CREDENTIALS_OPTIONS_CHECK_OUTPUT CXX_STANDARD 11)

?

I think that checking INTERFACE_COMPILE_FEATURES isn't so portable.
For example, libgtest-dev on Debian/Ubuntu still uses cxx_std_11 not cxx_std_14.

For CMake target conflict, we can use different CMake target names (such as arrow::GTest::gtest) and CMake variables to refer suitable CMake targets (such as set(ARROW_GTEST_GTEST arrow::GTest::gtest)) like we did for Zstandard:

arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake

Lines 2577 to 2589 in ddd0a33

	if(ZSTD_VENDORED)
	set(ARROW_ZSTD_LIBZSTD zstd::libzstd_static)
	else()
	if(ARROW_ZSTD_USE_SHARED)
	set(ARROW_ZSTD_LIBZSTD zstd::libzstd_shared)
	else()
	set(ARROW_ZSTD_LIBZSTD zstd::libzstd_static)
	endif()
	if(NOT TARGET ${ARROW_ZSTD_LIBZSTD})
	message(FATAL_ERROR "Zstandard target doesn't exist: ${ARROW_ZSTD_LIBZSTD}")
	endif()
	message(STATUS "Found Zstandard: ${ARROW_ZSTD_LIBZSTD}")
	endif()

[kou]

We can use FindXXXAlt.cmake such as FindGTestAlt.cmake to check such additional check.
We can use FindXXXAlt.cmake by resolve_dependency(... HAVE_ALT TRUE).

[kou]

The latter.
We can just wrap existing GTestConfig.cmake like we did in FindOpenSSLAlt.cmake: https://github.com/apache/arrow/blob/main/cpp/cmake_modules/FindOpenSSLAlt.cmake

In this PR, We limit the scope to error out whenever INTERFACE_COMPILE_FEATURES from GTest is lower than that? It will be universal to all platforms and truly detect the issue at CMake configure time.

Then in another PR/issue, we can discuss how to implement try_compile and FindXXXAlt.cmake as it will need non trivial work.

It makes sense. Could you update the title of this pull request and open a new issue for the follow-up action?

[kou]

FYI: If we use the latter approach, we can implement an automatically fallback solution instead of raising an error for C++ standard miss match. (If system GoogleTest isn't suitable, we can use our bundled GoogleTest instead of stopping cmake.)

[HaochengLIU]

Thanks for the suggestion @kou , I've updated the title of the PR and opened a new follow up issue. Please take a look when you have time

[kou]

Thanks!

[HaochengLIU]

The failing test - test_generic_options - looks like a flaky one

[HaochengLIU]

My only thought here is that this doesn't seem to be an issue for every MacOS user

@AlenkaF I totally agree. With the new change, I believe the doc update is no longer needed since users will do it by themselves with a meaningful error message now?

[kou]

+1

[ursabot]

Benchmark runs are scheduled for baseline = 5dde5d4 and contender = 68dc40a. 68dc40a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.18% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 68dc40a1 ec2-t3-xlarge-us-east-2
[Failed] 68dc40a1 test-mac-arm
[Finished] 68dc40a1 ursa-i9-9960x
[Failed] 68dc40a1 ursa-thinkcentre-m75q
[Finished] 5dde5d4e ec2-t3-xlarge-us-east-2
[Failed] 5dde5d4e test-mac-arm
[Finished] 5dde5d4e ursa-i9-9960x
[Finished] 5dde5d4e ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[austin3dickey]

FYI, this PR caused the cpp-micro benchmarks to start failing. See linked issue ^

[jorisvandenbossche]

Is this limitation necessary on non-MacOS systems? I have been using the conda installed gtest on linux, and don't remember to have run into problems with this

[HaochengLIU]

Yes. It's platform agnostic, only depending on which C++ standard gtest is compiled with.

[jorisvandenbossche]

But the reported issues all seem to involve people using Mac? As mentioned, I have always been using gtest from conda on linux, and never ran into any issues with it (while now this check avoids a successful compilation)

[HaochengLIU]

It depends on how conda builds/ships gtest on linux which I'm not familiar with. When you say "while now this check avoids a successful compilation", does it happen after this #34874?

[kou]

We use INTERFACE_COMPILE_FEATURES for this approach but it's not perfect because GoogleTest always sets cxx_std_14 (or cxx_std_11 in older Google Test): https://github.com/google/googletest/blob/7ee260c54921571b18b15049304426fe151c1265/googletest/cmake/internal_utils.cmake#L204

But GoogleTest may be built with C++17 even when INTERFACE_COMPILE_FEATURES is cxx_std_14 or cxx_std_11.
It's occurred when C++ compiler uses C++17 by default. (Recent Linux distributions will use g++ that uses C++17 by default.)

I'll implement #34813. Please wait for a few days.

FYI: We need a GoogleTest that uses https://github.com/google/googletest/blob/7ee260c54921571b18b15049304426fe151c1265/googletest/include/gtest/internal/gtest-port.h#L2470-L2482

[jorisvandenbossche]

When you say "while now this check avoids a successful compilation", does it happen after this #34874?

No, because when you use ARROW_DEPENDENCY_SOURCE=CONDA, it get actually set to SYSTEM for each of the dependencies (but with the path (ARROW_PACKAGE_PREFIX) pointing to the conda prefix)

@kou thanks for taking a look!