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.

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

The CMake Build documentation is too detached from reality #4681

Open
Mq-b opened this issue Dec 14, 2024 · 6 comments
Open

The CMake Build documentation is too detached from reality #4681

Mq-b opened this issue Dec 14, 2024 · 6 comments

Comments

@Mq-b
Copy link

Mq-b commented Dec 14, 2024

Does the feature exist in the most recent commit?

No.

Why do we need this feature?

I don't think anyone built and imported the GTest testing framework the way the documentation did:

cmake_minimum_required(VERSION 3.14)
project(my_project)

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

include(FetchContent)
FetchContent_Declare(
  googletest
  URL https://github.com/google/googletest/archive/03597a01ee50ed33e9dfd640b249b4be3799d395.zip
)
# For Windows: Prevent overriding the parent project's compiler/linker settings
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(googletest)

Describe the proposal.

I find this confusing, in fact I think what we should be writing is building from source and importing it using CMake:

find_package(GTest REQUIRED)
target_link_libraries(${PROJECT_NAME} PRIVATE GTest::GTest)

Also, I think "gtest_force_shared_crt" should be defined during the configuration step (e.g., cmake .. -Dgtest_force_shared_crt=ON), when generating the build system, rather than during the build or usage phase.

Let me repeat, I think our documentation should be written to build from source, like:

git clone https://github.com/google/googletest
cd googletest
mkdir build
cd build
cmake .. -Dgtest_force_shared_crt=ON
cmake --build . -j
cmake --install .

I forgot to add -Dgtest_force_shared_crt=ON earlier which caused me to use MT to link to the C++ standard library and keep getting errors.

If it's on Windows, it is also necessary to generate the Release version and install it.

Is the feature specific to an operating system, compiler, or build system version?

No.

@phetdam
Copy link

phetdam commented Dec 16, 2024

I vaguely remember the Google Test CMake quickstart not having the FetchContent stuff a while ago (1.10.0 was the latest release at the time). At that time, I think find_package was in the CMake quickstart, but I checked the quickstart-cmake.md commit history and can't find any historical record. Not sure if this is due to repo merging with Google Mock.

But as you mentioned, on Windows it's common to want both the Debug and Release configs, both of them linking to their respective shared C runtime libraries. To have Debug and Release libraries in the same installation root, the debug libs need a d or other suffix; as noted in #3843 this was removed (probably for the benefit of single-config CMake generators).

Until the documentation is updated this is how I build on Windows (64-bit, shared C runtimes, d suffix for debug libs). Assuming Google Test repo has already been cloned and checked out to a specific commit/tag, from top-level:

cmake -S . -B build_win -A x64 -Dgtest_force_shared_crt=ON -DCMAKE_DEBUG_POSTFIX=d
cmake --build build_win --config Debug -j
cmake --build build_win --config Release -j

Both configs can then be installed into whichever install root is preferred, e.g.

cmake --install build_win --prefix ..\googletest-1.15.0 --config Debug
cmake --install build_win --prefix ..\googletest-1.15.0 --config Release

This gives the all the expected Google Test and Google Mock debug and release static libraries.

Of course, from CMake, once GTEST_ROOT is set (in env or, as a CMake cache variable), then the normal stuff works:

find_package(GTest 1.10.0 REQUIRED)

@Mq-b
Copy link
Author

Mq-b commented Dec 16, 2024

Thank you, I have omitted some of my actual builds, it's roughly indeed executing two install and two build to ensure that both Debug and Release versions are built and installed.

@Mq-b
Copy link
Author

Mq-b commented Dec 16, 2024

I am indeed on a Windows environment, but I believe that whether it's Linux or Windows, we should prioritize using find_package and target_link_libraries to include.

@phetdam
Copy link

phetdam commented Dec 16, 2024

I am indeed on a Windows environment, but I believe that whether it's Linux or Windows, we should prioritize using find_package and target_link_libraries to include.

Yes, this is more meaningful on Linux (or similar) environment where often tagged Google Test versions are installed via the system package manager. In this case, it makes more sense to use find_package instead of FetchContent.

When I have some time I think I will try to make a PR with some updates to the documentation to highlight these other common methods of Google Test integration with CMake. Otherwise, others might raise similar issues.

@Mq-b
Copy link
Author

Mq-b commented Dec 16, 2024

Yes, you have exactly the same idea as me, I also define a CMAKE_PREFIX_PATH variable in windows to facilitate the introduction of tripartite libraries. I build all the libraries from source code.

https://github.com/Mq-b/CXX_LIB

@phetdam
Copy link

phetdam commented Dec 16, 2024

I might open a new issue since I am considering adding a new page in the "Guides" section for building from source, as I showed in my previous comment, as well as updating the CMake quickstart page to also include find_package usage with separately installed Google Test builds. Will reference this issue to provide additional context.

Edit: May not necessarily create a new issue but I definitely think build from source instructions should go on a new page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants