Update GTest to 1.12.1 #2422
Update GTest to 1.12.1 #2422
Conversation
1.12.1 as last GTest version supporting c++11 Fix LTO warnings for GTest (we already check if lto is supported) Fix ninja_test msvc use of unlink rather than _unlink warnings
| @@ -13,6 +13,7 @@ check_ipo_supported(RESULT lto_supported OUTPUT error) | |||
|
|
|||
| if(lto_supported) | |||
| message(STATUS "IPO / LTO enabled") | |||
| set(CMAKE_POLICY_DEFAULT_CMP0069 NEW) | |||
There was a problem hiding this comment.
Isn't this already default NEW since CMake 3.9?
There was a problem hiding this comment.
It is, however after reading your comment and scratching my head, I realised there's a subtle difference between cmake_policy(SET CMP0069 NEW) and set(CMAKE_POLICY_DEFAULT_CMP0069 NEW)
My understanding (which could well be incorrect) is that we need the latter here because we're propagating this policy to the gtest source, and this is the way to do that - see the last point here:
"Projects may set this variable before a call to add_subdirectory() that adds a third-party project in order to set its policies without modifying third-party code." - which whilst we don't call add_subdirectory explicitly, I'm assuming is what FetchContent does under the hood
Building it on windows 10 msvc seems to back this up. Omitting this line, using cmake_policy calls, setting the version in cmake_minimum_required to 3.15...3.24 (or basically any versions you like) all do nothing. The only thing that stops the spam - that I know of - is this line.
If there's a better way, happy to change to that instead
I suppose this line could be moved lower/closer to just above the gtest call though to reduce its scope (not that its a big deal since we're using the NEW behaviour anyway)
There was a problem hiding this comment.
OK this is a nice technique when having to use older 3rd party CMake subprojects.
|
I personally would rather see this PR merged rather than 2418 as 2422 fixes multiple issues along with being the newest/last GTest to support C++11. |
CMakeLists.txt
Outdated
| @@ -294,6 +274,10 @@ if(BUILD_TESTING) | |||
| endif() | |||
| find_package(Threads REQUIRED) | |||
| target_link_libraries(ninja_test PRIVATE libninja libninja-re2c GTest::gtest Threads::Threads) | |||
| if (MSVC) | |||
There was a problem hiding this comment.
Can this be WIN32 instead, since I do cross-compile using mingw64 on Linux
|
I agree with @scivision that this seems a better PR to merge to update GoogleTest. |
Follow up to #2418
1.12.1 as last GTest version supporting c++11
Fix LTO warnings for GTest (we already check if lto is supported)
Fix ninja_test msvc warnings about use of
unlinkrather than_unlink