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

Add support for Windows #474

Open
wants to merge 1 commit into
base: ros2
Choose a base branch
from

Conversation

traversaro
Copy link

The packages in this repo compile shared libraries, but does not expose any symbol on Windows, so no library is actually generated on Windows.

On Linux and macOS, everything compiles fine as by default all the symbols are visible. We can achieve exactly the same behavior in Windows by setting to ON the CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS CMake variable, so this PR sets the CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS variable to ON, to ensure that the compilation works fine on Windows.

Fix #427 .

@mvieth
Copy link
Contributor

mvieth commented Jan 13, 2025

Wouldn't it be better to work with __declspec(dllexport) instead? Sure, setting CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to ON is a quick and easy approach, but choosing which classes/functions to export seems cleaner?

@Rayman
Copy link
Contributor

Rayman commented Jan 15, 2025

I think the best way is using a visibility_control.hpp file in combination with target_compile_definitions(... PRIVATE "PCL_ROS_BUILDING_LIBRARY"). This approach is documented here: https://docs.ros.org/en/jazzy/The-ROS2-Project/Contributing/Windows-Tips-and-Tricks.html

@traversaro
Copy link
Author

Sorry, I missed @mvieth comment, thanks @Rayman for the following reply!

Sure, explicit symbol visibility has its own advantages (https://www.cs.dartmouth.edu/~sergey/cs108/ABI/UlrichDrepper-How-To-Write-Shared-Libraries.pdf) but also disadvantage, in particular one major disadvantage is the need to manually annotate each public symbol, that requires extensive effort both for the initial port on an existing project to explicit symbol visibility, and then to make sure that the symbol visibility macro are correctly placed in every following PR. More specifically, I would be really glad to do the visibility macros changes here, but realistically I do not have the time.

Different projects on ROS 2 have done different choice w.r.t. to this tradeoff, with ROS 2 core packages using visibility headers, while downstream projects like moveit2 or ros2_control using exporting all symbols (and using CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS).

What I am not sure to understand is how this is related to Windows support. From what I understand, this project worked fine exporting all symbols for a long time without any problem, so I am not sure why that should change if a small change to export all symbols also on Windows was added, like in this PR.

Regardless of the outcome of this discussion/PR, something that I would suggest to avoid in my experience is to export all symbols on Linux/macOS, while using manual symbol visibility macros just on Windows, as in that case all of the contributors would use Linux to propose changes, and the would not detect if the symbol visibility macros are not correctly placed.

@Rayman
Copy link
Contributor

Rayman commented Jan 15, 2025

Thank you for your reply. I see moveit2 is indeed using CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS.

While visibility control macros are the preffered way of working, I think it is indeed a major change and maybe CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is good enough for the forseeable future. You have my +1.

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

Successfully merging this pull request may close these issues.

build on Windows
3 participants