Skip to content
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

[FR] Allow linking against external mbedtsl #90

Open
werdahias opened this issue May 13, 2023 · 5 comments
Open

[FR] Allow linking against external mbedtsl #90

werdahias opened this issue May 13, 2023 · 5 comments

Comments

@werdahias
Copy link

Hi,

I started packaging this library for debian as prerequisite for openrgb. I encountered a few issues stemming from the embedding dependencies. No package in debian should depend/ship external libraries.

  • json.hpp : Could you maybe add a check to find the system version first, then defaulting to the included copy ?
  • mbed-tls: same issue basically. Otherwise I get the following warnings during the build:
dpkg-shlibdeps: warning: symbol mbedtls_ctr_drbg_random used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_entropy_func used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_timing_set_delay used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_timing_get_delay used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_x509_crt_init used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_entropy_free used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_ctr_drbg_init used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_entropy_init used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_ctr_drbg_seed used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries
dpkg-shlibdeps: warning: symbol mbedtls_ctr_drbg_free used by debian/libhueplusplus-dev/usr/lib/libhueplusplusshared.so found in none of the libraries

because the mbdedtls library from debian isn't picked up (and linked).

The tests aren't buildable offline either. I'd appreciate if you could employ a similar check because gtest and gmock are both in debian.

@Jojo-1000
Copy link
Collaborator

Hi, I implemented find_package for external libraries on the external-deps branch.
Could you please test that it correctly links to the external dependencies? I only tested that it still works without installed libraries.

@werdahias
Copy link
Author

werdahias commented May 17, 2023

awesome, thank you! Unfortunately the version of mbedtls in debian is to old so I can't test the build. Once that gets updated I'll report back.

@werdahias
Copy link
Author

since mbedtls 3.x is available in debian I will test this soon

@thekhalifa
Copy link

Hi, I started working on this debian package over the external-deps branch and noticed a couple of things:

  1. find_package results are stored in a case-sensitive variable. I think it's because MbedTLS use a mixed-case cmake file, so I patched these two lines in CMakeLists.txt to achieve what you intended:
-set(NEED_SUBMODULES NOT (${mbedtls_FOUND} AND ${nlohmann_json_FOUND}))
+set(NEED_SUBMODULES NOT (${MbedTLS_FOUND} AND ${nlohmann_json_FOUND}))

-if(NOT mbedtls_FOUND)
+if(NOT MbedTLS_FOUND)

Later in src/CMakeLists.txt you use the right case, so that's good.

  1. The output SO should be named/tagged with an ABI version of 1, so I patch src/CMakeLists.txt to change that slightly. the end result should be 'libhueplusplus.so.1' and is tagged with the same value as SONAME along with symlinks. CMake takes care of those with this change:
-install(TARGETS hueplusplusshared DESTINATION lib)
+set_target_properties(hueplusplusshared PROPERTIES OUTPUT_NAME hueplusplus SOVERSION 1)
+install(TARGETS hueplusplusshared DESTINATION ${CMAKE_INSTALL_LIBDIR})

-install(TARGETS hueplusplusstatic DESTINATION lib)
+set_target_properties(hueplusplusstatic PROPERTIES OUTPUT_NAME hueplusplus)
+install(TARGETS hueplusplusstatic DESTINATION ${CMAKE_INSTALL_LIBDIR})

You can ignore CMAKE_INSTALL_LIBDIR outside debian packaging, but it puts puts the output into 'lib/x86_64-linux-gnu' instead of lib only.

The only question I had was about your long-term branch here. Basing off of 'external-deps' seems temporary, do you have plans to merge those changes into master branch or should I assume external-deps as the long term branch?

A new release tag would also be helpful if you had plans for that, as the library versioning using a git commit is a bit less readable (comes out like git-describe or has YYYYMMDD in it).

@Jojo-1000
Copy link
Collaborator

Hi @thekhalifa, good to see that you are working on this. The reason why there was no merge in all this time is because the changes were not tested yet. I am going to make a pull request with your changes applied, so you can see if all the changes are good. Then we can make a release for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants