-
Notifications
You must be signed in to change notification settings - Fork 756
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
Add CMake configuration for test/googletest and test/unit #1034
Conversation
So, CMake downloads a copy of googletest when configuring, yes? Which means we are likely to be using different version across autotools and CMake. What are the chances that'll go wrong? |
I see a Unrelated the clang builds fail with
I believe the -Wmissing-field-initializers flag is an expansion from the -Wextra flag added to CMAKE_CXX_FLAGS by our main CMakeList.txt. Perhaps that the compilation of gtest shouldn't use those hardened flags ? (I had to do similar things for the automake build) |
For CMake builds, latest googletest 1.8.0 is downloaded. Following officially recommended integration for CMake-enabled projects https://github.com/google/googletest/blob/master/googletest/README.md "Use CMake to download GoogleTest as part of the build's configure step. This is just a little more complex, but doesn't have the limitations of the other methods." Since, our copy of test/googletest - is a very minimalist copy of googletest - does not include any official CMake scripts - would require copying parts of googletest CMakeLists.txt, compilater/linker flags (e.g. -lpthreads) for reliable multi-platform builds, it is reasoanable to rely on download All pros and cons advantages are discussed in teh README.md linked above. Closes #1033
I have updated the PR with the following:
The issue is, that our CMake configuration is written in old-school fashion and is behaving badly - it modifies global CMake flags. The current/modern CMake recommendation is to never do that. Instead, we should set any compiler/linker flags/defines per target, not globally or per directory.
Obivously, I can't speak for googletest, but I wouldn't expect a terrible breakage between releases. Perhaps @schwehr could tell more. However, I've pinpointed the download at googletest 1.8.0. This should ensure googletest 'constness'. @rouault
|
Okay, cool. I still don't really understand what the advantage of downloading googletest is when we already have it in the repo. Care to elaborate? |
I hoped the reference to/quote from googletest README will explain. Actually, we don't have it or we have it partially. We have minimal set of sources required to compile googletest library, but we don't have any googletest's This is explained in https://github.com/google/googletest/blob/master/googletest/README.md#incorporating-into-an-existing-cmake-project BTW, the download happens once, during CMake step. The (only?) disadvantage is that it requires internet connection. AFAIU, GNU autotools is still the main build configuration, so it shouldn't be a major issue, I guess. |
Everything green now. I think this is good enough to be merged. Thanks @mloskot ! I didn't initially import the whole googletest archive to avoid littering too much proj code base with it, but that's something we could revise if needed. Anyway I think the download approach is OK for now. |
Sorry, I read it but didn't understand it. Probably just me that is daft. Your explanation here makes a lot more sense to me. Thanks. Seems like an okay approach to me. |
For CMake builds, latest googletest 1.8.0 is downloaded.
Following officially recommended integration for CMake-enabled projects
https://github.com/google/googletest/blob/master/googletest/README.md
Since, our copy of test/googletest
for reliable multi-platform builds, it is reasoanable to rely on download
All pros and cons advantages are discussed in teh README.md linked above.
Closes #1033
Pity GNU autotools does not support the download mode.
Sample session to build and run proj, googletest, and proj tests