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

System library dependencies could be specified in the wrong order #2382

Closed
bschindler opened this issue Jan 25, 2018 · 6 comments · Fixed by #5582
Closed

System library dependencies could be specified in the wrong order #2382

bschindler opened this issue Jan 25, 2018 · 6 comments · Fixed by #5582

Comments

@bschindler
Copy link

Attached is a case which reproduces my issue, just issue conan create . test/test

Description: So hidapi depends on libusb and it adds it in cpp_info. The problem however is, that cmake does not really know that hidapi depends on libusb as they are just added using cpp_info.libs=["hidapi", "usb-1.0"]. This information is lost. Now, like this, the libs are specified in correct order so everything is fine. But as soon as a user of hidapi itself links against usb-1.0, cmake changes the link order causing undefined references.

So, if the pkgconfig block in test_package/CMakeLists.txt is commented out, the package builds fine. With the pkgconfig block, the lib fails to compile here on Ubuntu LTS 16.04 with undefined references to libusb because cmake links in reverse order (-lusb-1.0 -lhidapi). See https://stackoverflow.com/questions/11893996/why-does-the-order-of-l-option-in-gcc-matter for why this matters
Attached below is a reproducible case
hidapi.zip

@memsharded
Copy link
Member

Thanks very much for your reproducible case, it did really help.

I have managed to make it work, thanks to the data exposed in conanbuildinfo.cmake:

In test_package/CMakeLists.txt

find_package(PkgConfig REQUIRED)
pkg_check_modules(LIBUSB REQUIRED libusb-1.0)

include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
set(CONAN_LIBS_HIDAPI ${CONAN_LIBS_HIDAPI} ${LIBUSB_LIBRARIES})
conan_basic_setup(TARGETS)

add_executable(${PROJECT_NAME} example.cpp)
target_link_libraries(${PROJECT_NAME} PRIVATE CONAN_PKG::hidapi)
target_include_directories(${PROJECT_NAME} PRIVATE ${LIBUSB_INCLUDE_DIRS})
target_compile_options(${PROJECT_NAME} PRIVATE ${LIBUSB_CFLAGS_OTHER})

The source of this linkage error is that we are mixing 2 different package managers: conan and pkg-config, via cmake. and they are not aware of each other. Then the solution is to let know conan about the libusb dependencies as defined by pkg-config.

Another relatively elegant solution would be to wrap the libusb pkg-config in a conan recipe that abstracts the details (so it can be implemented differently in another OS that doesn't have libusb native or doesn't have support for pkg-config).

@bschindler
Copy link
Author

bschindler commented Jan 26, 2018

Thanks for the workaround, but to me, you are not getting to the bottom of the issue. I printed CONAN_LIBS_HIDAPI before and after the set:
hidapi;usb-1.0;pthread
hidapi;usb-1.0;pthread;usb-1.0

Even if this fixes the issue, this is very circumstancial. I digged a bit further and the issue has nothing to do with using 2 package managers. The second package manager just exposes an issue that exists within conan: The problem is that there is no way of expressing the dependency hidapi->usb-1.0. The recipe writes cpp_info.libs=["hidapi", "usb-1.0"] as if there are two independent libraries creating the dependency chain test->hidapi and test->usb-1.0. This is however wrong as it should read test->hidapi->usb-1.0.
In the generated conanbuildinfo.cmake, there is a function conan_package_library_targets. It in theory supports a deps argument to specify dependencies to your target but I see no way of getting libusb into that list. Both hidapi and ubs-1.0 are specified in the first argument, creating the dependency chain mentioned above.
So no, this has nothing to do with pkg-config.

To demonstrate, I did the following change to the generated conanbuildinfo.cmake (in the test_package):
-set(CONAN_LIBS_HIDAPI hidapi usb-1.0 pthread)
+set(CONAN_LIBS_HIDAPI hidapi)
+set(CONAN_DEPS_HIDAPI usb-1.0 pthread)

-conan_package_library_targets("${CONAN_LIBS_HIDAPI}" "${CONAN_LIB_DIRS_HIDAPI}"
CONAN_PACKAGE_TARGETS_HIDAPI "" "" hidapi)
+conan_package_library_targets("${CONAN_LIBS_HIDAPI}" "${CONAN_LIB_DIRS_HIDAPI}"
CONAN_PACKAGE_TARGETS_HIDAPI "${CONAN_DEPS_HIDAPI}" "" hidapi)

