search

conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.13k stars 969 forks source link

[bug] Problems with CMakeDeps generator #16911

Open gouriano opened 2 weeks ago

gouriano commented 2 weeks ago

Describe the bug

Environment: Linux cmake 3.21.2 conan 2.6.0

I have a problem linking against gRPC, but the real problem seems to be in CMakeDeps generator. It fails to generate proper library settings for dependent packages (in this case, abseil and protobuf)

How to reproduce it

Take test_package.cpp source file from abseil recipe https://github.com/conan-io/conan-center-index/blob/master/recipes/abseil/all/test_package/test_package.cpp

Create CMakeLists.txt

cmake_minimum_required(VERSION 3.15)
project(test_package LANGUAGES CXX)
find_package(absl REQUIRED)
add_executable(test test_package.cpp)
target_link_libraries(test abseil::abseil)

and create the following conanfile.txt:

[requires]
grpc/1.50.1
[options]
abseil*:shared=True
grpc*:shared=True
protobuf*:shared=True
[generators]
CMakeDeps
[layout]
cmake_layout

Yes, require grpc (which requires abseil, as we know) Now run

conan install . --build missing -s build_type=Release

and look at build/Release/generators/absl-release-x86_64-data.cmake we see: set(abseil_LIBS_RELEASE ) The list is empty.

Now delete build directory, add abseil into conanfile.txt, and do "conan install" again

[requires]
abseil/20230802.1
grpc/1.50.1
[options]
abseil*:shared=True
grpc*:shared=True
protobuf*:shared=True
[generators]
CMakeDeps
[layout]
cmake_layout

Now in absl-release-x86_64-data.cmake we see

set(abseil_LIBS_RELEASE absl_flags_parse absl_log_flags .. and the long list of libraries)

I believe, it is a bug?

I do not know what is abseil and I do not care. What I need is grpc. I request grpc, I "find_package(gRPC)", and then linking fails because abseil libraries are missing

The same story is with protobuf. If it is absent in the list of requires, protobuf_LIBS_RELEASE list in protobuf-release-x86_64-data.cmake file is empty. If present, protobuf_LIBS_RELEASE list is not empty. It is so in "shared" mode only. in "shared=False", everything works as expected - LIBS_RELEASE lists are never empty

memsharded commented 2 weeks ago

This seems connected to this: https://github.com/conan-io/conan-center-index/issues/24806, it is possible that it is a recipe issuel, will investigate

memsharded commented 2 weeks ago

As described in https://github.com/conan-io/conan/issues/16898#issuecomment-2316425098, the fact that set(abseil_LIBS_RELEASE ) shouldnt be an issue if abseil is not a direct dependency.

We are trying to reproduce with a full minimal example, but so far can't.

gouriano commented 2 weeks ago

but so far can't.

Interesting... With the described steps, I reproduce it easily, all the time. If you need any additional info about my environment, let me know.

memsharded commented 2 weeks ago

Yes, we would need a full example that produces the linking errors that other users are seeing. I think it is related, but the above setup is not enough:

So what is needed is a full repro case that:

So having the full consumer conanfile, CMakeLists.txt and .cpp files, together with the exact Conan commands is what would help. I have been trying to reproduce that with the test_package of grpc, which already contain such files, but so far I haven't been able to reproduce.

SpaceIm commented 2 weeks ago

Actually I'm quite surprised now that I'm reading git history of grpc recipe. I've added transitive_libs for abseil in https://github.com/conan-io/conan-center-index/pull/17284 (it was already there for protobuf), but it has been removed later on for abseil & protobuf by https://github.com/conan-io/conan-center-index/pull/24215 (while keeping transitive_headers, which is quite weird, those libs are not header-only so if headers are public, libs are also public). Why? I even provided a link https://github.com/conan-io/conan-center-index/pull/17284#issuecomment-1526082638 which was quite self explanatory for the reason why transitive_libs was required. abseil and protobuf are public dependencies of grpc.

So actually it might be a recipe issue.

Moreover test package has been so simplified in https://github.com/conan-io/conan-center-index/pull/24215 that it can't properly test anymore this kind of link issue.

gouriano commented 2 weeks ago

I hope the following is a better example.

Take two files - test_package.cpp and helloworld.proto, from the previous version of grpc recipe https://github.com/conan-io/conan-center-index/tree/1bdbd1323367c1d36cd078bf5a122c719d857448/recipes/grpc/all/test_package

Add CMakeLists.txt:

cmake_minimum_required(VERSION 3.15)
project(testapp)
find_package(gRPC REQUIRED)
add_executable(testapp test_package.cpp)
target_link_libraries(testapp gRPC::grpc++)
protobuf_generate(TARGET testapp PROTOS helloworld.proto 
    PROTOC_OUT_DIR "${CMAKE_CURRENT_SOURCE_DIR}")
protobuf_generate(TARGET testapp PROTOS helloworld.proto 
    PROTOC_OUT_DIR "${CMAKE_CURRENT_SOURCE_DIR}"
    LANGUAGE grpc GENERATE_EXTENSIONS .grpc.pb.h .grpc.pb.cc
    PLUGIN protoc-gen-grpc=$<TARGET_FILE:gRPC::grpc_cpp_plugin>)

Add conanfile.txt:

[requires]
grpc/1.50.1
[options]
*:shared=True
[generators]
CMakeDeps
CMakeToolchain
VirtualRunEnv
[layout]
cmake_layout

Configure

conan install . --build missing -s build_type=Release
cd build/Release
cmake ../../ --preset conan-release

Build:

