Add CMake configuration file and export installed targets

[accelerated]

Description

CMake improvements which aim at modernizing and simplifying the use of CppKafka and the discover-ability of installed files when linked by other projects. This also prevents from having to manually add dependent libraries and include directories.

• Update CMake minimum version to 3.9.2. Ideally this could be bumped up to 3.15. This version is now more than 1yr old.
• Added the GNUInstallDirs functionality proposed by @azat! Thanks for the improvement! The PR#200 is no longer needed.
• Make use of RdKafka CMake config file by calling the default find_package(RdKafka). This loads in all the exported targets, dependencies, include directories which are needed by this project. By doing this we no longer need a specific FindRdKafka.cmake module as well as manually specifying include directories and lib paths to RdKafka library. This is handled seamlessly by the exported targets. The only thing that needs to be done is adding RdKafka::rdkafka as a dependency to the cppkafka target.
  If RdKafka is installed in a non-standard directory, RDKAFKA_ROOT or CMAKE_INSTALL_PREFIX can be defined to point to it.
• Made use of the RdKafka installed version file to specify exactly the minimum version required via find_package(RdKafka "0.9.4" REQUIRED). This prevents depending on a non-compatible version of RdKafka.
• Similar to RdKafka, generate a config file, an exported targets file and a version file for CppKafka. This allows other projects to call find_package(CppKafka "0.3.1") for instance and link against it by using CppKafka::cppkafka as a dependency. Furthermore because of the transitive nature of exported targets, when including CppKafka::cppkafka all its dependencies are automatically included as well: RdKafka::rdkafka (and it's dependencies such as libssl, libuv, libz, etc...) and Boost::boost (optional.hpp header).
• The following CMake variables are also exported by this project: CppKafka_ROOT, CppKafka_INSTALL_INCLUDE_DIR and CppKafka_INSTALL_LIB_DIR.
• Moved all CMake template files under the cppkafka/cmake directory so that everything is in the same directory. The auto-generated files from these templates are stored under the temporary cppkafka/package directory prior to installation.
• Updated tests and examples targets to use the RdKafka::rdkafka dependency instead.
• CPPKAFKA_CONFIG_DIR Cmake option can allow installing the CppKafka config files elsewhere.
• Removed include/CMakeLists.txt since it was only a redirect to the directory below it. Instead we call add_subdirectory(include/cppkafka) directly.
• The Cmake option RDKAFKA_ROOT_DIR has been superseded by RDKAFKA_ROOT. The previous option is still used for backwards compatibility.

[accelerated]

Hi @azat, can you please try this branch and see that it fixes all your issues? Thanks!

[azat]

Looks like all needable bits are there, so I'm fine with it (I cannot test it right now, since I do not have neither of RdKafkaConfig.cmake/rdkafka-config.cmake in my librdkafka package, will take into this later)

[accelerated]

Looks like all needable bits are there, so I'm fine with it (I cannot test it right now, since I do not have neither of RdKafkaConfig.cmake/rdkafka-config.cmake in my librdkafka package, will take into this later)

You can simply build rdkafka with cmake. This will ensure that both files are generated and installed correctly. See included travis.yml file. Also I belive the default behavior for find_package() is to search for the RdKafkaConfig.cmake file and then fall back on module-search mode, in which it searches for the libraries themselves using RDKAFKA_ROOT or CMAKE_INSTALL_PREFIX.

[mfontanini]

I'll have a look at this over the weekend but regarding requiring to use an rdkafka version built using CMake, I don't think we should go that way. The rdkafka CMake build system is not maintained by the autor but by the community instead (e.g. read their README which states "NOTE: See CMake instructions for experimental CMake build (unsupported)"). We should have our own find script so we can ensure we find it no matter which build system it was built with.

[accelerated]

@mfontanini Agreed, however the cmake way is pretty much the future. The only thing which I haven't thought about is if you install the DPKG from some distro, they don't seem to include the CMAKE target files. So I think that the best way would be to keep your FindRdKafka.cmake script and use a CMAKE flag to either discover using RdKafkaConfig.cmake or your file

[azat]

You can simply build rdkafka with cmake

I can, but I cannot force others to do so, take a look at debian for instance

The default find_package should work even if the cmake config files are not there. I will do more testing to be sure.

How? find_package() requires {name}-config.cmake/{name}Config.cmake at proper locations.

We should have our own find script so we can ensure we find it no matter which build system it was built with

+1, otherwise this will existing setups

[accelerated]

BTW, the FindRdKafka.cmake can also be exported in the cmake/CppKafka directory along with the other cmake targets. So that an application using CppKafka can make use of it.

[accelerated]

Ok the cmake behavior was changed to the following:

• find_package() will first search for any installed RdKafkaConfig.cmake files. If it's not found it will search for module RdKafka.a/.so which is the current behavior.
• The Cmake will also install the FindRdKafka.cmake module so that it can be used by programs depending on CppKafka.
• Setting RDKAFKA_ROOT, RDKAFKA_ROOT_DIR or RDKAFKA_DIR has to be an absolute path or a relative path from the top-level cmake file. I believe the current setup in Travis.yml happend to work by coincidence since the librdkafka library was installed under the top-level cmake. Because this path is used as a HINT inside ./cmake/FindRdKafka.cmake, having a relative path from the cmake invocation point simply cannot work.

[azat]

There is one cmake warning (in 3.14.5)

CMake Warning (dev) at CMakeLists.txt:118 (find_package):
  Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
  Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.

  CMake variable RdKafka_ROOT is set to:

    /usr

  For compatibility, CMake is ignoring the variable.

Apart from that everything is works, thanks!

[accelerated]

Policy warning fixed.

[mfontanini]

Is this needed? I think I've used -Dsomething=../../x and it works IIRC.

[accelerated]

It didn't for me. Seems that find_package() expects a proper relative path from the point where it's being evaluated. In this case it was evaluated from cppkafka/cmake which made Travis fail. The second thing is that when using the CppkafkaConfig.cmake, which in turn calls FindRdKafka.cmake , you have no control of where it will be accessed from. This is meant to be used by 3rd party libs linking CppKafka. So FindRdKafka.cmake should have a proper path to the RdKafka root.

[mfontanini]

The include and library path shouldn't be polluted by including this script. Since this is exporting a target for rdkafka, you should be using target-specific options (e.g. target_include_directories)

[accelerated]

Hmmm but this script is used by CppKafka and also by any 3rd party library linking to CppKafka and using the CppKafkaConfig.cmake. So which target are we adding the include directories for? You can't say target_include_directories(RdKafka...) since we're not building this target, we are just importing it. Maybe I can do add_library(RdKafka IMPORTED) and then set the properties on the imported target. This is not something I have tried....will try and let you know.

[mfontanini]

We've packaged a bunch of libraries at work using conan + CMake and we normally have the Find*.cmake script create an imported target and add all dependencies in there. This works great and lets you have all dependencies defined per target rather than having to remember to link with your dependencies' dependencies. We do something like

add_library(foo INTERFACE IMPORTED)
set_target_properties(foo PROPERTIES
    INTERFACE_INCLUDE_DIRECTORIES ${FOO_INCLUDE_DIRS}
    INTERFACE_LINK_LIBRARIES "${FOO_LIBRARY};dep1;dep2;..."
)

Then you just import the library and link to it, ignoring dependencies (e.g. via find_package and target_link_libraries).

[accelerated]

Yeah this is what I just did. See my commit.

[mfontanini]

Following up on the comment in the top level CMakeLists.txt file, this shouldn't be here either unless there's an error.

[accelerated]

OK...see my previous comment. Since the find_package searches config and module, it's good for debugging to know what's going on. Maybe I can put this under CMAKE_VERBOSE_MAKEFILE or something like that?

[mfontanini]

If you want it for debugging purposes, this probably shouldn't be in this file but in the main CMake file instead (under some flag). A user importing cppkafka on their project (via this script) shouldn't care at all about how rdkafka is being found: they just care about the cppkafka target either being there or not after running the script.

[accelerated]

Ok I can leave it outside the CppKafkaConfig.cmake and only include it in the top CMakeLists of the CppKafka project itself. In that case you do want to know why you can't find RdKafka.

[accelerated]

BTW the REQUIRE makes sure that an error message will be printed if the module is not found.

[accelerated]

@mfontanini this feature is pretty much complete now. Lmk if there's anything else you want me to look at. I can squash the commits into a single one as there are too many.

[accelerated]

Hi @mfontanini, can you please take a look when you get a chance? Thanks!

[mfontanini]

Thanks for all this great work @accelerated ! Any other concern? I see you just pushed something. I tried it locally and everything seems to work fine.

[accelerated]

@mfontanini at this point it should be all good. If there are any issues they will be fixed pretty soon since we're making heavy use of all these cmake features for internal builds, etc. My only question was if you want to squash all this into a single commit. There's a lot of noise from all the back and forth.

[mfontanini]

Nah, don't worry about it. At least this way we know things changed progressively towards the state it's in now. Merging this now, let me know if there's any other problems. Sorry it took me a while, there's just so many issue emails coming in on this and another library I maintain that I just feel a bit overwhelmed some days and don't even get to open them.