search

conan-io / conan

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

[bug] Duplicate dependencies created by CMakeDeps #16498

Closed mgo80 closed 3 weeks ago

mgo80 commented 5 months ago

Describe the bug

I think i found a bug in CMakeDeps. My setup is as follows:

Operating System: Debian GNU/Linux 12.5 Conan version 2.4.1 cmake version 3.29.5 compiler: custom made (crosstool-ng) gcc 10.3 but i think this is not relevant for the bug

The symptom of this bug is an unspecific linker failure (ld.gold fails with "internal error"). I found out, that this is caused by the sheer amount of static libs passed to the linker. In our setup most of the libs come from conan packages and contains a lot of duplicates. I believe this is a potential bug that occurs with both Ninja and Unix Makefiles CMake generators.

After trying to find the root cause i could find this particular code in cmakedeps_macros.cmake, which is one of the CMakeDeps generated files:

if(NOT TARGET ${_LIB_NAME})
    # library_type can be STATIC, still UNKNOWN (if no package type available in the recipe) or SHARED (but no windows)
    add_library(${_LIB_NAME} ${library_type} IMPORTED)

The condition if (NOT TARGET) should recognize already existing targets but it does not as add_library(...IMPORTED) will use directory scope, as it has not the GLOBAL parameter. In other words: The target with the very same name is created multiple times in different (directory) scopes. Adding the GLOBAL parameter to add_library will make the problem vanish. But maybe has other implications. One implication seem to be an increased time consumption for the cmake call (both, configure and generating times). The very same code appears multiple times in cmakedeps_macros.cmake but also in the other generated files for each conan package. Not sure if the other places have similar implications.

So i would appreciate if you can take a look and share your opinion on that topic.

If you need more information, please let me know. I am happy to provide it.

Thanks in advance and many greetings Mirko

How to reproduce it

To reproduce the issue i would recommend the following:

The actual linker issue is rather hard to reproduce as our setup contains a quite large amount of code and dependencies. But there is a way to check the actual cause of this. See the minimal example here: https://github.com/mgo80/conan_cmakedeps_debug. There i tried to create a minimal cmake setup and copied the generated files from CMakeDeps into a module folder. The important part here is that at least 2 cmake calls to find_package exist in 2 different directories where none of them is a super/sub directory of the other. So i put them next to each other on the same level. And in these 2 directories libraries need to be defined which actually depend on the conan libary (here i chose openssl).

The repo has 3 branches:

Looking at the ninja browse tool (ninja - t browse) will then show the redundant/duplicate libraries like so:

image

You can see the libssl.a and libcrypto.a twice here. Also i tried using the GLOBAL parameter in the mentioned cmake code in branch self_contained_fix and then the duplicates vanish:

image

I also think that this potential fix will not harm the link order, but maybe has other implications, which i currently don't see. At least it worked in our actual more complex environment as the actual symptom with the intern linker error did not occur anymore.

jcar87 commented 4 months ago

hi @mgo80, thank you for reporting this and for taking the time to create a minimal reproducible example.

I think this is the default behaviour in CMake - where the targets created by find_package are not global - If I were to venture a guess, the same would be observed if we were relying on the FindOpenSSL provided by cmake, rather than the ones generated by Conan.

Have you confirmed by inspecting the command line that the linker is indeed receiving the same libraries multiple times? If the gold linker is crashing, this would seem like a bug in the linker - regardless of what is happening with CMake/Conan.

If there is something to fix is Conan, I would like to confirm if CMake behaves the same, in abscence of conan-generated files.

I can suggest other things, in the meantime:

I'd say we would be hesistant to declare global targets from Conan, if that isn't the default equivalent behaviour elsewhere.

mgo80 commented 4 months ago

First of all: thank you very much for working on the ticket and having a look!

Have you confirmed by inspecting the command line that the linker is indeed receiving the same libraries multiple times?

Yes, i can confirm that explicitly. For my example the linker command line looks like this:

/usr/bin/c++ -O3 -DNDEBUG main/CMakeFiles/main.dir/main.cpp.o -o main/main -L/home/<snip>/cmake5/modules/../openssl/lib -Wl,-rpath,/home/<snip>/cmake5/modules/../openssl/lib lib/liblib.a openssl/lib/libssl.a openssl/lib/libcrypto.a openssl/lib/libssl.a openssl/lib/libcrypto.a -ldl -lrt -Wl,-Bstatic -latomic -Wl,-Bdynamic -lpthread

and with the "global" option:

/usr/bin/c++ -O3 -DNDEBUG main/CMakeFiles/main.dir/main.cpp.o -o main/main -L/home/<snip>/cmake5/modules/../openssl/lib -Wl,-rpath,/home/<snip>/cmake5/modules/../openssl/lib lib/liblib.a openssl/lib/libssl.a openssl/lib/libcrypto.a -ldl -lrt -Wl,-Bstatic -latomic -Wl,-Bdynamic -lpthread

If the gold linker is crashing, this would seem like a bug in the linker - regardless of what is happening with CMake/Conan.

Yes, of course that is a bug in the linker. Unfortunately we use a quite old version and i still need to check if this bug has been fixed later or if i need to file a bug there...

If there is something to fix is Conan, I would like to confirm if CMake behaves the same, in abscence of conan-generated files.

I think if one would use imported targets like in the generated code it would result in the behavior. Not sure what you exactly mean.

I can suggest other things, in the meantime:

if you have a project where you know OpenSSL is required by multiple parts of the tree, you can move the calls to find_package at a root/common level to both subtrees.

We did that in the meantime, thanks for the proposal.

You can simply set CMAKE_FIND_PACKAGE_TARGETS_GLOBAL to ON (docs here) - to achieve the behaviour you need (global targets).

This i haven't seen in the docs yet. Thanks for pointing that out.

I'd say we would be hesistant to declare global targets from Conan, if that isn't the default equivalent behaviour elsewhere.

In the meantime i also think that making these targets global is not a good solution here. Considering 2 places trying to call find_package with disjoint sets of components would actually contradict global targets. Frankly i also don't see an easy solution here but i have probably not enough knowledge in cmake. If you have some other ideas here, i would appreciate that.

memsharded commented 3 weeks ago

Hi @mgo80 @jcar87

I have been reviewing this thread while working on https://github.com/conan-io/conan/pull/16964, a new CMakeDeps generator where we will have a chance to improve some things in the CMake generation.

But based on your feedback I think this would be the summary:

I think if one would use imported targets like in the generated code it would result in the behavior. Not sure what you exactly mean.

I understand this is what you have confirmed in the first last of your last comment, @mgo80, that the default CMake behavior is to have duplicated linked libraries, and CMake is not using GLOBAL imported targets. The question was to clarify this, because it is important for Conan to be as close as possible to the default CMake (and any other build tool) behavior, deviating from it causes confusions to users if not unexpected problems.

To summarize, I think the conclusions are that the recommended ways to approach this is to use the CMake ways:

I am closing this ticket as solved then, but please don't hesitate to re-open or create new tickets if there are any further questions or things to discuss. Thanks very much for your feedback!