westonrobot / scout_ros

ROS Support Package for Scout Robot
BSD 3-Clause "New" or "Revised" License
38 stars 21 forks source link

Add ASIO_ENABLE_OLD_SERVICES definition to scout_base_node #3

Closed ojura closed 2 years ago

ojura commented 2 years ago

Building ugv_sdk and scout_base in isolation (with catkin_make_isolated/catkin build) fails for me.

The ASIO_ENABLE_OLD_SERVICES compile definition is not exported for the ugv_sdk target.

It is needed when including ugv_sdk/include/ugv_sdk/details/async_port/async_can.hpp.

ojura commented 2 years ago

This is the build error we get with catkin_make_isolated/catkin build:

In file included from /home/juraj/scout_ws/src/ugv_sdk/include/ugv_sdk/utilities/protocol_detector.hpp:15,
                 from /home/juraj/scout_ws/src/scout_ros/scout_base/src/scout_base_node.cpp:10:
/home/juraj/scout_ws/src/ugv_sdk/include/ugv_sdk/details/async_port/async_can.hpp:41:16: error: ‘basic_stream_descriptor’ in namespace ‘asio::posix’ does not name a template type; did you mean ‘stream_descriptor’?
   41 |   asio::posix::basic_stream_descriptor<> socketcan_stream_;
      |                ^~~~~~~~~~~~~~~~~~~~~~~
      |                stream_descriptor
/home/juraj/scout_ws/src/ugv_sdk/include/ugv_sdk/details/async_port/async_can.hpp:49:34: error: ‘asio::posix::basic_stream_descriptor’ has not been declared
   49 |                     asio::posix::basic_stream_descriptor<> &stream);
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~
/home/juraj/scout_ws/src/ugv_sdk/include/ugv_sdk/details/async_port/async_can.hpp:49:57: error: expected ‘,’ or ‘...’ before ‘<’ token
   49 |                     asio::posix::basic_stream_descriptor<> &stream);
      |                                                         ^
make[2]: *** [CMakeFiles/scout_base_node.dir/build.make:63: CMakeFiles/scout_base_node.dir/src/scout_base_node.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1675: CMakeFiles/scout_base_node.dir/all] Error 2
make: *** [Makefile:141: all] Error 2
ojura commented 2 years ago

Note: this is necessary, because catkin does not support exporting compile definitions (source: https://github.com/ros/catkin/issues/988).

Here are some options on how to resolve this:

My proposal is doing it the first way, like in this PR.

rdu-weston commented 2 years ago

Note: this is necessary, because catkin does not support exporting compile definitions (source: ros/catkin#988).

Here are some options on how to resolve this:

* all clients of `ugv_sdk` need to add this definition (like in this PR);

* generate a custom config file for `ugv_sdk` which would add this definition to be used for targets linking to `ugv_sdk` (and which are including its headers)

* or `ugv_sdk/details/async_port/async_can.hpp`, which depends on this definition, needs to be removed from `ugv_sdk` public headers.

* `#define ASIO_ENABLE_OLD_SERVICES` could be added to `async_can.hpp` before `#include "asio/posix/basic_stream_descriptor.hpp"` (not 100% robust, because someone could in theory include this file before this)

My proposal is doing it the first way, like in this PR.

Let's use the first way for now. I already have "target_compile_definitions(${PROJECT_NAME} PUBLIC ASIO_ENABLE_OLD_SERVICES)" for ugv_sdk. In standard CMake project, it should do the work but apparently not with catkin. I will try if I can hide the asio related stuff from public headers but that will take more time.

ojura commented 2 years ago

Alright, thanks!

By the way, using catkin_make for building workspaces should be deprecated in favour of building packages in isolation with catkin_make_isolated (which is what the ROS build farm uses) or catkin build.

I understand that we still occasionally use catkin_make, especially in teaching, but consider adding a build pipeline test for checking if it builds in isolation :)