Visibility control headers

[lilustga]

Description

Add visibility control headers.
Set the building flag for the respective header in each CMakeLists.txt.
Add public macros in headers where the automatic symbols export is insufficient.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[rhaschke]

First of all, why did you decided to go for manual visibility control here, but for global visibility in moveit/srdfdom#69?

If you go for manual visibility (which in general is more memory efficient and thus should be preferred): Instead of manually writing the boiler-plate visibility-control header files, I suggest to use corresponding cmake functionality, namely generate_export_header.
Please have a look for an example at ros-visualization/rviz#1335

[lilustga]

@rhaschke

These are instances where the global visibility is insufficient. For example global static variables will not get exported properly without manual visibility.

The decision to use the boiler-plate headers follows this logic:
ament/ament_cmake#201 (comment)

[rhaschke]

These are instances where the global visibility is insufficient. For example global static variables will not get exported properly without manual visibility.

So, you (need to) mix global and manual visibility? IMO, that's horrible.

The decision to use the boiler-plate headers follows this logic:
ament/ament_cmake#201 (comment)

I don't agree with @wjwwood on this. Manually creating those visibility headers is potentially error-prone and auto-creating them isn't that costly I think. Maybe other maintainers can weigh in with their opinion?

[wjwwood]

Well we will have to agree to disagree. I don’t think the boilerplate headers are error prone and they haven’t changed in years and years. I won’t repeat my reasoning here since it’s on that issue.

[JafarAbdi]

@lilustga I tried to compile this branch with a windows machine but moveit_core is failing because it can't find boost zlib, do you know how to fix it .?

[lilustga]

@JafarAbdi That error can be fixed by removing the following from moveit_core/CMakeLists.txt. It is not required for the version of boost that is being used now.

# boost::iostreams on Windows depends on boost::zlib	
if(WIN32)	
  set(EXTRA_BOOST_COMPONENTS zlib)	
endif()

This PR specifically regards the visibility control headers, I discussed with Henning having specific topic PRs so we could get consensus on the couple main items individually.

This fork contains all the changes to build on windows: https://github.com/lilustga/moveit2/tree/builds_on_windows

[henningkayser]

@lilustga To me it looks like the include paths might be wrong which could lead to the issues with missing header files you described.

[codecov]

Codecov Report

Merging #286 (5ea9a1a) into main (9256987) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #286   +/-   ##
=======================================
  Coverage   48.00%   48.00%           
=======================================
  Files         157      157           
  Lines       14843    14843           
=======================================
  Hits         7124     7124           
  Misses       7719     7719           

Impacted Files	Coverage Δ
...n_detection_fcl/collision_detector_allocator_fcl.h	100.00% <ø> (ø)
...ene/include/moveit/planning_scene/planning_scene.h	42.31% <ø> (ø)
...bot_model/include/moveit/robot_model/robot_model.h	84.22% <ø> (ø)
...bot_state/include/moveit/robot_state/robot_state.h	88.47% <ø> (ø)
...it/planning_scene_monitor/planning_scene_monitor.h	20.00% <ø> (ø)
...e/include/moveit/kinematics_base/kinematics_base.h	44.45% <100.00%> (ø)
...include/moveit/robot_trajectory/robot_trajectory.h	94.45% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9256987...5ea9a1a. Read the comment docs.

[henningkayser]

I see green, awesome! I think we should merge this as is and add an issue describing why we are using manual visibility in these cases and what might be required to switch to global in the future.

[wolfv]

@lilustga you might be interested in our efforts to add Windows CI to the Moveit project using the packages from the RoboStack project. Our Foxy stack is currently a bit behind (noetic has much more packages), but with a little help we could create Windows packages so that they could be used as a basis for a CI similar to what we're adding here for Moveit 1: moveit/moveit#2604 (specifically this github action: https://github.com/RoboStack/moveit/blob/tests_with_conda_pkgs/.github/workflows/cross_platform_ci.yml)

You can find the currently available packages here: https://robostack.github.io/foxy.html

[lilustga]

@wolfv, I'm interested. Cross platform CI would be great! Lets discuss what's needed to get there.

[wolfv]

Do you want to join us on our Gitter chat room (Gitter.im/robostack/Lobby)? Or feel free to send me an email and we can discuss by email/video/...
You can find my email in the GitHub profile.

[AndyZe]

@ooeygui do you have an opinion on this?

[ooeygui]

@ooeygui do you have an opinion on this?

Hi @AndyZe ,
Lior and I have been working together for MoveIt2 support on Windows. Visibility Control headers are the recommended solution for exposing symbols outside of a binary on Windows. While the WINDOWS_EXPORT_ALL_SYMBOLS solution works well for many components (like nodelets), when exposing static member variables from a class (like Qt components), the automatic export does not work, so visibility control is needed.

We've documented this at the Azure Edge Robotics landing page - https://aka.ms/ros/visibility (and will be adding docs to ros.org as well).

Edit: We are also having conversations with @wolfv and team with Open Robotics about RoboStack. It's a great solution which creates one model for Windows, Mac and Linux. We have a tracking issue at https://aka.ms/ROSOnWindowsIssues - ms-iot/ROSOnWindows#324.

[lilustga]

closing because #530 was merged.