Control shared/static linking via BUILD_SHARED_LIBS

[dirk-thomas]

Rebased version of #62. Connect to ros2/ros2#468.

[wjwwood]

this makes it slightly less portable (depends on ament_cmake_ros rather than ament_cmake), but that doesn't matter for now, though we should consider how to integrate this setting (shared vs. static libraries) in other packages which will not depend on ament_cmake_ros or even ament_cmake, e.g. yaml-cpp or opencv, etc... depending on how high up you want to go in the static linking.

[dirk-thomas]

Any package can directly support BUILD_SHARED_LIBS. ament_cmake_ros only provides convenience by registering the option for you and choosing a default. I don't think it affects "portability" in any way nor do we have to consider how other packages support this.

[wjwwood]

I don't think it affects "portability" in any way

It makes this package depend on something outside of ament_cmake, that makes it harder to take up in some other non-ROS project, so I disagree.

We could, instead, decide not to depend on ament_cmake_ros and just introduce the option within rcutils itself, improving portability by reducing required dependencies.

[dirk-thomas]

We could, instead, decide not to depend on ament_cmake_ros and just introduce the option within rcutils itself, improving portability by reducing required dependencies.

This sounds orthogonal to this patch then imo. Please consider bringing it up in the ament_cmake_ros repo where we put the functionality when we needed it.

[wjwwood]

This sounds orthogonal to this patch then imo.

The only reason you're depending on ament_cmake_ros is to gain BUILD_SHARED_LIBS, so I don't think that's true. It's related directly to the purpose of this pull request.

Please consider bringing it up in the ament_cmake_ros repo where we put the functionality when we needed it.

Nothing needs to be changed in ament_cmake_ros. It's simply a choice of duplicate the setting or depend on something, and for normal ros packages depending on it is no problem, for this package it might be in the future.

I'm not asking you to change anything, but the idea that this is unrelated to the topic of the pr is way off in my opinion.

[mikaelarguedas]

I think @wjwwood's point is valid.
For packages that we don't consider "part of ROS" and we expect to be picked up outside of the ROS ecosystem, I'd prefer just adding the support for the BUILD_SHARED_LIBS option rather than bringing in ament_cmake_ros as a dependency, as it also does ROS specific things such as setting a ROS_PACKAGE_NAME. That was my reasoning for not doing it in ament_index_cpp.

Maybe a compromise could be:

• CMake sugar not related to ROS is provided by an ament_cmake-related package hosted in the ament organization that has nothing ROS specific.
  This package would provide:
  • BUILD_SHARED_LIBS option defaulting to ON
  • CMAKE_POSITION_INDEPENDENT_CODE defaulting to OFF?
    and would be use in this package and ament_index
• "ROS specific" cmake magic could stay in ament_cmake_ros and be used by ROS packages?

[dirk-thomas]

For packages that we don't consider "part of ROS" and we expect to be picked up outside of the ROS ecosystem, ...

From my point of view rcutils stands for "ROS C utilities". Therefore I don't see why it shouldn't be able to use ament_cmake_ros. That package poses pretty much no extra dependency here - we are already relying on ament_cmake which is a much much bigger dependency.

I'd prefer just adding the support for the BUILD_SHARED_LIBS option rather than bringing in ament_cmake_ros as a dependency, as it also does ROS specific things such as setting a ROS_PACKAGE_NAME.

Maybe we should consider to rename the definition to CMAKE_PROJECT_NAME?

---

While all of these options can be considered I would rather not let them stall this PR. They can be discussed in the context of ament_cmake_ros. And whatever the outcome of the conversation is if necessary this repository can be updated then.