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

Missing export of required public definition PLUGINLIB__DISABLE_BOOST_FUNCTIONS for downstream packages #2429

Closed
devrite opened this issue Jul 1, 2021 · 13 comments

Comments

@devrite
Copy link
Contributor

devrite commented Jul 1, 2021

  • Operating System:
    • Ubuntu 20.04
  • ROS2 Version:
    • rolling+galactic(flag depr.)
    • foxy
  • Version or commit hash:

Steps to reproduce issue

docker run --rm -it ros:foxy-ros-base-focal
apt-get update && apt-get install -y ros-foxy-navigation2
grep -r PLUGINLIB__DISABLE_BOOST_FUNCTIONS /opt/ros/foxy/share

Expected behavior

Similar output as below (e.g., also done by the rviz plugin) for all libs and plugins exported via navigation2. Missing for a lot of plugins. Just do a grep on the source to tree to identify which packages do public set the PLUGINLIB__DISABLE_BOOST_FUNCTIONS.

Other packages including, for example, headers from nav2_costmap_2d will eventually include pluginlib headers but the definition is not set, thus forcing a downstream package to set this definition itself or falsely enabling the boost features if boost is installed via another dependency already (e.g. if the boost headers are found).

Actual behavior

rviz_common/cmake/rviz_commonExport.cmake:  INTERFACE_COMPILE_DEFINITIONS "PLUGINLIB__DISABLE_BOOST_FUNCTIONS"
rviz_default_plugins/cmake/rviz_default_pluginsExport.cmake:  INTERFACE_COMPILE_DEFINITIONS "PLUGINLIB__DISABLE_BOOST_FUNCTIONS"
nav2_rviz_plugins/cmake/nav2_rviz_pluginsExport.cmake:  INTERFACE_COMPILE_DEFINITIONS "PLUGINLIB__DISABLE_BOOST_FUNCTIONS"
rosbag2_cpp/cmake/export_rosbag2_cppExport.cmake:  INTERFACE_COMPILE_DEFINITIONS "PLUGINLIB__DISABLE_BOOST_FUNCTIONS"
rosbag2_storage/cmake/export_rosbag2_storageExport.cmake:  INTERFACE_COMPILE_DEFINITIONS "PLUGINLIB__DISABLE_BOOST_FUNCTIONS"

Additional information

I will prepare a pull request for review, which should fix the problem. There are two possible approaches, switching to modern CMake Export as with the rviz plugin or manually exporting the definition via ament.

@SteveMacenski
Copy link
Member

SteveMacenski commented Jul 1, 2021

I see this all over the place searching the source https://github.com/ros-planning/navigation2/search?q=PLUGINLIB__DISABLE_BOOST_FUNCTIONS so its just the exporting I believe. Can you show me an example of the 2 options so I can compare? A set of PRs would be appreciated if this resolves an issue you're facing.

@SteveMacenski
Copy link
Member

As an aside, I see that you're interested in radars. I'd like to support a radar native costmap layer if that was a project you'd be interested in. I spent a few months creating the ROS standard interfaces for radars https://github.com/ros-perception/radar_msgs to support that effort

@devrite
Copy link
Contributor Author

devrite commented Jul 1, 2021

For the first option I have created a draft pr #2430.

For the other option I only can reference the source code of ament_cmake for now. I don't see an example in the official doc or did not find it, but here you go:

https://github.com/ament/ament_cmake/blob/master/ament_cmake_export_definitions/cmake/ament_export_definitions.cmake

@SteveMacenski
Copy link
Member

@devrite
Copy link
Contributor Author

devrite commented Jul 1, 2021

But please review it. Just to make sure it is not breaking anything. As far as radar goes, we are working on it in other projects, but besides PCLs are not using much right now. Which might be a way to integrate the messages I think. We have no consistent idea yet on other radar data (radar cube / ADC / FFT samples). This is essentially just a complex or real array whose interpretation greatly depends on the sensor used.

Maybe I have more info on it when we further progress with it. Also some other radar maps are essentially just birds eye view Image where each point is the intensity. (either polar cordinates or in xy). So far we basically have no other data than your message, so this looks fine.

