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

build(cmake): Use FindGTest if possible #95

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Conversation

xatier
Copy link
Contributor

@xatier xatier commented Dec 11, 2023

Utilize FindGTest to locate googletest if installed on the system. If googletest is not installed, fetch the specified version in src/CMakeLists.txt

Also remove redundant FetchContent blocks in subdirectories.

@xatier
Copy link
Contributor Author

xatier commented Dec 11, 2023

@lukhnos this is the follow-up from #94

There are actually some benefits from building googletest with tarball, listed here:
https://github.com/google/googletest/tree/main/googletest#incorporating-into-an-existing-cmake-project

Please let me know which approach you'd prefer.

@xatier
Copy link
Contributor Author

xatier commented Dec 11, 2023

I didn't notice any difference by using the system package (https://archlinux.org/packages/extra/x86_64/gtest/) vs the tarball, they are both v1.14.0 anyway. Due to the CMake issue I noted in README, I didn't get a chance to test this on Ubuntu. Perhaps we'll see in future Ubuntu releases.

Copy link
Collaborator

@lukhnos lukhnos left a comment

Choose a reason for hiding this comment

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

Thank you! Just one small nit re documentation.

Re:

There are actually some benefits from building googletest with tarball, listed here:
https://github.com/google/googletest/tree/main/googletest#incorporating-into-an-existing-cmake-project

I actually like your current approach, which uses a tagged version. Also just for my own edification, what are the benefits of using a tarball? I can't seem to find that in the gtest page above.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@xatier
Copy link
Contributor Author

xatier commented Dec 14, 2023

@lukhnos apologies for the delayed response. I've been a bit busy recently.

The main benefits of fetching CMake into the build directory instead of using the system installed version is that we can ensure googletest suite is built with the same tool chain as the project.

This has the significant advantage that the same compiler and linker settings are used between GoogleTest and the rest of your project, so issues associated with using incompatible libraries (eg debug/release), etc. are avoided.

Another benefit is that we may use a slightly newer version of googletest (v.1.14), vs the system package libgtest-dev_1.11.0-3_amd64.deb.

Utilize FindGTest to locate googletest if installed on the system. If
googletest is not installed, fetch the specified version in
src/CMakeLists.txt

Also remove redundant FetchContent blocks in subdirectories.
@xatier
Copy link
Contributor Author

xatier commented Dec 14, 2023

^ rebased onto upstream/master.

@xatier xatier requested a review from lukhnos December 14, 2023 14:38
@lukhnos
Copy link
Collaborator

lukhnos commented Dec 15, 2023

The main benefits of fetching CMake into the build directory instead of using the system installed version is that we can ensure googletest suite is built with the same tool chain as the project.

You mean gtest I assume. Now I see your point. At any rate, this is a relatively small and straightforward project, and as a result I feel that we should be as toolchain agnostic as possible (gcc vs clang, gtest shipped with dist vs fetched-into-build, and so on). So I think this PR is good to go.

@lukhnos lukhnos merged commit 2bc1b0a into openvanilla:master Dec 15, 2023
3 checks passed
@xatier
Copy link
Contributor Author

xatier commented Dec 15, 2023

yeah, I meant gtest, morning coffee didn't kick in just yet... ☕

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

Successfully merging this pull request may close these issues.

2 participants