search

Refinitiv / Real-Time-SDK

Other
186 stars 129 forks source link

Improve dependency management #244

Closed OleksandrKvl closed 6 months ago

OleksandrKvl commented 1 year ago

Seems that RTSDK makes some assumptions which makes it hard to integrate with our internal code base that uses Conan for dependency management. Below are the issues I've got so far.

  1. addExternal_zlib.cmake:501 relies on ZLIB_VERSION_STRING variable which is legacy/deprecated. You should use ZLIB_VERSION instead which is provided by the standard find_package() call (or at least support both). ZLIB_VERSION_STRING seems to be provided only by FindZLIB which is not the only way to get zlib, it doesn't work with Conan because it sets only standard variables.
  2. https://github.com/Refinitiv/Real-Time-SDK/blob/11d3722a87c1a16ab8d59be530bcefd282e53ab2/Cpp-C/Eta/Impl/Codec/CMakeLists.txt#L399 relies on $<TARGET_FILE:ZLIB::ZLIB>. This doesn't work with Conan because it provides zlib as an INTERFACE library which doesn't have any direct artifacts. It's not clear why client even need that librssl_tmp.arch, if this is a part of some installation routine then it should not be performed during configuration/build steps.

For now I've solved those problems simply by installing RTSDK (using FetchContent) before including Conan generated files. But this works only because we are using old way of consuming Conan dependencies via include(${CMAKE_BINARY_DIR}/conan_paths.cmake) directly in our CMake file. Modern way is to consume it via CMake toolchain file. In that case there's simply no way for the above solution. Honestly, I don't know if everything will work as expected in this project because now I got some dependencies from RTSDK and I'm not sure how well they will work with the rest of the project. Please consider to use more canonical ways of consuming dependencies to be friendly to different approaches.

L-Karchevska commented 1 year ago

@OleksandrKvl Thank you for submitting this issue! We will inestigate it and deal with it in the subsequent releases.

MitchellKato commented 6 months ago

We do not use Conan with our testing, and, unfortunately, can not guarantee compatibility with any given framework. As such, We're closing this issue.

OleksandrKvl commented 6 months ago

I didn't ask you to do any Conan-specific change but to use things in a more standard way to make it more friendly to whatever approach people use.