Tests tweaks #38
Tests tweaks #38
Conversation
There was a problem hiding this comment.
Thanks, LGTM! :)
Unfortunately the value 17 of the property was only added in CMake 3.8, so when using older CMake versions the flag is still passed manually
Yes, this was why I originally went with the target_compile_options approach. However, like mentioned in the comments, I don't think this is necessary any longer.
CMakeLists.txt
Outdated
| # CXX_STANDARD 17 is only available in CMake 3.8 and newer | ||
| if (CMAKE_VERSION VERSION_GREATER "3.7") | ||
| set_target_properties("Test${test}" PROPERTIES CXX_STANDARD 17) | ||
| elseif(NOT MSVC) | ||
| target_compile_options("Test${test}" PRIVATE "-std=c++17") | ||
| endif () |
There was a problem hiding this comment.
Feel free to just raise the minimum required version to 3.8. Even the old-stable versions of both Ubuntu and Debian have newer packages these days, so I don't think we have to support even older stuff any longer.
I'd also assume this to be broken on e.g. CMake 3.7.1, because that would technically be greater than 3.7?
The documentation says:
omitted components are treated as zero
There was a problem hiding this comment.
Feel free to just raise the minimum required version to 3.8
Oh ok, I'll update the PR.
I'd also assume this to be broken on e.g. CMake 3.7.1, because that would technically be greater than 3.7?
Yeah you're right, I missed that
There was a problem hiding this comment.
I think Gtest anyways started complaining about our old CMake version last time I configured/generated from scratch ^^
There was a problem hiding this comment.
I think Gtest anyways started complaining about our old CMake version last time I configured/generated from scratch
That's because GTest 1.10 uses a deprecated minimum required version, see google/googletest#3040 and google/googletest#3523
CMakeLists.txt
Outdated
| # CMake 3.20 and upstream GTestConfig.cmake | ||
| if (TARGET GTest::gtest) | ||
| add_library(gtest ALIAS GTest::gtest) | ||
| # CMake 3.5 and newer | ||
| elseif (TARGET GTest::GTest) | ||
| add_library(gtest ALIAS GTest::GTest) | ||
| # Older CMake versions | ||
| else () | ||
| add_library(gtest INTERFACE) | ||
| find_package(Threads REQUIRED) | ||
| target_include_directories(gtest INTERFACE "${GTEST_INCLUDE_DIRS}") | ||
| target_link_libraries(gtest INTERFACE "${GTEST_LIBRARIES}" Threads::Threads) | ||
| endif () |
There was a problem hiding this comment.
Like with the other comment, feel free to just raise the required CMake version. The oldest non-EOL Ubuntu version (18.04) has CMake 3.10.2, oldest non-EOL Debian (buster) has 3.13.4. Since these two distros are typically the slowest to upgrade, it's pretty safe to assume that all other important ones have newer versions already. I'm not particularly concerned with the commercial extended support releases.
This helps with automated testing, as tests can be ran with `ctest` or by "building" the target `test` I also changed the way the C++ standard version is handled; the tests require C++17, and this was previously accomplished by manually passing the `-std=c++17` flag, while the "CMake way" of doing this is to set the `CXX_STANDARD` property. Unfortunately the value `17` of the property was only added in CMake 3.8, so I bumped the minimum required version. I've also set the `CXX_EXTENSIONS` and `CXX_STANDARD_REQUIRED` to false and true, respectively, to ensure that `-std=c++17` gets passed instead of `-std=gnu++17` on GCC-like compilers and that CMake doesn't fallback to older standards versions when the requested standard is not available
This reduces compile times and allows tests to be ran in environments without internet access (e.g. when building a Debian package). CMake will look for an installed copy of GoogleTest on the system, and will use it if found, and will fallback to download and unpack it otherwise.
Tachi107
force-pushed
the
tests-tweaks
branch
from
5414e03
to
dcf47a7
Compare
Using -pthread is the "right way" of enabling threading support in GCC and similar, and this can accomplished with the THREADS_PREFER_PTHREAD_FLAG option. Its use is also recommended by the CMake devs, see https://cmake.org/cmake/help/latest/module/FindThreads.html#variable:THREADS_PREFER_PTHREAD_FLAG
Tachi107
force-pushed
the
tests-tweaks
branch
from
dcf47a7
to
c3d7e5e
Compare
The changes are not that big, but I decided to split them in multiple commits because every one of them does a different thing. They are all related though, so I prepared a single PR. Here's their description (from the commit messages):
test: make tests runnable with ctest
This helps with automated testing, as tests can be ran with
ctestor by "building" the targettestI also changed the way the C++ standard version is handled; the tests require C++17, and this was previously accomplished by manually passing the
-std=c++17flag, while the "CMake way" of doing this is to set theCXX_STANDARDproperty. Unfortunately the value17of the property was only added in CMake 3.8, so when using older CMake versions the flag is still passed manually. I've also set theCXX_EXTENSIONSandCXX_STANDARD_REQUIREDto false and true, respectively, to ensure that-std=c++17gets passed instead of-std=gnu++17on GCC-like compilers and that CMake doesn't fallback to older standards versions when the requested standard is not available.build: use system GTest when available
This reduces compile times and allows tests to be ran in environments without internet access (e.g. when building a Debian package).
CMake will look for an installed copy of GoogleTest on the system, and will use it if found, and will fallback to download and unpack it otherwise.
There are a lot of checks involved since CMake changed the way FindGTest works in newer CMake versions, and they are needed to ensure 3.1 compatibility.
build: use -pthread when possible
Using -pthread is the "right way" of enabling threading support in GCC and similar, and this can accomplished with the THREADS_PREFER_PTHREAD_FLAG option. Its use is also recommended by the CMake devs, see cmake.org/cmake/help/latest/module/FindThreads.html
More words than code changes :)