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

Embedding of gtest/gmock in source tree #488

Open
rleigh-codelibre opened this issue Apr 25, 2017 · 6 comments · May be fixed by #1171
Open

Embedding of gtest/gmock in source tree #488

rleigh-codelibre opened this issue Apr 25, 2017 · 6 comments · May be fixed by #1171

Comments

@rleigh-codelibre
Copy link
Contributor

The current approach used by yaml-cpp is to embed gtest/gmock in test/gmock-1.7.0. While this works fine for the most part, it doesn't work well when you're integrating it into a larger build which also includes its own gtest, e.g. a later version such as 1.8.0. It would be nice if this could be made optional. The headers and libraries can conflict.

As a suggestion, this is the approach I took in a different project: GTest.cmake. Here, we pass in a variable GTEST_SOURCE which points to the location of the sources. If unset, we simply use find_package and use an external version. If set, we use the internal version (or the version pointed to outside the source tree). An option could be added to yaml-cpp to do an equivalent action, e.g. build-gtest or build-gmock. It could even be defaulted based upon the result of find_package.

Also worth noting: gtest 1.8 combines gmock and no longer enforces the requirement to "vendor" into a source tree. It's now locally installable again, with the proviso that it needs building with a compatible set of compiler options (so no different than most other libraries!).

Regards,
Roger

@keith-bennett-gbg
Copy link

I had a hell of a time trying to get gtest to play nice with other projects that also do the Bad Thing add_subdirectory(googletest). I spent more than a day fighting this without much luck.

I ended up just building and installing yaml-cpp. I should probably do the same for gtest, but that doesn't solve the issue that more than one repository is doing Bad Things.

@rleigh-codelibre
Copy link
Contributor Author

In an ideal world, no project would do the embedding thing. gtest made this difficult due to its draconian (and IMO unjustified) restrictions on installing. I've switched to using the system gtest or building it as a regular package in a super-build setup. For either case, being able to disable it in each leaf package is generally sufficient to fix things!

@PhilMiller
Copy link

It looks like GTest does have some accommodation for this issue:
https://github.com/google/googletest/blob/main/CMakeLists.txt#L32

@PhilMiller
Copy link

I'll post a PR, I guess

PhilMiller added a commit to PhilMiller/yaml-cpp that referenced this issue Feb 7, 2023
Goole Test itself documents the `INSTALL_GTEST` option as something that projects embedding it should set to `OFF`. Leaving it on means that projects downstream from libraries that embed it are likely to encounter conflicting copies. This is particularly annoying because GTest does not maintain anything like a stable API, and so causes builds to fail if include paths pick up an inappropriately installed copy ahead of the one that the code wanted.

Fixes jbeder#488
@PhilMiller PhilMiller linked a pull request Feb 7, 2023 that will close this issue
@rleigh-codelibre
Copy link
Contributor Author

Since I originally reported the issue nearly six years ago, googletest is now distributed in a directly usable form by most Linux distributions, homebrew, vcpkg etc. I and others did the work to have it properly packaged for all common platforms. So the need to "vendor" it is now greatly reduced. I've ceased to do so in all of my own projects.

Kind regards,
Roger

@PhilMiller
Copy link

Sadly, I'm still in an environment dealing with a lot of projects that do vendor it. I would hope the maintainers consider this worth accepting a fix for.

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 a pull request may close this issue.

3 participants