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

Allow static library builds #64

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allow static library builds #64

wants to merge 4 commits into from

Conversation

jslee02
Copy link
Contributor

@jslee02 jslee02 commented Sep 30, 2015

This PR enables urdfdom to build static libraries by:

  • adding shared/static build option
  • adding URDFDOM_STATIC definition for static build
  • removing explicit build type specification from add_library(~)

This does not change the default build behavior (i.e., shared build) when the build type is not specified.

# 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. 👍

@traversaro
Copy link
Contributor

By the way I just noticed that in exportdecl.h a strange console_bridge_EXPORTS macro is used

# ifdef console_bridge_EXPORTS
... How it is possible that the dll generation works?

@scpeters
Copy link
Contributor

scpeters commented Dec 9, 2015

@jslee02 can you resolve conflicts?

# Resolved conflicts:
#	CMakeLists.txt
#	urdf_parser/CMakeLists.txt
@jslee02
Copy link
Contributor Author

jslee02 commented Dec 9, 2015

I resolved the conflicts in the latest commit.

console_bridge_EXPORTS seems to need to be specified somewhere. Do we need to add an option for console_bridge_EXPORTS like URDFDOM_STATIC?

@scpeters
Copy link
Contributor

scpeters commented Dec 9, 2015

Should console_bridge_EXPORTS be handled by the console_bridge code?

@jslee02
Copy link
Contributor Author

jslee02 commented Dec 9, 2015

I think it should be.

One concern is that the definition console_bridge_EXPORTS wouldn't be passed to the client libraries such as urdfdom. So we might need to pass the definition to console_bridge-config.cmake and
console_bridge.pc.in, or create a config.h file in console_bridge and add the definition to config.h.

Alternatively, since URDFDOM_DLLAPI is not used in urdfdom anywhere, so we could consider to remove it.

@scpeters
Copy link
Contributor

scpeters commented Dec 9, 2015

console_bridge_EXPORTS is in include/console_bridge/exportdecl.h, but I don't know why it would be in urdfdom. I'm guessing that @thomas-moulard wrote the console bridge exportdecl.h first then did a search and replace with urdfdom and committed in 75078b8, but missed a piece? We should probably replace console_bridge_EXPORTS with urdfdom_EXPORTS

@scpeters
Copy link
Contributor

scpeters commented Dec 9, 2015

cc @j-rivero we have a question about a weird console_bridge_EXPORTS symbol

@j-rivero
Copy link
Contributor

j-rivero commented Dec 9, 2015

I buy Steve's hypothesis about what Thomas did. He was the first implementing packaging for urdfdom and console_bridge.

Makes no sense to me to mix different variable names in an exportdecl file.

@scpeters
Copy link
Contributor

I've submitted #75 to fix console_bridge_EXPORTS

@thomas-moulard
Copy link
Contributor

I am not sure how I introduced this bug but I believe replacing console_bridge_EXPORTS with urdfdom_EXPORTS is the correct solution, sorry for that!

@jslee02 jslee02 changed the title Enable to build static libraries Allow static library builds Feb 26, 2018
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.

5 participants