make
...
[100%] Linking CXX executable testapp
/usr/bin/ld: CMakeFiles/testapp.dir/helloworld.pb.cc.o: undefined reference to symbol '_ZN6google8protobuf8internal14ArenaStringPtr12ClearToEmptyEv'
//export/home/gouriano/Conan/.conan2/p/b/protob24d2b536231b/p/lib/libprotobuf.so.32: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

Add protobuf into conanfile.txt:

[requires]
grpc/1.50.1
protobuf/3.21.12
[options]
*:shared=True
[generators]
CMakeDeps
CMakeToolchain
VirtualRunEnv
[layout]
cmake_layout

configure, build. The output now says

[ 66%] Linking CXX executable testapp
/usr/bin/ld: CMakeFiles/testapp.dir/helloworld.grpc.pb.cc.o: undefined reference to symbol '_ZN4absl12lts_202308025MutexD1Ev'
//export/home/gouriano/Conan/.conan2/p/b/absei9f439b2f74c73/p/lib/libabsl_synchronization.so.2308.0.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
jcar87 commented 2 weeks ago

Apologies for the grpc/abseil case - this is a regression that I introduced when adding the new version. fix here: https://github.com/conan-io/conan-center-index/pull/25087/files

if headers are public, libs are also public

Arguably, that is an overarching generalisation which does not apply to all cases. for a library A that has transitive_headers=True on library B - transitive_libs=True would only be needed if the headers of library A contain implementation causing the consumer to require symbols/references that are only in library B. This is not the case all the time - in a lot of cases the transitive headers are require to know types, or a macro, or only reference "pointers" of things - but do not cause the consumer to reference symbols. in other cases, it may be "fully public" dependency, but one such that it is impossible for the consumer to be "unaware" that another library is transitively being used - to the point where the consumer should also be depending on the transitive dependency.

The case at hand (grpc) is different - the references are mostly in generated code (by protoc) in the consumer side - so it's neither of the cases above - in order to determine what is transitive we need to check the generated code too.

transitive_headers=True and transitive_libs=True are NOT equivalents to cmake "PUBLIC" modifier.

CMake's target_link_libraries(A PRIVATE B) translates to transitive_libs=False if B is shared, else transitive_libs=True, but transitive_headers=False.

Whereas Conan's transitive_libs=True(vs leaving it undefined) means they are always propagated when linking against "this" - regardless of library types. We tend to discourage this on purpose so that users can retain the flexibility to have a shared library that statically links a transitive dependency, causing the symbols to be provided by the shared library to consumers (such that the static library does not propagate beyond "this"). If transitive_libs=True, and "this" recipe is shared, and the transitive dependency is static, we'll end up with duplicated symbols.

From what I can see, equivalent functionality was requested in CMake here

And was implemented in CMake 3.27 as the COMPILE_ONLY genex: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#id30

thus transitive_headers=True and leaving transitive_libs undefined, is a very valid case and is the intention in Conan 2 to support it.

I do like CMake's PRIVATE/PUBLIC/INTERFACE, but in practice, they do not map 1:1 with the Conan 2 traits, they map to the low-level behaviour of CMake (LINK_ONLY, COMPILE_ONLY).

Perhaps there is a case for Conan to give the ability to express in the recipe that consumers will reference symbols from a transitive dependency that may require propagating a linking library, depending on the specific combination of shared/static that we have.

Moreover test package has been so simplified in https://github.com/conan-io/conan-center-index/pull/24215 that it can't properly test anymore this kind of link issue.

I remember testing the new recipe revision against the old test_package, as well as other consumers that existed in Conan Center, manually, precisely to avoid this kind of regression. Bad on my part, as I clearly did not test the specific platforms where this issue arises.

jcar87 commented 1 week ago

Thanks @gouriano for providing more details. The abseil build error is being fixed in https://github.com/conan-io/conan-center-index/pull/25087/files which will be merged today.

As for the protobuf errors, if you are calling protobuf_generate in CMake, and are adding the generated sources to a target, then you should directly depend on protobuf - not only in the conanfile.py, but also in target_link_libraries - which is what the old test package was actually doing.

Abseil, on the other hand is implicit and unknown to the consumer - hence the need for the fix.

gouriano commented 1 week ago

I disagree. I do not see why protobuf is different than abseil. _protobufgenerate uses a code generator, while linking uses libraries. grpc requires protobuf, always, unconditionally. When I "require" grpc, I should get correct settings for protobuf. When I define IMPORTED library in CMake, I am not supposed to know what does that require. Maybe, it requires packages X and Y, whatever. I get them all automatically. Same thing here. I link against grpc. grpc requires protobuf, I should get it automatically. Anyway, I now know a workaround, I know how to handle this. To fix it or not, is your job, to make it work - mine.

jcar87 commented 1 week ago

Using protobuf_generate on the consumer side means that the consumer is using protobuf directly and not implicitly - both at the cmake level, and the C++ level - thus, protobuf is a direct dependency and should be expressed as such. This is in line with the practices recommended by:

Linking directly with libprotobuf when using gRPC C++ with protobuf generate, is also what all the grpc examples do, which is what was missing from your code

As well as explicit calls to both find_package(Protobuf) and find_package(gRPC):

This is also what we had in in the original test package:

What we are advocating for and supporting in the recipe is to use gRPC in line with how it is in the gRPC examples and documentation. Abseil is different because it is used with (almost) zero visibility on the consumer side.

vakatov commented 1 week ago

grpc requires protobuf, always, unconditionally

FWIW, if I remember it correctly, theoretically gRPC can use other protocols (not protobuf). Not sure if anybody does that nowadays though.