[package] asio-grpc: ability to not link any library

weatherhead99 commented 1 year ago

Description

Due to an underlying issue in gRPC, infinite loops in programs can occur pretty easily if one accidentally links a program against both grpc++.so and grpc++_unsecure.so (see e.g. https://github.com/grpc/grpc/issues/24422, there are many roughly similar bugs that boil down to the same thing).

Upstream asio-grpc provides the cmake variable "ASIO_GRPC_DISABLE_AUTOLINK" which, if set, causes the find module to not link against any grpc library. One of the (documented, e.g. in https://github.com/Tradias/asio-grpc/issues/61) uses of this is to choose yourself how to link against grpc from downstream build systems.

In the conan package for asio-grpc, this variable is (of course) not honored because it uses the autogenerated CMake module functionality built into the CMakeDeps generator. However, in order to get around this, I have ended up having to put extra code in my cmake files, like e.g.

#NOTE: the below is to get around gRPC "infinite loop" bugs in their initialization
#if we are using asio-grpc directly from upstream this should work
set(ASIO_GRPC_DISABLE_AUTOLINK TRUE)
find_package(asio-grpc)

#however, if we're using the conan package, then the above is currently not supported.
#we can do the below ugly hack to mess around with the deps in that case
if(TARGET asio-grpc_DEPS_TARGET)
  message(WARNING "CONAN BUILD hack: patching asio-grpc deps...")
  set_target_properties(asio-grpc_DEPS_TARGET PROPERTIES INTERFACE_LINK_LIBRARIES
  "$<$<CONFIG:Release>:>;$<$<CONFIG:Release>:>;$<$<CONFIG:Release>:Boost::headers>")

endif()

note that this introduces a necessity to edit my own CMakeLists.txt file in order to work with conan, explicitly against the design goals of CMakeDeps/ CMakeToolchain. I see a few of ways to resolve this:

1) instead of using autogenerated CMakeDeps, package the upstream asio-grpc Find module directly and install it into the conan package, for those using CMake downstream

2) somehow add a custom block to the CMakeDeps generating part which allows this variable to be used (I don't see an easy way to do this actually/ an obvious block where to put this) this is likely preferred but I don't immediately see how to do it

3) add an extra option to the package like "no-autolink" which doesn't affect binary compatibility but alters the generate() method such that it doesn't link to grpc anymore. This way at least we can get round this problem in the consuming conanfile rather than having to alter our consuming build scripts.

Happy to work on implementation of this but I'm not sure which of the above approaches will be preferred. At the moment, if you happen to use conan asio-grpc with a program that also uses grpc in any way it's extremely easy to write a very simple program which infinitely loops. Ultimately the problem is in grpc itself but since there are many documented community workarounds I think we need one too.

Package and Environment Details

Package Name/Version: asio-grpc/2.4.0
Operating System+version: Linux Ubuntu 20.04 Linux openSUSE Leap 15.2 probably all of them, at least all linux
Compiler+version: GCC 9
Conan version: conan 2.0.6
Python version: Python 3.11.3

Conan profile

Host profile: [settings] arch=x86_64 build_type=Release compiler=gcc compiler.cppstd=gnu17 compiler.libcxx=libstdc++11 compiler.version=9 os=Linux [options] /:shared=True [conf]

Build profile: [settings] arch=x86_64 build_type=Release compiler=gcc compiler.cppstd=gnu17 compiler.libcxx=libstdc++11 compiler.version=9 os=Linux [options] /:shared=True [conf]

Steps to reproduce

write conanfile which uses asio-grpc and also grpc with a custom CMakeLists.txt

conan build . 
cd build
./example

infinite loop

Logs

None relevant

friendlyanon commented 1 year ago

There is the cmake_build_modules property for CMakeDeps: https://docs.conan.io/2/reference/tools/cmake/cmakedeps.html?highlight=cmake_build_modules
You can include custom code via that. There are existing examples in the recipes for that if you need help checking how it works.

jcar87 commented 1 year ago

Hi @weatherhead99 - thank you for reporting this issue. Indeed it is rather unfortunate that the incompatibility of grpc++ and grpc++_unsecure in the same process can only be detected at runtime. Not particularly keen on ASIO_GRPC_DISABLE_AUTOLINK either, but I can see why it is there - the moniker "autolink" suggests something similar to what happens in Boost, when in reality it is something different altogether (autolink on windows by means of including the header that has a pragma). I believe a more robust approach would have been to have a toggle for which of the two to use, rather than use none and leave the consumer to figure this out differently - this goes against the tide of leveraging modern CMake leaving the end consumer to link something.

I think the best solution and one that matches existing patterns is your number (3), that is having an option in the asio-grpc recipe, one that is evaluated in the package_info() guarding this assignment here: self.cpp_info.requires = ["grpc::grpc++_unsecure"] - so -o asio-grpc:grpc_disable_autolink=True could achieve something similar., or better yet, one that expresses whether grpc_unsecure is used (via True/False), rather than leave it without linking anything at all - Happy to open a PR with this if it helps - by the looks of it, asio-grpc 3.0 will revert to using grpc++, so we need to be mindful about that as well.

weatherhead99 commented 1 year ago

agree with this and that it fits conan pattern best. However, I think strongly we shouldn't name the option "autolink". Because, using the actual cmake autolink variable also disables linking in e.g. libunifex etc whereas this option would just be used to not automatically link a grpc library.

We could instead have an option like "grpc_target" which has values of "grpc++", "grpc++_unsercure" and None as the default which defaults to whatever the default would be in that version of asio-grpc. How does that sound?

Also happy to do the PR just wanted to get consensus on what to go for.

weatherhead99 commented 1 year ago

and p.s. it also isn't easily detectable at runtime (except by looking at it consuming 100% CPU forever and not doing anything), I think this is just a very unfortunate issue in upstream grpc all round really.

weatherhead99 commented 1 year ago

another thought: in order to maintain compatibility with "bare" CMake scripts using the ASIO_GRPC_DISABLE_AUTOLINK variable, we will need to have the option to not link anything. i.e. we don't actually want to link against grpc::grpc++ twice either.

Tradias commented 1 year ago

Not particularly keen on ASIO_GRPC_DISABLE_AUTOLINK either

This variable was added before I even knew about the gRPC secure/unsecure conflict. It's intention (aside from the poorly chosen name) is to provide an escape hatch for CMake users that provide their dependencies through other means, like in this example: https://github.com/Tradias/asio-grpc/issues/60#issuecomment-1365957537

weatherhead99 commented 1 year ago

and the philosophy of modern conan is that you CMake script should require NO modifications. This is fine because it doesn't complain about ASIO_GRPC_DISABLE_AUTOLINK not existing, but you have to be able to mimic the behaviour in conan without modifying downstream CMake recipes which in other cases would build against the direct checkout of asio-grpc and its own CMake package

conan-io / conan-center-index