@devrite
Copy link
Contributor Author

devrite commented Jul 1, 2021

Ex. of it in work https://gitlab.com/ApexAI/performance_test_ci/-/blob/29181ee9ca1c35598ab3fd53d521859ab9b1a06f/performance_test/CMakeLists.txt#L100

That looks alot cleaner to me

Yep. This is essentially the other option. Although I am not sure which approach is currently preferred by ros guidelines as you can do both (even in the documentation) either modern cmake export or via ament. This options is necessary if there is no target to export anyway or the definition is not attached to a target like in the library cases.

@devrite
Copy link
Contributor Author

devrite commented Jul 1, 2021

And here, where it will fail for an upstream project if this header gets used.

@devrite
Copy link
Contributor Author

devrite commented Jul 1, 2021

Just mentioning: Maybe the header of plugin lib should not be used in a public header. This would avoid the problem too. If this is the case then we should also set the definitions PRIVATE, as they are not intended for interfaces. But I just assumed since it was set PUBLIC it is necessary.

@devrite devrite changed the title Missing of export required public definition PLUGINLIB__DISABLE_BOOST_FUNCTIONS for downstream packages Missing export of required public definition PLUGINLIB__DISABLE_BOOST_FUNCTIONS for downstream packages Jul 1, 2021
@SteveMacenski
Copy link
Member

SteveMacenski commented Jul 1, 2021

I'm also just thinking about maintenance as new things are added, doing the ament way is way simpler and you'd be far more likely to be able to rely on me catching that in a review than the current changes. If ament has a solution and its that clean, that's my preferred method.

What's the issue you run into that this came up in? I see you grep it, but I don't necessarily understand the problem.

Other packages including, for example, headers from nav2_costmap_2d will eventually include pluginlib headers but the definition is not set, thus forcing a downstream package to set this definition itself or falsely enabling the boost features if boost is installed via another dependency already (e.g. if the boost headers are found).

I don't necessarily see this as a problem. Someone might actually want boost to be used for some reason. So we set it specifically for our packages to disable when its not needed because we don't use it in Nav2. I don't assume that no one wants it (or else, why would it be possible?). I agree though it might be a quality of life improvement for developers and I'm OK with doing that as long as the way its done is clean and easy to maintain.

@devrite
Copy link
Contributor Author

devrite commented Jul 2, 2021

I found it when compiling grid_map_costmap_2d. Where it failed. Then just saw that some other plugins export it (see the actual behaviour output). Even nav2 rviz. It is even set to public in cmake which usually communicates the user needs it. All of this made me assume we want the user to have it.

So the question is does a new plug in based on another need the PLUGINLIB header to use any nav functions of the nav packages and the definition? Or what about a user just using some functions but not writing a plugin?

If not no PLUGINLIB header should not be part of a public header and to make it clear that the user writing a plugin will decide if he wants the features or not the definition should be made private then. As you see modern cmake (as with nav2_rviz has done already) would otherwise auto export it to the user.
Also I have not looked into those features, can they make anything incompatible or are those just add-ons? Since nobody at grid_map complained I guess they are not incompatible. But they use boost::shared_ptrs but I guess since C++14 in ROS we do mot want people to mix those?

I can prepare another pr if you want with just ament export. But I will leave nav2 rviz as is.

@devrite
Copy link
Contributor Author

devrite commented Jul 2, 2021

Just saw. Pluginlib removed those feature disable in the galactic/rolling branch (deprecated boost use) for example so it might be just a foxy issue after all.

ros/pluginlib#199

So I will just prepare a pr for foxy that exports via ament. And we should probably write one for removing it altogether for rolling and galactic.

@SteveMacenski
Copy link
Member

Oh got it, if this is only a Foxy issue, I'm far more open to any solution since it only effects 1 distribution. I see you opened a few new PRs, I'll take a look

@SteveMacenski
Copy link
Member

They've both been merged! Closing (unless there's more?)

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

No branches or pull requests

2 participants