Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Symbol Visibility in ROS on Windows #322

Merged
merged 3 commits into from
Mar 18, 2021
Merged

Symbol Visibility in ROS on Windows #322

merged 3 commits into from
Mar 18, 2021

Conversation

lilustga
Copy link
Contributor

New Page under the Porting Folder which describes how to add explicit symbol visibility to a ROS package.

@lilustga lilustga requested a review from a team as a code owner March 15, 2021 21:00
mkdocs.yml Outdated Show resolved Hide resolved
docs/Porting/SymbolVisibility.md Outdated Show resolved Hide resolved
docs/Porting/SymbolVisibility.md Outdated Show resolved Hide resolved
docs/Porting/SymbolVisibility.md Outdated Show resolved Hide resolved
docs/Porting/SymbolVisibility.md Outdated Show resolved Hide resolved
docs/Porting/SymbolVisibility.md Outdated Show resolved Hide resolved
docs/Porting/SymbolVisibility.md Outdated Show resolved Hide resolved
@lilustga lilustga requested a review from ooeygui March 16, 2021 01:44
#endif
#define MY_LIB_PUBLIC_TYPE
#endif
#endif // MY_LIB__VISIBILITY_CONTROL_HPP_
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be missing something, but it seems that with this visibility control header, the code cannot be compiled/used as a static library?

Copy link
Member

Choose a reason for hiding this comment

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

@lilustga can you comment on @traversaro's question? It would be good to capture this in the doc as well

Copy link
Contributor

@traversaro traversaro Mar 16, 2021

Choose a reason for hiding this comment

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

Related comment in an older PR: ros2/rclcpp#638 (comment) and ros/console_bridge#40 (comment) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@traversaro, I generated a header using generate_export_header and saw how it uses LIB_NAME_STATIC_DEFINE to not set the TOOLS_EXPORT macro to either dllimport or dllexport. We could add the same logic to this boilerplate header, would that be sufficient to allow for building the same lib as both static and shared?

Copy link
Contributor

@traversaro traversaro Mar 17, 2021

Choose a reason for hiding this comment

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

If you have a non-generate export header, if you add a LIB_NAME_STATIC_DEFINE to disable dllimport and dllexport decoration, you also need to specify that that the macro needs to be definde when the library is a static library, i.e. :

get_target_property(lib_name_TYPE lib_name TYPE)
if(lib_name_TYPE  STREQUAL "STATIC_LIBRARY")
  target_compile_definitions(lib_name PUBLIC LIB_NAME_STATIC_DEFINE)
endif()

Note that however this solution only works correctly in downstream projects that link the static version of the library if the downstream projects link the library using CMake and linking with the exported target. If they do not use CMake or they do not link with the exported target, they need to define LIB_NAME_STATIC_DEFINE manually depending of whether they are linking the static version of lib_name or the shared one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insight, I'll test this in a future workstream and incorporate it into the documentation.

Copy link
Contributor

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

@ooeygui
Copy link
Member

ooeygui commented Mar 16, 2021

@lilustga Let's remove the section from the cookbook that @traversaro mentions above since we're elevating this to a top level link.

@ooeygui ooeygui merged commit c85b5e9 into ms-iot:master Mar 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants