Improve the Fetch gtest workflow docs#140
Conversation
Remove the extra variable set.
Raise the visibility for the common case of not having a packaged googletest available. Presets remain an open issue.
steve-downey
requested review from
bretbrownjr,
camio,
dietmarkuehl,
neatudarius and
wusatosi
as code owners
bretbrownjr
left a comment
bretbrownjr
left a comment
There was a problem hiding this comment.
No objections from me. Let's get at least one other reviewer to take a look, though. I am not the target audience for the docs. Of course they make sense to me!
There was a problem hiding this comment.
I feel like having this documentation is better, but the fact that we can no longer use presets for FetchContent workflows has bit me and I expect will bite many others.
The default/simplest thing should be something that we know works (e.g. FetchContent with known versions). Using find_package and hoping the system provides something suitable is not reliable at all.
|
|
||
| ```shell | ||
| cmake -B build -S . \ | ||
| -DCMAKE_TOOLCHAIN_FILE=./cmake/gnu-toolchain.cmake \ |
There was a problem hiding this comment.
should we include the gnu toolchain here?
There was a problem hiding this comment.
The toolchain files are not designed with expectation to be use directly.
And gnu-toolchain heavily assumes the use of gnu compilers (it sets CMAKE_CXX_COMPILER to g++).
Right now direct use like this will only set CMAKE_CXX_COMPILER, maybe it would be a better idea to omit it for now.
There was a problem hiding this comment.
Without those toolchain files, there's no access to the sanitizer configurations we're looking at.
Of course since they're also only applied in the DEBUG configuration they aren't as likely to catch a significant number of problems when the compiler takes advantage of UB, hoists expressions, elides or inlines calls, and so forth.
Pulling the line out, though, because it's a larger discussion.
Remove the CMAKE_TOOLCHAIN_FILE in an example, let it rely on finding a system compiler and using that.
wusatosi
left a comment
wusatosi
left a comment
There was a problem hiding this comment.
Concern addressed.
Of course since they're also only applied in the DEBUG configuration they aren't as likely to catch a significant number of problems when the compiler takes advantage of UB, hoists expressions, elides or inlines calls, and so forth.
Pulling the line out, though, because it's a larger discussion.
I agree, I think we need to redesign our presets after the packaging stabilizes.
5 participants
Pull the documentation of how to fetch GTest from github out of the details block and higher into the docs. This is will be a common workflow and should be easier to find.
Also fix a nit in the cmake module where GTest_FOUND is set for any package that is pulled in via the module.