signalapp / libsignal-protocol-c

GNU General Public License v3.0
1.41k stars 295 forks source link

Refactor CMake files to use (mostly) best practices #132

Closed bruxisma closed 3 years ago

bruxisma commented 4 years ago

Hello! I apologize for the large commit, and I understand if not all it should be merged in all at once, however these CMake files themselves needed quite a bit of work. This was, quite frankly, the most impactful changes I could make that were also less intrusive. I'd like to help improve these further, but wanted feedback on my current set of changes.

The following changes have been made

1) GNUInstallDirs is now used to set locations. The ones of note are:

The above values replace the previously set cache variables that also doubled as options.

2) find_package(signal-protocol) will now work if installed directly. This makes it more viable to build this on Windows with MinGW.

3) All warning flags are checked now, as some newer versions of clang have valid warnings intended for GCC and some newer versions of GCC have valid warnings intended for Clang. Additionally, the old way didn't know how to deal with using 'clang-cl', which is an MSVC compatible driver for Clang.

4) CMake's minimum version was bumped to 3.4. This is a good minimum to have for truly "modern" CMake, and is available on Linux distributions still receiving security updates. If someone is on an older system, they can easily install a recent version via PyPI (i.e., pip install) or grab a .sh installer directly from Kitware's GitHub Mirror. Kitware goes to great lengths to ensure that binary compatibility with older operating systems and expecting no longer supported operating systems to be able to build this library without having to modify some packages or install a single dependency is I think too much of a burden on the maintainers. (Additionally, the CMake minimum was set to 2.8.4, whereas the last CMake release of 2.8 was 2.8.12)

5) An ALIAS library was added with the name signal::protocol. This is also the name of the exported target via find_package. 6) This PR does not yet tackle improving the unit test situation. That will follow in a soon to be finished commit. 7) The HAVE_SECUREZEROMEMORY check is still performed, however it wasn't being used. Adding this as a compile definition is quite simple, and a followup commit will attempt to add it.

Lastly, using add_compile_options over target_compile_options isn't ideal, however, it is much better than manually modifying CMAKE_C_FLAGS.

bruxisma commented 4 years ago

(As an aside, I have signed the CLA. Unsure why it didn't validate :/)

tcyrus commented 4 years ago

@slurps-mad-rips It validates now