Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update GTest to 1.12.1 #2422

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Update GTest to 1.12.1 #2422

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8227479
Update GTest to 1.12.1
Arghnews Apr 16, 2024
112576f
Change MSVC define to WIN32 to exclude mingw
Arghnews Apr 17, 2024
1f8ddb3
Only set define on Windows using MSVC
Arghnews Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 14 additions & 29 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this already default NEW since CMake 3.9?

Copy link
Contributor Author

@Arghnews Arghnews Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK this is a nice technique when having to use older 3rd party CMake subprojects.

set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_RELEASE TRUE)
else()
message(STATUS "IPO / LTO not supported: <${error}>")
Expand Down Expand Up @@ -227,42 +228,21 @@ endif()

include(CTest)
if(BUILD_TESTING)

# Can be removed if cmake min version is >=3.24
if (POLICY CMP0135)
cmake_policy(SET CMP0135 NEW)
endif()

find_package(GTest)
if(NOT GTest_FOUND)
include(FetchContent)
FetchContent_Declare(
googletest
URL https://github.com/google/googletest/archive/release-1.10.0.tar.gz
URL_HASH SHA1=9c89be7df9c5e8cb0bc20b3c4b39bf7e82686770
URL https://github.com/google/googletest/archive/refs/tags/release-1.12.1.tar.gz
URL_HASH SHA256=81964fe578e9bd7c94dfdb09c8e4d6e6759e19967e397dbea48d1c10e45d0df2
)
FetchContent_MakeAvailable(googletest)

# Before googletest-1.11.0, the CMake files provided by the source archive
# did not define the GTest::gtest target, only the gtest one, so define
# an alias when needed to ensure the rest of this file works with all
# GoogleTest releases.
#
# Note that surprisingly, this is not needed when using GTEST_ROOT to
# point to a local installation, because this one contains CMake-generated
# files that contain the right target definition, and which will be
# picked up by the find_package(GTest) file above.
#
# This comment and the four lines below can be removed once Ninja only
# depends on release-1.11.0 or above.
if (NOT TARGET GTest::gtest)
message(STATUS "Defining GTest::gtest alias to work-around bug in older release.")
add_library(GTest::gtest ALIAS gtest)

# NOTE: gtest uninit some variables, gcc >= 1.11.3 may cause error on compile.
# Remove this comment and six lines below, once ninja deps gtest-1.11.0 or above.
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "1.11.3")
check_cxx_compiler_flag(-Wmaybe-uninitialized flag_maybe_uninit)
if (flag_maybe_uninit)
target_compile_options(gtest PRIVATE -Wno-maybe-uninitialized)
endif()
endif()

endif()
endif()

# Tests all build into ninja_test executable.
Expand Down Expand Up @@ -291,6 +271,11 @@ if(BUILD_TESTING)
if(WIN32)
target_sources(ninja_test PRIVATE src/includes_normalize_test.cc src/msvc_helper_test.cc
windows/ninja.manifest)

if(MSVC)
# Silence warnings about using unlink rather than _unlink
target_compile_definitions(ninja_test PRIVATE _CRT_NONSTDC_NO_DEPRECATE)
endif()
endif()
find_package(Threads REQUIRED)
target_link_libraries(ninja_test PRIVATE libninja libninja-re2c GTest::gtest Threads::Threads)
Expand Down
Loading