CMake Integration

[waqqas]

Build NanoLog (cpp17) with cmake.

• Added functionality to generate NanoLog.pc file, to easily integrate with other cmake based projects using PkgConfig package
• Refactor: Moved unit test, decompressor and perf code to separate sub-directories
• Refactor: Removed googletest as git sub-module. google-test can be installed using apt-get install googletest

Sample integration with cmake project

find_package(PkgConfig REQUIRED)
pkg_check_modules(NANOLOG REQUIRED NanoLog)
target_link_libraries(main ${NANOLOG_LDFLAGS})
target_include_directories(main PUBLIC ${NANOLOG_INCLUDE_DIRS})
target_compile_options(main PUBLIC ${NANOLOG_CFLAGS_OTHER})

TODO: move preprocessor version to cmake

[syang0]

Hi waqqas,

 Thanks for the contribution 👍 .

Sorry for the late reply, I had to find the time to pull the patch and check it out. Some comments/fixes I'd like before I merge the patch are:

1. Could you write a section in the README on how to compile the project with cmake and how to integrate it with other cmake projects that will use NanoLog as a library?
2. Also for the README, I noticed that you removed the gtest submodule. Can you also conveniently include some instructions for newcomers on how to install/download gtest and point cmake towards it?
3. It seems like cmake generates a whole bunch of files and folders that are not yet .gitignore'd. Could you add them to the .gitignore?

Some other notes I have are:

1. Can you bring the integration tests or Preprocessor NanoLog into the cmake script?
2. Your restructuring of the source directory breaks some of the GNUmakefile scripts. Can you fix them (if not I'll have to fix them before merging your patch)?

Best Regards,
Stephen Yang

[andrewkcorcoran]

This should be marked as PUBLIC not PRIVATE to ensure that non standalone builds get the correct include directories.

[andrewkcorcoran]

We should support people consuming the library directly through CMake rather than through PkcConfig. https://cliutils.gitlab.io/modern-cmake/chapters/install.html

[andrewkcorcoran]

I don't think removing the googletest submodule is the correct way to go. Requiring a manual installation of gtest is just added work and has potential for breaking changes if the user installs a version of gtest that has breaking changes to the one we ran our CI against. The existing solution of using submodules to refer to an exact commit ensures consistency between user experience and our CI builds.

[andrewkcorcoran]

GeneratedCode.cc is not a file that exists in the repo - it has to be generated by the preprocessor script. I don't think this command succeed on a standard check out. @syang0 do you have CI checks enabled on pull requests? I see no checks ran on this PR and I suspect they should be failing.

[syang0]

No, CI checks are only enabled on the main branch. They wouldn't have run anyway for this PR since it substantially changed the build structure and where the executables exist.

[andrewkcorcoran]

Perhaps ci builds should be enabled for pull requests to master so? Looks like there's an option in the GitHub settings for that. It would avoid accidently merging a PR that breaks everyone on matter. This would require this pr to modify the yaml settings to add CMake builds.

…

On Thu, 13 Feb 2020, 6:50 am syang0, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In CMakeLists.txt <#22 (comment)>: > @@ -0,0 +1,50 @@ +cmake_minimum_required(VERSION 3.10) + +project(NanoLog VERSION 0.0.1 DESCRIPTION "Nanolog is an extremely performant nanosecond scale logging system for C++ that exposes a simple printf-like API.") + +list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake/modules") + +set(CMAKE_CXX_STANDARD 17) + +# pre-requisites +find_package(Threads REQUIRED) +find_package(RT REQUIRED) + +file(GLOB HEADERS ${PROJECT_SOURCE_DIR}/runtime/*.h) +file(GLOB SOURCES ${PROJECT_SOURCE_DIR}/runtime/*.cc ${PROJECT_SOURCE_DIR}/runtime/testHelper/GeneratedCode.cc) No, CI checks are only enabled on the main branch. They wouldn't have run anyway for this PR since it substantially changed the build structure and where the executables exist. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#22?email_source=notifications&email_token=AL5LS3E5XI4L4T3ZSPWZ5ODRCSYO7A5CNFSM4KGT6GGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCVKX7PI#discussion_r378625818>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL5LS3AZRXYMXHROWJQYSLDRCSYO7ANCNFSM4KGT6GGA> .