-
Notifications
You must be signed in to change notification settings - Fork 68
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
Fixes for usage on windows #91
Conversation
This change looks good. We (Azure Edge Robotics) were making an equivalent fix. |
@Ace314159 In parallel, We are working on MoveIt2 for Windows - moveit/moveit2#504. |
Codecov Report
@@ Coverage Diff @@
## ros2 #91 +/- ##
=======================================
Coverage 68.21% 68.21%
=======================================
Files 3 3
Lines 607 607
=======================================
Hits 414 414
Misses 193 193
Continue to review full report at Codecov.
|
There was one more change I had to make that I didn't realize. Loading the MoveIt Rviz plugins fails because it is unable to find |
@Ace314159 great catch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix and sorry for the delay!
This looks good to me, unfortunately, we still don't have a standard/documented way to export the symbols in ros-planning
repos
I'll wait for a few hours to see if other maintainer have a concern otherwise I'll merge it
@@ -64,6 +66,7 @@ install(PROGRAMS | |||
) | |||
|
|||
set_target_properties(${PROJECT_NAME} PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this .? adding SRDFDOM_EXPORT
to SRDFWriter
should make this obsolete, right .?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that should make it obsolete. We would need to add SRDFDOM_PUBLIC
because the export should only occur while building and it should import when another library includes it.
However, you would need to manually add SRDFDOM_PUBLIC
to all new classes, and that might be easy to forget. I still think the global export should be used since it's much simpler than tagging all new classes and more closely matches the behavior of other compilers.
@@ -0,0 +1,35 @@ | |||
#ifndef SRDFDOM__VISIBILITY_CONTROL_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use GenerateExportHeader .? Do you know what are the drawbacks .? I'm happy to open a PR against your branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been some discussion here: moveit/moveit2#286 (review), and I think using the visibility headers instead of the cmake function would be better. To quote from links from that discussion:
In summary, I prefer the boilerplate header we use (e.g. https://github.com/ros2/rclcpp/blob/231b991098d685ff019c44edb80a8ff170d65e4a/rclcpp/include/rclcpp/visibility_control.hpp#L25-L54) to the boilerplate generated code that cmake has to generate every time you build. Neither change, but the static one is nicer in my opinion because you don't waste time generating it each time and because it lets static code analysis (or like autocomplete for text editors) work without having to first build the package and have the build/install space for that package on the path for your tool. Also, if you were to port a package to a different build system (e.g. bazel or something) then you don't have to also port the behavior of
generate_export_header
.
We have a document on Visibility on Windows: https://aka.ms/ros/visibility. For many cases, the cmake macro for Windows will work; but there are limitations - such as when there is a static export from a class. |
I changed 2 things to get this building on Windows and working with MoveIt 2.
empty_vector_
when build the tests. This is because theWINDOWS_EXPORT_ALL_SYMBOLS
CMake property doesn't work for static variables, so in such cases manual visibility control headers are required.ament_export_dependencies