ros2 / cartographer_ros

Provides ROS integration for Cartographer.
Apache License 2.0
121 stars 59 forks source link

fix cmake to prevent multiple definitions #63

Closed wolfv closed 2 years ago

wolfv commented 2 years ago

I've seen double definition errors for a couple of symbols and I think the reason is that the _main.cpp files should not be part of the library (they are used to create the executables later in the CMake file).

clalancette commented 2 years ago

Oh, excellent point. @doisyg Do you agree?

wolfv commented 2 years ago

You can find some of these errors here: https://github.com/RoboStack/ros-humble/runs/6999402135?check_suite_focus=true#step:3:3239

doisyg commented 2 years ago

Nice catch. If you confirm that the executables are still usable and properly exported, that looks good to me.

wolfv commented 2 years ago

I haven't been able to confirm that the executables are fine but they are linking correctly on our CI. I am really convinced that you shouldn't add multiple main() functions to the library. It should be enough to declare the ..._main.cpp files with the executables in add_executable(...) and link to the library.

clalancette commented 2 years ago

I am really convinced that you shouldn't add multiple main() functions to the library. It should be enough to declare the ..._main.cpp files with the executables in add_executable(...) and link to the library.

Yeah, I don't think anyone is disputing that. We just want to make sure that the executables are still working.

wolfv commented 2 years ago

What's the best way to test the executables? You can try the robostack builds if you want, on Linux:

# this installs micromamba into ~/.local/bin/micromamba
curl micro.mamba.pm/install.sh | bash
source ~/.bashrc
micromamba create -n ros ros-humble-cartographer-ros -c robostack-humble -c conda-forge
micromamba activate ros
# now you can execute the cartographer-ros binaries
clalancette commented 2 years ago

I was able to confirm that at least the main node starts fine with the following:

ros2 run cartographer_ros cartographer_node -configuration_directory src/cartographer_ros/cartographer_ros/configuration_files -configuration_basename backpack_2d.lua

So this looks good to me. I'm going to go ahead and merge this in.