feat: improved library build system when building as third party #2298
Conversation
Added aliases for targets avoiding multiple definitions with foreign code. Minor fixes: missing semicolons and multiple 'tls_callback_func' definitions when building as a static library. Signed-off-by: Davide Bertelli <[email protected]>
…ned logic of target linking.
…g built as a static library
eboasson
left a comment
eboasson
left a comment
There was a problem hiding this comment.
Thank you very much @Davide-1998 ! I am not much of a CMake user and I have never gotten around to trying to solve some vague reports about difficulties building it as a sub-project. So it is really nice to see a PR for addressing those issues.
It looks good to me, even if I there are some things I am not sure of yet. I'd appreciate it if you could help me with them.
src/ucunit/CMakeLists.txt
Outdated
| LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}" COMPONENT lib | ||
| ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}" COMPONENT lib) | ||
| endif() | ||
| install( |
There was a problem hiding this comment.
I remember adding this install. I didn't really want to install ucunit because I don't think it should be installed alongside Cyclone and would pollute the install dirs. However, I couldn't get the static build to work without it.
Thinking about it now, for the normal (dynamically linked) builds, you probably wouldn't want to install the version generated with -DBUILD_TESTING=1 anyway because it forces exporting all symbols.
So probably it is fine to install it always. (And perhaps it will even come in handy for someone 🙂)
There was a problem hiding this comment.
The problem with ucunit comes into being whenever BUILD_TESTING=TRUE.
This causes the dds_security_*_wrapper targets to have ucunit as their direct dependency, thus their install command will require it to be inside an export group.
It would be possible to avoid installing ucunit by forcibly require it to be a STATIC library. In this way the security wrappers will use its code without needing it to be installed and exported.
By the way, if we build it statically we would also need to enable the POSITION_INDEPENDENT_CODE for it, otherwise another relocation issue is flagged.
| set_target_properties(ddsc PROPERTIES | ||
| VERSION ${PROJECT_VERSION} | ||
| SOVERSION ${PROJECT_VERSION_MAJOR} | ||
| POSITION_INDEPENDENT_CODE ON |
There was a problem hiding this comment.
Won't this force position independent code always? I remember trying to set it to ON and running into a problem. I think it was the one mentioned here:
Line 33 in 518c546
There was a problem hiding this comment.
This property will set position indipendent code only for the DDSC target (and, in a future commit, also for CDR). I had to enable it due to relocation issues happening when building CycloneDDS as a static library on linux.
Specifically the errors were:
- relocation R_X86_64_TPOFF32 for symbol
tsd_thread_statefrom ddsi_thread.c. Fixed by adding POSITION_INDEPENDENT_CODE property on DDSC target - relocation R_X86_64_PC32 for symbol
ddsrt_hh_emptyfrom dds_cdrstream.c. Fixed by adding POSITION_INDEPENDENT_CODE property on CDR target
Seeing the shared nature of the project and the many relocation issues, I suggest to directly set CMAKE_POSITION_INDEPENDENT_CODE=ON whenever the library is built with BUILD_SHARED_LIBS=ON.
Further tests with this setting on Windows Environment are required though.
src/idl/CMakeLists.txt
Outdated
| target_link_libraries(idl | ||
| PUBLIC | ||
| ${PROJECT_NAME}::ddsc | ||
| $<$<NOT:$<STREQUAL:"${CMAKE_PROJECT_NAME}","CycloneDDS">>:${PROJECT_NAME}::cdr> |
There was a problem hiding this comment.
Does this mean that when building Cyclone as a subproject, the CDR library would be split off into a separate shared library and that each application using Cyclone from that build is linking against it directly?
I don't mind if that's the case, but I do want to make sure I understand.
There was a problem hiding this comment.
Yes indeed, when CycloneDDS will be built as a subproject, CDR will become a shared library, or static depending by the value of BUILD_SHARED_LIBS, and each application linking to CycloneDDS::ddsc will also indirectly link to CDR being one of its dependencies
…g ddsrt_init when building statically Not calling ddsrt_init triggers a debug assertion failure on TLS
…cifiers to allow cmake to chose the best one
|
Hi @Davide-1998 I really would like everyone to benefit from the cleaner CMake usage you propose. I am more or less waiting until I no longer see commits getting added 😱 I thought that might be the case, but it seems the CI is quite unhappy now. When the CI's green and you let me know it is as good as it needs to be for now, then I'll do a final review and (hopefully) merge it. Do you think that's a reasonable plan? |
|
Good afternoon. |
2 participants
Fix #2297
This PR adds the following:
src/ddsrt/src/dynlib.cwhenDDSRT_HAVE_DYNLIBis falsecdrtargetfilesystem.hheader inddsrttarget headersDDS_STATIC_DEFINEforsrc/ddsrt/src/cdtors.candsrc/idl/src/string.cto avoid multipletls_callback_funcdefinitions when building as a statically.POSITION_INDEPENDENT_CODEstatements