[Benchmarks] All cpp microbenchmarks are failing with new GTest CMake Error

[austin3dickey]

Describe the bug, including details regarding any error messages, version, and platform.

Since the PR #34765 was merged, the cpp-micro benchmarks are failing with the following error:

CMake Error at cmake_modules/ThirdpartyToolchain.cmake:2236 (message):
  System GTest is built with a C++ standard lower than 17.  Use bundled GTest
  via passing in CMake flag

  -DGTest_SOURCE="BUNDLED"

Here is an example Buildkite build.

This error message was explicitly added in that PR, so we'll need to take action to correct it.

cc @HaochengLIU

Component(s)

Benchmarking, C++

[jonkeane]

Oddly, it looks like from the output in buildkie, GTest_SOURCE is being set to bundled: https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-arm64-m6g-linux-compute/builds/2413#018736cc-c891-4214-9536-87de3d50e702/6-1069

I'm also curious if this else block is indicating that GTest_SOURCE=BUNDLED works?

arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake

Lines 2257 to 2261 in efb0be0

	else()
	# TODO: How to solve BUNDLED case? Do we install bundled GoogleTest?
	# string(APPEND ARROW_TESTING_PC_CFLAGS " -I${GTEST_INCLUDE_DIR}")
	# string(APPEND ARROW_TESTING_PC_LIBS " -lgtest")
	endif()

Also is it accurate | right that we have the message(FATAL_ERROR ... there above the if/else checking GTest_SOURCE?

[HaochengLIU]

Hi, I do not have access to the output you linked. What PR #34765 does is to error out when discover discrepancy between CXX standard between gtest and Arrow. If it's indeed set to BUNDLED, it will hits this CMake block and build GTest from source(it should be built with C++17 if i read code here correctly? )

[HaochengLIU]

A quick fix is to change if((${gtest_cxx_standard} STREQUAL "cxx_std_11") OR (${gtest_cxx_standard} STREQUAL to if(AAPLE AND (${gtest_cxx_standard} STREQUAL "cxx_std_11") OR (${gtest_cxx_standard} STREQUAL in this line, limiting the scope to APPLE only. Add discuss the long term fix in this follow up issue #34813. @austin3dickey Can you please give it a shot?

[jonkeane]

Another follow up here: the build that buildkite is running uses this file for flags like these — and we do have GTest_SOURCE=BUNDLED there:

arrow/dev/conbench_envs/benchmarks.env

Line 45 in efb0be0

GTest_SOURCE=BUNDLED

[jonkeane]

Some more notes, that benchmark.env (and the hooks.sh next to it in the hierarchy, which calls it) is called by https://github.com/voltrondata-labs/arrow-benchmarks-ci/blob/0d74a7a525d631f26057ad6a6b4248df96e96ec3/buildkite/benchmark/utils.sh#L34-L35 during our benchmark builds.

So this should just work already. @assignUser Do you have a chance to take a look at this?

[HaochengLIU]

• So this should just work already. @assignUser Do you have a chance to take a look at this?

Do you mean to assign the issue to that user or it's a typo?

[jonkeane]

Do you mean to assign the issue to that user or it's a typo?

I did indeed mean to tag Jacob (@assignUser) who knows a bunch about our CI stuff (as well as has some experience with the benchmarking setup). But I totally see how that username looks funny there 😂

[assignUser]

:D I'll have a look later today!

[HaochengLIU]

haha, I was firstly shocked by how smart github is to map assignUser to issue assignee :)

[kou]

We may want to move the change to if(GTest_SOURCE STREQUAL "SYSTEM"):

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index caf33f224b..c0b289cea8 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2229,15 +2229,15 @@ if(ARROW_TESTING)
                      1.10.0
                      USE_CONFIG
                      ${GTEST_USE_CONFIG})
-  get_target_property(gtest_cxx_standard GTest::gtest INTERFACE_COMPILE_FEATURES)
+  if(GTest_SOURCE STREQUAL "SYSTEM")
+    get_target_property(gtest_cxx_standard GTest::gtest INTERFACE_COMPILE_FEATURES)
 
-  if((${gtest_cxx_standard} STREQUAL "cxx_std_11") OR (${gtest_cxx_standard} STREQUAL
-                                                       "cxx_std_14"))
-    message(FATAL_ERROR "System GTest is built with a C++ standard lower than 17. Use bundled GTest via passing in CMake flag
--DGTest_SOURCE=\"BUNDLED\"")
-  endif()
+    if((${gtest_cxx_standard} STREQUAL "cxx_std_11") OR (${gtest_cxx_standard} STREQUAL
+                                                         "cxx_std_14"))
+      message(FATAL_ERROR "System GTest is built with a C++ standard lower than 17. Use bundled GTest via passing in CMake flag
+  -DGTest_SOURCE=\"BUNDLED\"")
+    endif()
 
-  if(GTest_SOURCE STREQUAL "SYSTEM")
     find_package(PkgConfig QUIET)
     pkg_check_modules(gtest_PC
                       gtest

[HaochengLIU]

We may want to move the change to if(GTest_SOURCE STREQUAL "SYSTEM"):

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index caf33f224b..c0b289cea8 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2229,15 +2229,15 @@ if(ARROW_TESTING)
                      1.10.0
                      USE_CONFIG
                      ${GTEST_USE_CONFIG})
-  get_target_property(gtest_cxx_standard GTest::gtest INTERFACE_COMPILE_FEATURES)
+  if(GTest_SOURCE STREQUAL "SYSTEM")
+    get_target_property(gtest_cxx_standard GTest::gtest INTERFACE_COMPILE_FEATURES)
 
-  if((${gtest_cxx_standard} STREQUAL "cxx_std_11") OR (${gtest_cxx_standard} STREQUAL
-                                                       "cxx_std_14"))
-    message(FATAL_ERROR "System GTest is built with a C++ standard lower than 17. Use bundled GTest via passing in CMake flag
--DGTest_SOURCE=\"BUNDLED\"")
-  endif()
+    if((${gtest_cxx_standard} STREQUAL "cxx_std_11") OR (${gtest_cxx_standard} STREQUAL
+                                                         "cxx_std_14"))
+      message(FATAL_ERROR "System GTest is built with a C++ standard lower than 17. Use bundled GTest via passing in CMake flag
+  -DGTest_SOURCE=\"BUNDLED\"")
+    endif()
 
-  if(GTest_SOURCE STREQUAL "SYSTEM")
     find_package(PkgConfig QUIET)
     pkg_check_modules(gtest_PC
                       gtest

I'm totally fine with above change
Q: In BUNDLE mode, should gTest compiled with whatever CXX_STANDARD Arrow is using? If so, the existing code should work out of box.... It smells like a configuration issue in CI setup..? Curious to learn what Jacob's findings are.