-
Notifications
You must be signed in to change notification settings - Fork 175
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
[Core] Enable build as static library #1584
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this issue @DownerCase . Please see my comments.
What remains is, that users need to ensure that no application links eCAL twice (e.g. multiple "copies" of eCAL in different shared libraries / executables.
The configuration of static vs shared libraries always gives me a headache. In the very end, every executable should decide for all its dependencies. And we should avoid pre-build dependencies / installers for libraries😉
"Enable the eCAL Npcap Receiver (Win10 performance fix for UDP communication)" | ||
${ECAL_NPCAP_SUPPORT} | ||
) | ||
include(ecal/ecal-core-options.cmake) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this will work (including the core options). The idea was not to give every core option as an eCAL option, as that would lead to an explosion of build options. Rather we set some options to fixed values.
On the other hand, core can be build standalone in different variations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is fine until you want to turn a core option on/off but can't because full-fat eCAL is hardcoding its value. As we would be in this case with the static core, we'd either need a top level eCAL option for this setting that just sets the core equivalent or duplicate the core option.
I'll put it back if you can say how to make this an available option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for a full eCAL build, some of the core options need to be set in specific ways. e.g. we will will always need publisher / subscriber / client / service, otherwise the apps will not build.
This is why I don't want e.g. to see ECAL_CORE_MONITORING
as a user option, because then the complete build would not work.
For other options we might want generic eCAL options.
e.g. we map set(ECAL_CORE_BUILD_SAMPLES ${BUILD_SAMPLES})
In general, I'm not 100% happy with our CMake configurations at the moment. For the long run, I would like to be able to build each and every component individually, and that's on the longer term roadmap.
if(NOT eCAL_IS_SHARED) | ||
find_dependency(tcp_pubsub) | ||
find_dependency(ecaludp) | ||
endif() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part needs to be moved to the bottom of the file, see https://gitlab.kitware.com/cmake/cmake/-/issues/25958.
Or we could just write this as
include("${CMAKE_CURRENT_LIST_DIR}/helper_functions/ecal_add_functions.cmake")
but I can do this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah. Don't rely on PACKAGE_PREFIX_DIR
its undocumented and as you've seen when they changed it in 3.29 initially they broke a lot of packages on vcpkg... 😅
Also, you can't move the find_dependency
calls to the bottom. You need to do them before creating your targets otherwise you get "target references other target which does not exist" errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you can move them to the bottom. CMake checks only after everything is parsed. (I did this for a workaround with protobuf 3.26, which uses abseil, and created the problems described in the issue.)
Also eCAL doesn't use PACKAGE_PREFIX_DIR
but its own relative variables, which CMake then expands...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you can move them to the bottom. CMake checks only after everything is parsed. (I did this for a workaround with protobuf 3.26, which uses abseil, and created the problems described in the issue.)
I tested the change yesterday and I was getting errors, had to move it back up or do find_package(protobuf)
first... 😕
Also eCAL doesn't use
PACKAGE_PREFIX_DIR
but its own relative variables, which CMake then expands...
Yeah, I was looking at the expanded version... Using CMAKE_CURRENT_LIST_DIR
is what the docs use https://cmake.org/cmake/help/v3.29/guide/importing-exporting/index.html#id8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use CMAKE_CURRENT_LIST_DIR
instead of @PACKAGE_eCAL_install_cmake_dir@
and i will create a separate PR for that.
@@ -558,26 +563,28 @@ target_link_libraries(${PROJECT_NAME}_c ${PROJECT_NAME}) | |||
|
|||
target_compile_definitions(${PROJECT_NAME}_c | |||
INTERFACE ECAL_C_DLL | |||
PUBLIC | |||
ASIO_STANDALONE | |||
ASIO_DISABLE_VISIBILITY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the asio defines are there for a reason, though I don't remember what that reason is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just deduplicated it, they're still on core, core_c inherits them from the public interface of core which it is linked to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense 👍
Thanks for the quick review!
I seem to remember seeing this sentiment from eCAL before, but I don't understand? Could you give a more detailed example of your concern regarding this? Static vs Shared linkage does get painful when a package supplies both simultaneously, but we're not doing that. The focus here is to give downstream users an option for eCAL to be built as a static library. Primarily I see this being used in package managers (eg: vcpkg/Conan) where users expect the option to consume, either a single package or all packages, statically or dynamically based on their goals. Also, I tested communication between one application built with shared library and one built with static. No observed problems, it worked just fine, as you'd expect. |
Let's take a look e.g. at the monitor plugins. If every monitor plugin were to link eCAL statically and then the monitor loads the different plugins, you've got eCAL let's say in three different plugins. Per se, this is not a problem, but eCAL is working a lot with global variables. So before, due to the nature of shared objects, it was made sure that certain things (binding to UDP, opening certain resources, ...) where done only once per process, but now they'd be done three times. |
That's a good point, the plugin system is not something I have used so didn't cross my mind. Though I just had an experiment to see what actually currently happens:
So maybe it would make sense to put a poison pill in the plugin libraries to detect the |
Hadn't forgotten about this, but it has been longer than I thought! TLDR: Yup, problem is hard. API redesign is a better answer. Assuming the following situation: In this case the main executable and dependency would have their own copies of eCAL's global variables, and would both need to do the initialization logic. If the shared library expects the parent program to do this for it, we would break that assumption and the shared library would not work, as it can not see that the main program (which linked statically) has done the initialization. Given eCAL's use of globally scoped variables/resource management the only way (that I know of) to ensure there is only one instance within the program is to have that global state be part of a shared object, exactly as you do currently. Potentially this global state could be split out from the rest of the library but that sounds like more of a band-aid solution that doesn't really solve the issue. From this we deduce two things:
With these rules established, the next question (as was so rightly asked at the start) is of enforcement; how to prevent these rules from being broken. Whilst I do not believe these problems would be common, I can not in good mind continue to push for this in its current state. I want to make things easier (eg: adding to vcpkg, PyPi) for everyone, not add additional problems to you (the maintainers) just to gain some extra flexibility for niche use cases. As such, I will mark this PR as draft awaiting a response and likely close it entirely afterwards. PS: API Redesign for eCAL 6? |
Description
Adds the opt-in possibility to build the eCAL Core libraries as static rather than always being shared libraries.
This is mainly an ecosystem focused change to enhance use where users may desire a static-only build for standalone executables.
The main changes are:
ECAL_CORE_BUILD_SHARED
ON
so existing behaviour is unchangedeCAL::core
andeCAL::core_c
ECAL_API
definition inecal_os.h
to work with both static and shared library types based on the new macro definitionECAL_STATIC
ecal-core-options.cmake
CAVEATS:
ecaludp
andtcp_pubsub
packages to be available for linking; the CMake config has been updated to reflect this.Related issues
Fixes #549
Cherry-pick to