eclipse-ecal / ecal

📦 eCAL - enhanced Communication Abstraction Layer. A high performance publish-subscribe, client-server cross-plattform middleware.
https://ecal.io
Apache License 2.0
844 stars 175 forks source link

eCAL vcpkg #359

Closed niclar closed 2 years ago

niclar commented 3 years ago

Hi, would you consider adding a vcpkg build (for the ecal libraries not the gui & thirdparty libraries) ? -That would greatly ease the build and integration process on windows & linux.

Kerstin-Keller commented 3 years ago

Hi @niclar,

in general we're not opposed to adding eCAL to any dependency manager. However, we do not have any experience with vcpkg whatsoever, and it's not so much in our scope. We're happy to give guidance and accept contributions for adding eCAL to vcpkg.

Does it require our repo to have any packaging files, or is there a central repository with recipes? In general, our build is based purely on CMake and highly configurable, such that it's possible to consume external dependencies and build the sdk only.

niclar commented 3 years ago

Hi, it's a central repository with recipes based on cmake.

I'm trying to create a ecal port but I run in to the same ecal cmake recursion issue as reported in the vcpkg project; https://github.com/microsoft/vcpkg/issues/11307

With portfile.cmake like;

vcpkg_from_github(
    OUT_SOURCE_PATH SOURCE_PATH
    REPO continental/ecal
    REF 88d77f278f5e8f3dcb9b4443e3a4e2bc9a3cf5ce #eCAL v5.9.0
    SHA512 a483568b16ae191410d7a8cd8bfba59570be2a2f912f3fa9718fe74a4a5f3de910bed5831781e38617924da69c1cc625836df2e507a9ef7dea04e03c597de8fe
    HEAD_REF master)

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    OPTIONS
        -D_WIN32_WINNT=0x0A00
        -DHAS_QT5=OFF
        -DBUILD_SAMPLES=OFF
        -DECAL_INSTALL_SAMPLE_SOURCES=OFF 
        -DECAL_THIRDPARTY_BUILD_SPDLOG=OFF 
        -DECAL_THIRDPARTY_BUILD_TINYXML2=OFF
        -DECAL_THIRDPARTY_BUILD_FINEFTP=OFF
        -DECAL_THIRDPARTY_BUILD_TERMCOLOR=OFF
        -DECAL_THIRDPARTY_BUILD_PROTOBUF=OFF
        -DECAL_THIRDPARTY_BUILD_CURL=OFF
        -DECAL_THIRDPARTY_BUILD_HDF5=OFF
        -DECAL_LINK_HDF5_SHARED=OFF
        -DCPACK_PACK_WITH_INNOSETUP=OFF
    PREFER_NINJA)

vcpkg_install_cmake()

file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/include)

file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/lib/cmake)
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/lib/cmake)

file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/bin)
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/bin)

file(INSTALL ${SOURCE_PATH}/LICENSE.txt DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright)

git_revision_information.cmake:35 in ecal triggers the errounous recursion below in vcpkg;


  ...
  C:/src/thirdparty/vcpkg/scripts/buildsystems/vcpkg.cmake:784 (_find_package)
  C:/src/thirdparty/vcpkg/scripts/buildsystems/vcpkg.cmake:784 (_find_package)
  C:/src/thirdparty/vcpkg/scripts/buildsystems/vcpkg.cmake:784 (_find_package)
  C:/src/thirdparty/vcpkg/scripts/buildsystems/vcpkg.cmake:784 (_find_package)
  CMakeLists.txt:353 (_find_package)
  thirdparty/cmake_functions/git/git_revision_information.cmake:35 (FIND_PACKAGE)
  CMakeLists.txt:364 (git_revision_information)

Commenting out FIND_PACKAGE(Git) in git_revision_information.cmake circumvents the issue only to trigger the next recursion error; find_package(Protobuf REQUIRED) in ecalproto/CMakeFiles.txt

Kerstin-Keller commented 3 years ago

I suspect that the problem is in the following lines of code:

https://github.com/continental/ecal/blob/a8d04cb408e0bc6918b07026db0d31ffc5f7d5aa/CMakeLists.txt#L351-L355

We're using a pattern here described by Daniel Pfeifers talk at CppNow (slide 34), where find_package is redefined, to be called only when a given package is NOT included as a submodule and build with the rest of eCAL. It's a pattern that works very well, but breaks, if anyone else is also redefining the same function, which vcpkg seems to do.

What we can do is to make an option for the redefinition, or guard it somehow, but then still the build will fail, at least for CMakeFunctions. Should we make a branch to experiment there?

niclar commented 3 years ago

Thanks @Kerstin-Keller. Yes I'll make it optional and see how far it gets me.

Vcpkg can patch the build / cmakes so there's no need for a branch just yet.

Besides this recursion error there are some thirdparty dependencies (tclap, simpleini) that don't resolve cleanly but these are probably best fixed in vcpkg.

Kerstin-Keller commented 3 years ago

Ok, thanks. Let me know if you encounter any other problems. Tclap and simpleini are tricky, since they don't officially provide any CMake find packages. Hence eCAL provides some itself, they are stored here: https://github.com/continental/ecal/tree/master/cmake/Modules

niclar commented 3 years ago

FYI filed a PR https://github.com/microsoft/vcpkg/pull/19802 just now (mvp builds & runs in windows & linux) -feel free to integrate the patches or variants of them in ecal.

Kerstin-Keller commented 3 years ago

Thanks @niclar for moving this foward. I will take a look at the necessary changes and what we can incorporate into the repository!

niclar commented 3 years ago

@Kerstin-Keller can you have a look at https://github.com/microsoft/vcpkg/pull/19802 how do you want to handle the static build? It seems to me dependencies are incorrectly dynamically linked, no ?

Kerstin-Keller commented 3 years ago

Hi @niclar, as of now, we have disabled static builds for eCAL in general, to a) reduce transitive dependencies (as you know, eCAL, even the core, has quite a lot of dependencies - and if we link them statically, we don't need to distribute them with eCAL). The other point is that linking ecal twice (e.g. static linkage into an application and dll at the same time can lead to serious trouble). And then we also have the eCAL Time Plugins. You can disable them, as strictly speaking, for the average use-case, they are not required.

Are static builds a requirement for vcpackage? I will take a look at the issue, and comment there, too.

niclar commented 3 years ago

a static build is a requirement for us if nothing else.

"linking ecal twice", just remove the dynamic dependencies for the static build , make it restricted if needed and all is well defined and good.

Kerstin-Keller commented 3 years ago

In general, you might find some problems when consuming eCAL then, considering eCALConfig.cmake. Some dependencies are header only (asio, simpleini, so they won't be a problem), but I am not 100% sure if it applies to to all the dependencies.

niclar commented 3 years ago

The static dependencies required for a static ecal core is "pthread", "asio", "tclap", "simpleini", "hdf5", "protobuf"

That build works & runs just fine and would constitute a mvp static build.

That was what I originally pushed.

FlorianReimold commented 2 years ago

The discussion seems to be inactive. Can we close this issue?