search

ros-industrial / industrial_core

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

Fix build with latest moveit's master branch #244

Closed gleichdick closed 3 years ago

gleichdick commented 5 years ago

PlanningRequestAdapter::initialize() is now pure virtual so we have to provide an empty implementation on each PlanningRequestAdapter subclass.

See also ros-planning/moveit#1621

Please be careful whether this patch breaks backward compatibility with older moveit builds ( where a virtual initialize() method is implemented and not pure virtual)

gleichdick commented 5 years ago

Thank you for your reply, I removed the override specifier so that it compiles with kinetic and melodic. I didn't check it before, my bad. Please be aware that it breaks the ABI (because a new virtual method is added to a public class) in kinetic and melodic.

So there are multiple solutions:

  1. Close this PR and continue the discussion after a new ROS release
  2. Check moveit_core_VERSION and set a flag as you suggested
  3. Write a small cpp file that tests whether PlanningRequestAdapter is abstract without custom initialize() with CMake try_compile and set a flag

The drawback of a flag is that it has to stay until support for kinetic and melodic has been dropped and that it will be also present in third party code (which e.g. includes filter_base.h)

I'm happy implementing options 2 or 3 but IMHO the first option might be the best.

Regards,

Bjarne

gavanderhoorn commented 3 years ago

I believe approach 2 was included in #258, so this should now work again.