Adds BUILD_SHARED_LIBS option#215
Conversation
|
Thank you for keeping it up! |
hefloryd
left a comment
hefloryd
left a comment
There was a problem hiding this comment.
This doesn't work on Windows? It would be great if you could fix that. Alternatively, maybe add a message and abort build if on windows and BUILD_SHARED_LIBS is set.
| ${OSHW_SOURCES} | ||
| ${OSHW_EXTRA_SOURCES}) | ||
| target_link_libraries(soem ${OS_LIBS}) | ||
|
|
| ${OSAL_SOURCES} | ||
| ${OSHW_SOURCES} | ||
| ${OSHW_EXTRA_SOURCES}) | ||
| target_link_libraries(soem ${OS_LIBS}) |
There was a problem hiding this comment.
target_link_libraries is independent of BUILD_SHARED_LIBS
| ${OSAL_SOURCES} | ||
| ${OSHW_SOURCES} | ||
| ${OSHW_EXTRA_SOURCES}) | ||
| target_link_libraries(soem ${OS_LIBS}) |
There was a problem hiding this comment.
set_property(TARGET soem PROPERTY POSITION_INDEPENDENT_CODE ON)?
There was a problem hiding this comment.
Ignore that, seems -fPIC is set automatically when building SHARED so this is not needed.
| target_link_libraries(soem ${OS_LIBS}) | ||
|
|
||
|
|
||
| if(BUILD_SHARED_LIBS) |
There was a problem hiding this comment.
Is this needed? Seems like it would suffice to just set BUILD_SHARED_LIBS to TRUE.
https://cmake.org/cmake/help/v3.0/variable/BUILD_SHARED_LIBS.html#variable:BUILD_SHARED_LIBS
There was a problem hiding this comment.
You had add_library(soem STATIC
If the word STATIC is removed, then yes, it suffices to set that option to TRUE, it is global
I don't know, but it should, what it's added does not have anything to do with the OS, as far as I am concerned |
|
There is an alternative, and it is to build both using something like: add_library(soem STATIC
${SOEM_SOURCES}
${OSAL_SOURCES}
${OSHW_SOURCES}
${OSHW_EXTRA_SOURCES})
target_link_libraries(soem ${OS_LIBS})
add_library(soem_shared SHARED
${SOEM_SOURCES}
${OSAL_SOURCES}
${OSHW_SOURCES}
${OSHW_EXTRA_SOURCES})
target_link_libraries(soem_shared ${OS_LIBS})
set_target_properties(soem_shared PROPERTIES
OUTPUT_NAME soem)Installing a renamed target works as well, for instance this: install(TARGETS soem_shared DESTINATION ${SOEM_LIB_INSTALL_DIR})copies the @hefloryd Does that look like better way? Is there any inconveniente to have both static and shared libraries installed? If so, I can close this PR and prepare another one (considering your comments here as well) |
I don't think there is any inconvenience really but it would probably break the windows build (this PR doesn't work there, see https://ci.appveyor.com/project/hefloryd/soem/build/23). It may also break other (embedded) targets that don't support shared libraries. I would suggest just adding the BUILD_SHARED_LIBS option, defaulting to OFF, and removing the word STATIC. Looks like that would be enough since cmake seems to handle the rest automatically. |
|
I tried the approach above building both static and shared simultaneously.. using appveyor results in a successful build, check it out: What do you think? |
|
That's interesting. I am not sure but I think there may be a problem with the build - you might be overwriting the static library with the import library for the dll, hence the "up-to-date" message for soem.lib in the install section (if cmake decides based on timestamps, if they are actually identical it wouldn't matter of course. I don't know windows well enough to say for sure). Also, what would happen with targets that don't support shared libraries? I don't see how cmake would be able to proceed with the shared lib section (unless it just skips it). Just adding the BUILD_SHARED_LIBS option seems like a safer change. |
|
Hey, I guess there are no too many windows users watching this repo to give a hand on the matter Just read a bunch of posts around, some of them mentioning the DLL hell, to finally agree: it is safer to add an option default to OFF Probably the most interesting post, if anyone is interested: I'll update the PR with your comments then, thanks |
|
Hi there, it's been a while. I'm not completely sure of my own PR after two years... :) We definitely use it in shared mode, but I guess it's static also to have a tiny bit more of runtime performance and avoid So, I'm gonna close this PR, and actually consider to link statically this lib. Thanks! |
3 participants
It is basically the same as #89 by @thopiekar but configured opt-out to keep the current default build procedure as static and not break pipelines around
It also provides feedback about the install prefix, since it is usually
/usr/localand notCMAKE_SOURCE_DIRSuggestions welcome!