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

Allow static library builds #64

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+39 −5
Open

Allow static library builds #64

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72e61e5
Enable static library building
jslee02 Sep 30, 2015
e40bcd8
Add config.h to pass URDFDOM_STATIC definition to client libraries
jslee02 Oct 1, 2015
25d4010
Remove unintended lines in CMakeLists.txt and modify logic of setting…
jslee02 Oct 1, 2015
6d8e971
Merge remote-tracking branch 'upstream/master' into static_build
jslee02 Dec 9, 2015
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
9 changes: 9 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ if (HAS_VISIBILITY)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fvisibility=hidden")
endif()

# Using dynamic linking by default
option(BUILD_SHARED_LIBS "Build shared libraries" ON)

# If building static libraries, set URDFDOM_STATIC definition for symbol
# visibility settings.
if (NOT BUILD_SHARED_LIBS)
add_definitions(-DURDFDOM_STATIC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the definition only here will mean that client binaries that link urdfdom as a static library will import the headers with URDFDOM_DLLAPI set to __declspec(dllimport).
This at least by inspection of https://github.com/ros/urdfdom/blob/master/urdf_parser/include/urdf_parser/exportdecl.h and knowing that client libraries will not have the URDFDOM_STATIC definition set.
Are you sure this is ok and not causing linking problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. This cannot resolve the linking problems since the client libraries will not have URDFDOM_STATIC.

There are two possible ways I can think:

  • pass the definition set to the client through urdfdom.pc or FindUrdfdom.cmake
  • directly config exportedcl.h to define proper preprocessors in the cmake process

I'm not sure which one is preferred way.

Edit: It seems urdfdom.pc is not generated for Windows though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked how is this implemented in YARP (a library that we use that I am 100% sure that supports building as static library in Windows) and apparently the approach used is the second one that you suggested : https://github.com/robotology/yarp/blob/master/conf/template/yarp_config_api.h.in .
Basically the exportdecl.h equivalent file is configured at cmake time and proper definition is added with a #cmakedefine command.

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

I added distinct config.h file instead of configuring exportdecl.h because I prefer to use config.h file to includes all the definitions configured at cmake time. But I'm fine with using exportdecl.h for if this is not a preferable way to urdfdom.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess your approach is sound. 👍

endif()

# This shouldn't be necessary, but there has been trouble
# with MSVC being set off, but MSVCXX ON.
if(MSVC OR MSVC90 OR MSVC10)
Expand Down
8 changes: 4 additions & 4 deletions urdf_parser/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
include_directories(include)

add_library(urdfdom_world SHARED src/pose.cpp src/model.cpp src/link.cpp src/joint.cpp src/world.cpp)
add_library(urdfdom_world src/pose.cpp src/model.cpp src/link.cpp src/joint.cpp src/world.cpp)
target_link_libraries(urdfdom_world ${tinyxml_libraries} ${console_bridge_LIBRARIES} ${Boost_LIBRARIES})
set_target_properties(urdfdom_world PROPERTIES SOVERSION 0.3)

add_library(urdfdom_model SHARED src/pose.cpp src/model.cpp src/link.cpp src/joint.cpp)
add_library(urdfdom_model src/pose.cpp src/model.cpp src/link.cpp src/joint.cpp)
target_link_libraries(urdfdom_model ${tinyxml_libraries} ${console_bridge_LIBRARIES} ${Boost_LIBRARIES})
set_target_properties(urdfdom_model PROPERTIES SOVERSION 0.3)

add_library(urdfdom_sensor SHARED src/urdf_sensor.cpp)
add_library(urdfdom_sensor src/urdf_sensor.cpp)
target_link_libraries(urdfdom_sensor urdfdom_model ${tinyxml_libraries} ${console_bridge_LIBRARIES} ${Boost_LIBRARIES})
set_target_properties(urdfdom_sensor PROPERTIES SOVERSION 0.3)

add_library(urdfdom_model_state SHARED src/urdf_model_state.cpp src/twist.cpp)
add_library(urdfdom_model_state src/urdf_model_state.cpp src/twist.cpp)
target_link_libraries(urdfdom_model_state ${tinyxml_libraries} ${console_bridge_LIBRARIES} ${Boost_LIBRARIES})
set_target_properties(urdfdom_model_state PROPERTIES SOVERSION 0.3)

Expand Down