Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build with latest moveit's master branch #244

Closed

Conversation

gleichdick
Copy link

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

See also moveit/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)

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

Some comments:

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

Yes, but only on the master branch.

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

In it's current form, I don't think we can accept this change. As Travis shows, it doesn't build on either Kinetic or Melodic.

Generally I'm not a fan of it, but could we add some conditional compilation directives to filter_base.h and other files that would need it? It might be possible to use the moveit_core_VERSION CMake variable, which we might need to pass along to the preprocessor using target_compile_definitions(..).

Otherwise we'd have to create a branch specifically to track MoveIt's master branch, which I'm not willing to do at this time.

@gavanderhoorn gavanderhoorn changed the title Fix build with latest moveit core package Fix build with latest moveit's master branch Nov 15, 2019
PlanningRequestAdapter::initialize() is a pure virtual function on current
moveit master so we have to provide an empty implementation on each subclass.

See also moveit/moveit#1621
@gleichdick
Copy link
Author

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants