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.

Sign up for GitHub

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

Jump to bottom

Fixes for usage on windows #91

Merged
merged 4 commits into from
Jun 28, 2021
Merged

Fixes for usage on windows #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcf1cbd
Add visibility control
Ace314159 Jun 3, 2021
2e69632
Add TinyXML2 to export dependencies
Ace314159 Jun 3, 2021
562972f
Run clang-format
JafarAbdi Jun 23, 2021
1e712ba
Install to bin folder too
Ace314159 Jun 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ ament_target_dependencies(${PROJECT_NAME} PUBLIC

install(TARGETS ${PROJECT_NAME}
EXPORT ${PROJECT_NAME}
DESTINATION lib
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin
)
install(DIRECTORY include/${PROJECT_NAME}/
DESTINATION include/${PROJECT_NAME}
Expand All @@ -64,6 +66,7 @@ install(PROGRAMS
)

set_target_properties(${PROJECT_NAME} PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
Copy link
Member

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 .?

Copy link
Author

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.

target_compile_definitions(${PROJECT_NAME} PRIVATE "SRDFDOM_BUILDING_DLL")

if(BUILD_TESTING)
find_package(ament_cmake_gtest REQUIRED)
Expand All @@ -90,4 +93,5 @@ ament_export_libraries(${PROJECT_NAME} ${TinyXML2_LIBRARIES})
ament_export_dependencies(console_bridge)
ament_export_dependencies(urdfdom_headers)
ament_export_dependencies(urdf)
ament_export_dependencies(TinyXML2)
ament_package()
4 changes: 3 additions & 1 deletion include/srdfdom/model.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@
#include <memory>
#include <tinyxml2.h>

#include "visibility_control.h"

/// Main namespace
namespace srdf
{
/** \brief Representation of semantic information about the robot */
class Model
class SRDFDOM_PUBLIC Model
{
public:
Model()
Expand Down
35 changes: 35 additions & 0 deletions include/srdfdom/visibility_control.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#ifndef SRDFDOM__VISIBILITY_CONTROL_H_
Copy link
Member

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

Copy link
Author

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.

#define SRDFDOM__VISIBILITY_CONTROL_H_

// This logic was borrowed (then namespaced) from the examples on the gcc wiki:
// https://gcc.gnu.org/wiki/Visibility

#if defined _WIN32 || defined __CYGWIN__
#ifdef __GNUC__
#define SRDFDOM_EXPORT __attribute__((dllexport))
#define SRDFDOM_IMPORT __attribute__((dllimport))
#else
#define SRDFDOM_EXPORT __declspec(dllexport)
#define SRDFDOM_IMPORT __declspec(dllimport)
#endif
#ifdef SRDFDOM_BUILDING_DLL
#define SRDFDOM_PUBLIC SRDFDOM_EXPORT
#else
#define SRDFDOM_PUBLIC SRDFDOM_IMPORT
#endif
#define SRDFDOM_PUBLIC_TYPE SRDFDOM_PUBLIC
#define SRDFDOM_LOCAL
#else
#define SRDFDOM_EXPORT __attribute__((visibility("default")))
#define SRDFDOM_IMPORT
#if __GNUC__ >= 4
#define SRDFDOM_PUBLIC __attribute__((visibility("default")))
#define SRDFDOM_LOCAL __attribute__((visibility("hidden")))
#else
#define SRDFDOM_PUBLIC
#define SRDFDOM_LOCAL
#endif
#define SRDFDOM_PUBLIC_TYPE
#endif

#endif // SRDFDOM__VISIBILITY_CONTROL_H_