search

ros2 / rmw_implementation

CMake infrastructure and dependencies for rmw implementations
Apache License 2.0
21 stars 48 forks source link

Should ament_index_cpp only be find_package(..)ed when runtime selection is enabled? #209

Closed gavanderhoorn closed 1 year ago

gavanderhoorn commented 1 year ago

Bug report

Required Info:

This isn't so much an issue as it is a question: #189 added a build dependency on ament_index_cpp to rmw_implementation:

https://github.com/ros2/rmw_implementation/blob/8c3c133eb070a4f19259f027cc2cea720b28eff0/rmw_implementation/CMakeLists.txt#L15

From the rest of the CMakeLists.txt, ament_index_cpp only appears to be used when RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION is ON:

https://github.com/ros2/rmw_implementation/blob/8c3c133eb070a4f19259f027cc2cea720b28eff0/rmw_implementation/CMakeLists.txt#L44-L61

However, because find_package(ament_index_cpp ..) appears outside the if(..) else (..), it's always executed, which leads to problems on (build) platforms which do not support C++ nor runtime RMW selection.

Should line 15 be moved to somewhere around here, so it is also guarded by RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION?

If this would be OK, I could submit a PR introducing this change.

clalancette commented 1 year ago

Should line 15 be moved to somewhere around here, so it is also guarded by RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION?

If this would be OK, I could submit a PR introducing this change.

Yes, that seems totally reasonable to me; as you mention, it is only needed when DISABLE_RUNTIME_SELECTION is OFF. Looks like this was probably just an oversight, so a PR to fix that would be welcome.

gavanderhoorn commented 1 year ago

See #210.