search

rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
199 stars 55 forks source link

Refactor CMake usage to best practices #1542

Open ronaldtse opened 3 years ago

ronaldtse commented 3 years ago

The setup instructions of googletest suggests usage of the CMake FetchContent commands, but we're not doing that right now: https://google.github.io/googletest/quickstart-cmake.html

We would want to take this chance to refactor the CMake configuration and build flow using current best practices.

ronaldtse commented 3 years ago

Ping @thierryseegers

thierryseegers commented 3 years ago

@ronaldtse Am I right in thinking that you'd ultimately like to get rid of as much workarounds/complexity as possible? I'm referring to these (there may be more):

ronaldtse commented 3 years ago

@thierryseegers indeed, the less complexity we have, the easier to maintain. Thanks!

ni4 commented 3 years ago

JFYI: GTEST_SOURCES/DOWNLOAD_GTEST were added in support of offline builds, please see https://github.com/rnpgp/rnp/issues/1410 for discussion.

ronaldtse commented 3 years ago

Thanks @ni4 for the reminder... let's see if we can simplify given this constraint of #1410.

thierryseegers commented 3 years ago

@ronaldtse After a simple conversion of ExternalProject to FetchContent, a chain of annoyances ensued:

  1. When building the target of FetchContent, the current compilation options and flags will apply, in the case of an MSVC build, it picks up /std:c++latest from the root CMakeLists.txt. This fails compilation of @c43f710 specified in the original gtest-CMakeLists.txt.in. It's a single compilation error in a header: std::result_of is now std::invoke_result_t with /std:c++latest.
  2. Curiously, on other platforms, gmock gets built when it appears it wasn't with the old way of doing things. I'm not positive I understand why. That also fails compilation with a single compilation error.

Issue no 2 is circumvented by forcing googletest's BUILD_GMOCK option to OFF.

Issue no 1 is proving more complicated. Upgrading googletest to release-1.11.0 (the latest) fixes the compilation problem for MSVC but brings in a new one on CentOS7. It appears that std::is_trivially_copy_constructible is not part of the libstdc++ found on CentOS7. From a cursory investigation, it looks like /std:c++latest may be needed only for one feature: designated initializers of structs (some examples found in ecdh.cpp,50:57).

Thus the crossroads it appears I'm finding myself at is:

For info, here are my CI results. There's a failure observed for MSVC/x64 but it's a timeout for the cli_tests-Compression test which I'm assuming is spurious.

ronaldtse commented 3 years ago

@thierryseegers thanks for #1547. Could you help with the rest of CMake usage and then CPack? Thanks.