Fixes a bug where includes are not visible to parent projects when rs_driver is submoduled.

[stephendb]

Howdy, couple line change to fix a bug with sub-moduling this library.

add_library(rs_driver INTERFACE) makes the project a header only library so that target_include_directories properly get forwarded to parent projects. Without this parent projects will fail to find the headers when used as submodules (particularly useful for windows users).

Also changed the lidar params from private to public as these are very useful to get param settings for user.

[HaoQChen]

Hi stephend, we have test as

when release last version and it work.

So could you give us some information to test about it?

1. which system do you use
2. and could you show us your file tree
3. show us your parent cmakelists

as for the public VS private, we have discussion about it. In our opinion, this parameter only need for decoder, it is little-used for user. Could you tell us what do you use it for? We will discuss it later

thank for your issue~~

[stephendb]

Hi Hao, include_directories(${rs_driver_INCLUDE_DIRS}) ONLY includes rs_drivers include directories and not it's dependencies, i.e., Boost, PCAP. Cloning your project as a submodule and then including 3.2 in CMakeLists.txt causes cmake to fail to find the boost and pcap header files. This is because your project doesn't add the dependencies as target_includes.

Setting your project as interface with the target_includes and then things work in other projects.

1. which system do you use
   Windows

2. Very simple
   

Parent CMakeLists.txt:
cmake_minimum_required(VERSION 3.10)

project(m1)

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

add_executable(parse_to_csv parse_to_csv.cpp)

add_subdirectory(rs_driver)
find_package(rs_driver REQUIRED)
include_directories(${rs_driver_INCLUDE_DIRS})
target_link_libraries(parse_to_csv ${rs_driver_LIBRARIES})
target_link_libraries(parse_to_csv rs_driver)

Notice the addition of target_link_libraries pointed at your project which pulls in the dependency headers.

Well I'm an end user and I'm using these params :) The params are convenient for end user because it has information about the different lidar models for the end users programs. MSOP_ID is useful for packet recognition, and things like LASER_NUM and CHANNELS_PER_BLOCK are useful for users own parsing code.

[HaoQChen]

I have a discussion about this with my partners, and here is our opinion

1. Consider some users will use a higher version of Eigen or Boost for some new features, which our driver does not support. So in our opinion, it is better to separate rs_driver's dependence from main dependence. So you may need to write your own dependence in parent CMakeLists.txt if you want to use them in parse_to_csv.cpp
2. We will change private to public for some user who want advanced development next release.

Thanks for your suggestion again, must be a very careful person

[stephendb]

Thanks for the response Hao. I think there is a misunderstanding behind what the INTERFACE tag does and why ALL header only CMake projects have it.

End user is still able to use a different version of Boost and Eigen with this setup. In fact Eigen/Boost are not marked in your CMakeLists.txt with a specified version. For example I am using latest Boost and Eigen which were already installed before trying out the rs_drivers. Typically you would want to set a minimum version, surprised this was missing.

It is industry practice to declare your header only library an INTERFACE in CMake. I would implore you to check out ANY popular header only library:
https://github.com/nlohmann/json/blob/14be8c69fea01db6d225ebefdffc14b184cac297/CMakeLists.txt#L70
http://mariobadr.com/creating-a-header-only-library-with-cmake.html
https://stackoverflow.com/questions/60604249/how-to-make-a-header-only-library-with-cmake
https://dominikberner.ch/cmake-interface-lib/

The point of that line is to specify rs_drivers project dependencies. Right now your target is effectively missing it's dependencies which is incorrect.

Hope you found that helpful!

[kvern-code]

Hi all,
I am experiencing the problem that Stephen has detailed. My main file is failing to find the headers when using rs_driver as a submodule. Here is my file tree, cmake, main file (lidar example given) and build output. I am on Ubuntu 20.04

CMakeLists.txt
test.txt

I have implemented Stephen's commit and the problem is still present. It is clear from the output that the test.cpp file isn't getting the dependencies it needs. Any help is greatly appreciated. I have been banging my head on this for 3 days.

[stephendb]

Fixed your code so it at least compiles now, you forgot your int main() and needed a using namespace robosense::lidar;
Also, added target_link_libraries(lidar_driver rs_driver) since just like with me your code wouldn't compile without this PR's INTERFACE addition to rs_driver's CMakeLists.txt. (hint hint Hao)
CMakeLists.txt
test.txt

test.txt is a txt because github doesn't let you add .cpp so make sure to change extension back ;)

[HaoQChen]

Hi all,
I am experiencing the problem that Stephen has detailed. My main file is failing to find the headers when using rs_driver as a submodule. Here is my file tree, cmake, main file (lidar example given) and build output. I am on Ubuntu 20.04

Hi kvern, in your case, it seem to have 2 BUGs

1. You missing the namespace. You can add it like this

2. missing pthread lib. You can add one line at rs_driver/CMakeLists.txt after line 86 like this

the second one is similar with Stephen

[HaoQChen]

Sorry Stephen, I seem to have misunderstood you.

rs_driver's current cmake is traditional cmake, target_include_directories which you suggest is modern cmake. And the current implement does miss some dependencies.

There are docs to change and more test to do when change to modern cmake, we are trying to fix the traditional one and change to modern in next release.

[HaoQChen]

Hi Stephen, a fix has been push to main branch, I have test it with kvern's code in VS2017 and it has solved the include problem.

Could you please test the main branch in your environment?

We will try to change to modern cmake in next release as you suggested

Thanks!

[stephendb]

Yes, that does fix the include issue for me. However, that branch still had private instead of public on the factory param methods so I had to comment some things in my code.

Cheers!

[kvern-code]

Hi all,
Thanks for all the support! This is my first time writing on git and it has been incredibly helpful. My code now compiles, now I just wait for my RS unit to arrive! I did append the lines that Stephen added in his commit as will as the
list(APPEND EXTRERNAL_LIBS "-lpthread" as Hao recommended. I used Stephen's CMakeLists.txt as well.
I was still having issues finding the Boost components. Building with
cmake ../ -DBoost_NO_BOOST_CMAKE=ON
fixed the issue. I found that solution near the bottom of this page. https://github.com/rdkit/rdkit/issues/3030 It appears to be an issue with Ubuntu 20.04 and the newer versions of Boost. I should also add that after I compile it once with cmake ../ -DBoost_NO_BOOST_CMAKE=ON I can compile it without cmake ../ -DBoost_NO_BOOST_CMAKE=ON It is weird behavior.

This is my first C++ project for work, looking forward to development!