Remove AVIF_ENABLE_GTEST cmake option. #2527
Conversation
CMakeLists.txt
Outdated
| ) | ||
| if(NOT AVIF_GTEST STREQUAL "OFF") | ||
| set(AVIF_ENABLE_GTEST ON) |
There was a problem hiding this comment.
no need to define an extra variable anymore: just replace the two instances of it below with AVIF_GTEST. Same in tests/CMakeLists.txt
There was a problem hiding this comment.
I though it was easier like this (and fewer changes) than checking NOT AVIF_GTEST STREQUAL "OFF" everywhere, no? Or does if(AVIF_GTEST) work? (I didn't think so since it's a string). I'm happy to change it though if you think it's better.
There was a problem hiding this comment.
Hmm, I thought it would work with "OFF", https://cmake.org/cmake/help/latest/command/if.html#constant but maybe you can try and see if it works. If not, if it is equal to "OFF", just set it to OFF
There was a problem hiding this comment.
That does seem to work. TIL!
| `AVIF_ENABLE_GTEST` CMake options. They require GoogleTest | ||
| (`-DAVIF_GTEST=SYSTEM` or `-DAVIF_GTEST=LOCAL`). | ||
| The remaining tests can be built by enabling `AVIF_BUILD_TESTS` and setting | ||
| `AVIF_GTEST` to `SYSTEM` or `LOCAL`. |
There was a problem hiding this comment.
Optional: We can consider keeping the comment "They require GoogleTest."
| if(AVIF_LIB_USE_CXX OR AVIF_BUILD_APPS OR (AVIF_BUILD_TESTS AND (AVIF_ENABLE_FUZZTEST | ||
| OR AVIF_ENABLE_GTEST)) | ||
| ) | ||
| if(AVIF_LIB_USE_CXX OR AVIF_BUILD_APPS OR (AVIF_BUILD_TESTS AND (AVIF_ENABLE_FUZZTEST OR AVIF_GTEST))) |
There was a problem hiding this comment.
Do you think AVIF_ENABLE_FUZZTEST can receive the same treatment?
There was a problem hiding this comment.
Indeed, we talked about it but I wanted to add more stuff at the same time.
I fixed it in #2529 (and fuzztest is now downloadable from CMake just like any other dependency)
There is no need to have two AVIF_ENABLE_GTEST and AVIF_GTEST options.
Now setting AVIF_GTEST=LOCAL or AVIF_GTEST=SYSTEM automatically turns ont gtest tests.