search

ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
154 stars 181 forks source link

`simple_message` doesn't compile without `LINUXSOCKETS` defined #188

Closed jdlangs closed 3 years ago

jdlangs commented 6 years ago

This is mainly a problem when a dependent project includes some header files from simple_message and the developer doesn't realize he/she has to set this compile flag. You end up getting errors about socket types not defined and the only way to figure out what's going on is to go look in the code.

Should this be changed so that the LINUXSOCKETS behavior is default and the other platform builds require a flag?

shaun-edwards commented 6 years ago

It's been a while, but this sounds like an issue I've had before. It's a lack of CMake ability on my part. Do you know what change to make?

jdlangs commented 6 years ago

I'm not sure what the best practices around conditional compilation for different platforms are. In this case, since it looks like only other condition is the the MOTOPLUS flag, would it make sense just to change #ifdef LINUXSOCKETS to #ifndef MOTOPLUS?

shaun-edwards commented 6 years ago

That might work. It wouldn't support more than two options though (like a third socket library). Still feels like a CMake approach would be better, like exporting flags or something. I'd accept a PR on this one. Very few people (or maybe no one) use the headers for MOTOPLUS (shared code was thrown out a long time ago).

gavanderhoorn commented 6 years ago

@shaun-edwards wrote:

Still feels like a CMake approach would be better, like exporting flags or something.

exactly, that would be the correct way to do this, although we'd have to make a choice for a particular set of value(s) for the defines.

For Catkin packages, this could be done with a CFG_EXTRAS file. See export gcc flags though catkin? CFG_EXTRAS? on ROS Answers for an example (discusses compiler flags, but it works for other things as well).

An alternative (and non-Catkin) approach would be to make sure a simple_message_DEFINITIONS variable gets set whenever simple_message is find_package(..)d. Consumers would then need to add something like this to their CMakeLists.txt (newer CMake versions might use slightly different syntax, I haven't checked):

add_definitions(${simple_message_DEFINITIONS})

Such a variable should probably be part of the simple_messageConfig.cmake file (a "config file" in CMake parlance). Not sure how to implement that with Catkin.

As simple_message is a Catkin package -- and we are already using a CFG_EXTRAS file (here) -- that might be the simplest way to do this. Non-ROS usage of simple_message should still work, as Catkin packages can be find_package(..)d.

gavanderhoorn commented 3 years ago

Closing as #262 should address this.