facebookincubator / velox

A composable and fully extensible C++ execution engine library for data management systems.
https://velox-lib.io/
Apache License 2.0
3.54k stars 1.17k forks source link

googletest / gtest version bump up to the latest from 1.13? #10729

Closed zuyu closed 2 months ago

zuyu commented 3 months ago

Description

Should we do a version bump up for googletest / gtest due to the version mismatches explained below?

I understand we use 1.13 everywhere [1] [2] [3], which was released on Jan 18, 2023 [4].

Recently on Mac OS, on the other hand, w/ #10422 merged, the googletest version installed from brew [5] is the latest 1.15.2. Moreover, it would cause some confusion whether the cmake script that installs 1.13 [3] ever runs or not.

In other words, IMO we should at least keep the googletest / gtest version consistent across all the env.

[1] https://github.com/facebookincubator/velox/blob/main/scripts/velox_env_mac.yml#L51 [2] https://github.com/facebookincubator/velox/blob/main/scripts/velox_env_linux.yml#L51 [3] https://github.com/facebookincubator/velox/blob/main/CMake/resolve_dependency_modules/gtest.cmake#L16 [4] https://github.com/google/googletest/releases/tag/v1.13.0 [5] https://formulae.brew.sh/formula/googletest

assignUser commented 3 months ago

I am not opposed to a version bump, but is there really a need for it, i.e. does 1.15 cause errors that don't happen with 1.13 or something like that?

Moreover, it would cause some confusion whether the cmake script that installs 1.13 [3] ever runs or not.

The log clearly states which version was used and it can also be forced via the GTest_SOURCE envvar, so I don't think there really is a reason for confusion? :)

zuyu commented 3 months ago

There might not be a real need, but just for consistency across all the platforms.

My main confusion is that how GTest::gtest works in both the latest version and 1.13, where in 1.13 only gtest is used, but no namespace GTest.

assignUser commented 3 months ago

1.13 also provides the namespaced targets both in the export and when used as a sub project: https://github.com/google/googletest/blob/v1.13.0/googletest/CMakeLists.txt#L103