googletest icon indicating copy to clipboard operation
googletest copied to clipboard

Documentation: incosistency in Quickstart Building with CMake

Open shinfd opened this issue 3 years ago • 1 comments

Describe the bug

docs/quickstart-cmake.md contains below text:

In the above example, 609281088cfefc76f9d0ce82e1ff6c30cc3591e5 is the Git commit hash of the GoogleTest version to use; we recommend updating the hash often to point to the latest version.

However the hash is not mentioned in the example. Below is the source.

https://github.com/google/googletest/blob/78aa2ba4401af0832edcc0d29f229a50ec8b86e5/docs/quickstart-cmake.md?plain=1#L58-L72

Steps to reproduce the bug / Does the bug persist in the most recent commit? / What operating system and version are you using? / What compiler and version are you using? / What build system are you using?

Not applicable

Additional context

PR #3787 changed FetchContent to use GIT_TAG with release tag instead of URL field with zip archive, but left the succeeding text unchanged. Modifying the text accordingly to refer to the matching tag (in this case release-1.11.0) instead of hash will fix the discrepancy.

However I am not sure if this is the best approach. The said PR addressed the issue of

the CMake contained in the zip file provided to FetchContent_Declare does not provide GTest::gtest_main

by changing the documentation to direct the reader to FetchContent via git clone. This has more overhead than fetching via zip artifact. Also, using archive URL had kept the CMake document in line with Bazel setup. Hence it may be better to address the original CMake issue before re-tweaking the documentation.

shinfd avatar Sep 05 '22 12:09 shinfd

Further research showed that issue of missing GTest::gtest_main may have been resolved by #3155.

I used below sample:

cmake_minimum_required(VERSION 3.14)
project(my_project)

# GoogleTest requires at least C++14
set(CMAKE_CXX_STANDARD 14)

include(FetchContent)
FetchContent_Declare(
  googletest
  URL https://github.com/google/googletest/archive/58d77fa8070e8cec2dc1ed015d66b454c8d78850.zip # release-1.12.1
  # URL https://github.com/google/googletest/archive/6c5c4554ac218a8e19168edc121b1ad232015185.zip # commit merging PR 3155
  # URL https://github.com/google/googletest/archive/3ff1e8b98a3d1d3abc24a5bacb7651c9b32faedd.zip # commit prior to PR 3155 merge
)

# For Windows: Prevent overriding the parent project's compiler/linker settings
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(googletest)

enable_testing()

add_executable(
  mytest
  test.cpp
)
target_link_libraries(
  mytest
  GTest::gtest_main
)

include(GoogleTest)
gtest_discover_tests(mytest)

CMake successfully runs with newer builds, but switching back to pre-3155 archive shows Target "mytest" links to: GTest::gtest_main but the target was not found..

If this check correctly replicated the issue reported in #3787, it may be better to revert the document back to the original FetchContent_Declare using URL, but with address referring to more recent commit hash.

shinfd avatar Sep 05 '22 12:09 shinfd