And the package builds just fine as cmake is now aware of the hidapi->usb dependency, even with pkg-config enabled. So, this is an issue in conan. What is needed is a facility within pacakge_info to tell conan that hidapi depends on libusb which seems currently not possible.

Cheers

@memsharded
Copy link
Member

So no, this has nothing to do with pkg-config.

It has nothing to do with pkg-config per-se. It has to do with the fact that we are mixing 2 different sources of information about dependencies. One is provided by conan, and the other one is provided by the system from .pc files collected by package config.

And yes, so far conan doesn't model intra-package dependencies. The suggested approach so far would be to wrap libusb-1.0 in a recipe. That wouldn't be difficult, and would be an elegant solution, if you want some guidance how to approach it, I'd like to help.

We are considering to add some functionality to model inter-package dependencies. It is not a simple feature, and might take some time. This is the issue for it: #2387. Please follow progress there, and this one might be closed if you are fine with the workaround.

@bschindler
Copy link
Author

I don't mind creating a libusb package just for this particular case, but I just don't think this scales as a general solution as the amount of system packages that may need wrapping is gigantic. So to be frank, I don't think that is a preferrable solution as we'd be duplicating the work done by the distributions.

It has nothing to do with pkg-config per-se. It has to do with the fact that we are mixing 2 different sources of information about dependencies.

No, that's wrong. If conan creates an incorrect dependency chain, the dependency chain is incorrect. Just because another package manager is used to make the problem surface doesn't make it a problem of the interaction between the two. It could just as well happen that you use another package in conan using libusb and the same issue could happen. I haven't attempted to do this, but I'm fairly confident it can be done purely using conan.

@memsharded
Copy link
Member

If conan creates an incorrect dependency chain, the dependency chain is incorrect.

In this case conan is not the one creating an incorrect dependency chain. This incorrect dependency chain is defined by CMake, the CMakeLists.txt inside test_package, which is adding link libraries to the target without conan knowing.

It could just as well happen that you use another package in conan using libusb and the same issue could happen. I haven't attempted to do this, but I'm fairly confident it can be done purely using conan.

Please try to do so, it will be very valuable in case you manage to make it fail with pure conan. Conan respects the relative order, so when several packages include usb-1.0, the merging of self.cpp_info.libs will respect that order, so it shouldn't fail. But of course it is possible that there is some bug or corner case that fails, so looking forward to getting an issue if this happens, many thanks!

@bschindler
Copy link
Author

In this case conan is not the one creating an incorrect dependency chain. This incorrect dependency chain is defined by CMake, the CMakeLists.txt inside test_package, which is adding link libraries to the target without conan knowing.

Sorry no... the dependency chain exported by the hidapi package is wrong. The correct dependency chain for the hidapi package is test->hidapi->libusb but what conan exports in conanbuildinfo.cmake is test->hidapi and test->libusb. These two are different and thus what I'm getting from conan is wrong. So one could claim that the conan package is wrong in that it makes conan think that libusb is a library provided by conan which it is not. That would lead to the solution you suggest of creating a package for libusb. There is however a significant catch to that: Not every distro packages libraries using the same granularity, so finding a good match to whatever distro is quite some work and then in addition, should the conan community really wrap every system library there is to make conan work?
If this is how conan wants to deal with it, a huge amount of packages currently around would have to be changed and a lot of new ones would have to be created. Is this really what you want?

Lets assume test happened to use libusb on its own (using a conan referring to the same library) and also use hidapi (unchanged recipe!), then the same issue should arise which is what I'm attemtping to do right now, however I run into what seems to be a crash on conan's side with the attached dummy package (v1.0.3):

Traceback (most recent call last):
File "conan/conans/client/command.py", line 1099, in run
File "conan/conans/client/command.py", line 254, in install
File "conan/conans/client/conan_api.py", line 63, in wrapper
File "conan/conans/client/conan_api.py", line 402, in install
File "conan/conans/client/manager.py", line 369, in install
File "conan/conans/client/installer.py", line 282, in install
File "conan/conans/client/installer.py", line 396, in _build
File "conan/conans/client/installer.py", line 437, in _propagate_info
File "conan/conans/model/build_info.py", line 184, in update
File "conan/conans/model/build_info.py", line 122, in update
File "conan/conans/model/build_info.py", line 115, in merge_lists
TypeError: can only concatenate list (not "str") to list

I'm a bit lost at this point so I could use some help...

conanfile.py.txt

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