Conan: CMake generator using TARGETS malfunctions with OpenCV, Protobuf, ZLib dependency chain.

[Frans-Willem]

We're using CMake with conan_basic_setup(TARGETS), which appears to trigger a malfunction with OpenCV, protobuf, and it's shared dependency on zlib.
For example with:

add_executable(test_link main.cpp)
target_link_libraries(test_link CONAN_PKG::opencv CONAN_PNG::protobuf)

We would expect the link library order to look like (opencv lib) (protobuf libs) (zlib libs). Instead, however, we're seeing (opencv libs) (zlib libs) (protobuf libs), resulting in linker errors in protobuf (or opencv, if you swap them around).

It appears to result from the fact that Conan is using INTERFACE IMPORTED targets in CMake, which in the dependency graph will result in their own seperate node.
calling cmake with -DCMAKE_LINK_DEPENDS_DEBUG_MODE=ON, we see that the dependencies it's trying to order look like:

• test_link -> CONAN_PKG::opencv; CONAN_PKG::protobuf
• CONAN_PKG::opencv -> opencv libs; CONAN_PKG::zlib
• CONAN_PKG::protobuf -> protobuf libs; CONAN_PKG::zlib
• CONAN_PKG::zlib -> zlib libs.

Note that the expected dependencies from opencv libs -> zlib libs, or protobuf libs -> zlib libs, are lost in this graph, and CMake's library order is thus correct according to this dependency graph.

I believe this may be solved by using just IMPORTED (without INTERFACE), setting the actual libraries as IMPORTED_LOCATION, and setting the dependencies in INTERFACE_LINK_LIBRARIES, but I must admit I'm not an expert in CMake imported targets like this.
Other solutions that would result in a dependency tree that do incorporate the expected dependencies are probably possible too.

[Frans-Willem]

Another possible solution, I think, would be to expand the dependencies in INTERFACE_LINK_LIBRARIES, as with non-imported libraries it will assume the link order is important and introduce dependencies according to the ordering.

[Frans-Willem]

The temporary fix we've added at this point, which is very hacky indeed but appears to fix the problem, is the following:

   # Fix Conan diamond dependency problem.
   # https://github.com/conan-io/conan/issues/1875
   # https://stackoverflow.com/questions/46663033/cmake-imported-targets-link-order-with-diamond-dependency-is-not-what-we-expect

   message(WARNING "Adding protobuf, opencv, zlib dependency workaround. Is this still needed?")
   get_property(ZLIB_LINK_LIBS TARGET CONAN_PKG::zlib PROPERTY INTERFACE_LINK_LIBRARIES)
   get_property(PROTOBUF_LINK_LIBS TARGET CONAN_PKG::protobuf PROPERTY INTERFACE_LINK_LIBRARIES)
   get_property(OPENCV_LINK_LIBS TARGET CONAN_PKG::opencv PROPERTY INTERFACE_LINK_LIBRARIES)
   set_property(TARGET CONAN_PKG::protobuf PROPERTY INTERFACE_LINK_LIBRARIES ${PROTOBUF_LINK_LIBS} ${ZLIB_LINK_LIBS})
   set_property(TARGET CONAN_PKG::opencv PROPERTY INTERFACE_LINK_LIBRARIES ${OPENCV_LINK_LIBS} ${ZLIB_LINK_LIBS})

[memsharded]

Hi Frans-Willem!

I believe this may be solved by using just IMPORTED (without INTERFACE)

We are using INTERFACE, because in some cases, like header only libraries, IMPORTED won't work, will it? Also, is it possible to create IMPORTED targets for packages containing more than one library? Because that is also a common use case, packages that contain several libraries.

I am not sure if I understand correctly. Conan is already setting the INTERFACE_LINK_LIBRARIES to the package dependencies, like:

set_property(TARGET {name} PROPERTY INTERFACE_LINK_LIBRARIES ${{CONAN_FULLPATH_LIBS_{uname}}} ${{CONAN_SHARED_LINKER_FLAGS_{uname}_LIST}} ${{CONAN_EXE_LINKER_FLAGS_{uname}_LIST}}
                                                                 $<$<CONFIG:Release>:${{CONAN_FULLPATH_LIBS_{uname}_RELEASE}} ${{CONAN_SHARED_LINKER_FLAGS_{uname}_RELEASE_LIST}} ${{CONAN_EXE_LINKER_FLAGS_{uname}_RELEASE_LIST}}>
                                                                 $<$<CONFIG:RelWithDebInfo>:${{CONAN_FULLPATH_LIBS_{uname}_RELEASE}} ${{CONAN_SHARED_LINKER_FLAGS_{uname}_RELEASE_LIST}} ${{CONAN_EXE_LINKER_FLAGS_{uname}_RELEASE_LIST}}>
                                                                 $<$<CONFIG:MinSizeRel>:${{CONAN_FULLPATH_LIBS_{uname}_RELEASE}} ${{CONAN_SHARED_LINKER_FLAGS_{uname}_RELEASE_LIST}} ${{CONAN_EXE_LINKER_FLAGS_{uname}_RELEASE_LIST}}>
                                                                 $<$<CONFIG:Debug>:${{CONAN_FULLPATH_LIBS_{uname}_DEBUG}} ${{CONAN_SHARED_LINKER_FLAGS_{uname}_DEBUG_LIST}} ${{CONAN_EXE_LINKER_FLAGS_{uname}_DEBUG_LIST}}>
                                                                 {deps})

Where the final deps are the dependent packages, so the dependencies to Zlib should be indeed there.

Also, the conan test suite also includes a diamond, and uses the TARGETS approach, and it is passing in all OS: https://github.com/conan-io/conan/blob/develop/conans/test/integration/diamond_test.py#L60

Which OS and which version of conan and cmake are you using? Cheers!

[Frans-Willem]

Hi memsharded,

Thanks for the prompt reply! I'm sorry I haven't been able to reply earlier!

I've looked at the unit-test, and it does in fact display this error too, although somehow in this case the link order does not seem to throw g++ off-balance, I'm not sure why it's working in this case.

I do hope that you agree that in the link order for this unit test hello0 should come after both hello1 and hello2, right ?

If you add these lines to _diamond_test to check the actual link order, I see diamond_cmake_targets_test failing on my system:

    link_cmd = load(client2.current_folder + "/CMakeFiles/say_hello.dir/link.txt")
    index_hello0 = link_cmd.index("libhelloHello0")
    index_hello1 = link_cmd.index("libhelloHello1")
    index_hello2 = link_cmd.index("libhelloHello2")
    index_hello3 = link_cmd.index("libhelloHello3")

    self.assertTrue(index_hello1 < index_hello0) # Hello1 depends on Hello0, so Hello0 should come later in the link order.
    self.assertTrue(index_hello2 < index_hello0) # Hello2 depends on Hello0, so Hello0 should come later in the link order.
    self.assertTrue(index_hello3 < index_hello1)
    self.assertTrue(index_hello3 < index_hello2)

My system being: ArchLinux, with CMake 3.9.4, GCC 7.2.0.

[Frans-Willem]

Did some more digging, but I can't find out why the link order does not seem to be important with the hello-world libraries in the unit test, but it is somehow being a problem in our protobuf/opencv/zlib hell.

Is the linker order mixup on it's own enough to keep this issue open ?

[memsharded]

Yes, lets keep this open. In fact we have had linker order problems in the past, this kind of diamond test was implemented for checking exactly this. Test suite pass in all major OS including FreeBSD and Solaris, conan is widely use with different compilers... don't know what is happening but I would like to have a detailed look.

BTW, is the order of libs correct if using the old cmake approach without TARGETS?

[Frans-Willem]

Yes, when using the old CMake approach, CONAN_LIBS is set properly, as it's doing so outside of CMake.

The problem, in my view, appears to be that the dependency graph you think you're telling CMake, is not the one CMake is actually using.

In the diamond test, the dependency graph you'd like to instruct CMake with would look like this:
http://graphviz.herokuapp.com/?token=d355bac5eaaa76bedc1dec0d5367ea48

digraph G {
  libhelloHello4 -> libhelloHello3
  libhelloHello3 -> libhelloHello1
  libhelloHello3 -> libhelloHello2
  libhelloHello1 -> libhelloHello0
  libhelloHello2 -> libhelloHello0
}

However, with CMake's targets, the actual dependency graph you're pulling up in CMake is the following:
http://graphviz.herokuapp.com/?token=b709e1217011d8a088fe3203b2e07662

digraph G {
  libhelloHello4 -> CONAN_PKG_Hello3
  CONAN_PKG_Hello3 -> libhelloHello3,CONAN_PKG_Hello1,CONAN_PKG_Hello2
  CONAN_PKG_Hello2 -> libhelloHello2,CONAN_PKG_Hello0
  CONAN_PKG_Hello1 -> libhelloHello1,CONAN_PKG_Hello0
  CONAN_PKG_Hello0 -> libhelloHello0
}

Note that in the second graph, the order "CONAN_PKG_Hello3 libhelloHello3 CONAN_PKG_Hello1 CONAN_PKG_Hello2 libhelloHello1 CONAN_PKG_Hello0 libhelloHello0 libhelloHello2" is valid, while it is definitely not in the first.

You can confirm this by running cmake -DCMAKE_LINK_DEPENDS_DEBUG_MODE=ON in the directory of the unit test, with the output being:

Link dependency analysis for target helloHello4, config noconfig
item 0 is [CONAN_PKG::Hello3]
  item 1 must follow it
  item 2 must follow it
  item 3 must follow it
item 1 is [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello3/0.1/lasote/stable/package/e2cc150cdcbc5906e3a8ef4875eb4bbb3093decf/lib/libhelloHello3.a]

item 2 is [CONAN_PKG::Hello1]
  item 4 must follow it
  item 5 must follow it
item 3 is [CONAN_PKG::Hello2]
  item 5 must follow it
  item 6 must follow it
item 4 is [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello1/0.1/lasote/stable/package/72bb63a6ddbe703c3e97e12a0c6b844a936e8965/lib/libhelloHello1.a]

item 5 is [CONAN_PKG::Hello0]
  item 7 must follow it
item 6 is [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello2/0.1/lasote/stable/package/72bb63a6ddbe703c3e97e12a0c6b844a936e8965/lib/libhelloHello2.a]

item 7 is [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello0/0.1/lasote/stable/package/6db7dfeb5382a0976d758d6c1841acf4cf1c7f1c/lib/libhelloHello0.a]


The strongly connected components are:
Component (0):
  item 1 [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello3/0.1/lasote/stable/package/e2cc150cdcbc5906e3a8ef4875eb4bbb3093decf/lib/libhelloHello3.a]
  topo order index 1
Component (1):
  item 4 [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello1/0.1/lasote/stable/package/72bb63a6ddbe703c3e97e12a0c6b844a936e8965/lib/libhelloHello1.a]
  topo order index 3
Component (2):
  item 7 [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello0/0.1/lasote/stable/package/6db7dfeb5382a0976d758d6c1841acf4cf1c7f1c/lib/libhelloHello0.a]
  topo order index 6
Component (3):
  item 5 [CONAN_PKG::Hello0]
  followed by Component (2)
  topo order index 5
Component (4):
  item 2 [CONAN_PKG::Hello1]
  followed by Component (1)
  followed by Component (3)
  topo order index 2
Component (5):
  item 6 [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello2/0.1/lasote/stable/package/72bb63a6ddbe703c3e97e12a0c6b844a936e8965/lib/libhelloHello2.a]
  topo order index 7
Component (6):
  item 3 [CONAN_PKG::Hello2]
  followed by Component (3)
  followed by Component (5)
  topo order index 4
Component (7):
  item 0 [CONAN_PKG::Hello3]
  followed by Component (0)
  followed by Component (4)
  followed by Component (6)
  topo order index 0

target [helloHello4] links to:
  target [CONAN_PKG::Hello3]
  item [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello3/0.1/lasote/stable/package/e2cc150cdcbc5906e3a8ef4875eb4bbb3093decf/lib/libhelloHello3.a]
  target [CONAN_PKG::Hello1]
  item [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello1/0.1/lasote/stable/package/72bb63a6ddbe703c3e97e12a0c6b844a936e8965/lib/libhelloHello1.a]
  target [CONAN_PKG::Hello2]
  target [CONAN_PKG::Hello0]
  item [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello0/0.1/lasote/stable/package/6db7dfeb5382a0976d758d6c1841acf4cf1c7f1c/lib/libhelloHello0.a]
  item [/tmp/tmpj3qdceudconans/path with spaces/.conan/data/Hello2/0.1/lasote/stable/package/72bb63a6ddbe703c3e97e12a0c6b844a936e8965/lib/libhelloHello2.a]

But as I said before, I have no idea why G++ is somehow accepting the invalid link order in this specific unit test, as far as I know it should definitely fail, and it is failing in our case...

[memsharded]

Hi, I have been investigating this for a while, little success. I think the key is:

The problem, in my view, appears to be that the dependency graph you think you're telling CMake, is not the one CMake is actually using.

So, I have reviewed the inputs, and they seem good. They are in order and everything relates to each component well.

Could it be a cmake bug? I would say that if:

target [CONAN_PKG::Hello2] => target[CONAN_PKG::Hello0]
target [CONAN_PKG::Hello2]  => item [libhelloHello2.a]
target [CONAN_PKG::Hello0]  => item[ libhelloHello0.a]

There is no reason that libhelloHello0.a appears first in the link order, seems a bug. WDYT? Maybe submit the case to CMake issue tracker?

[Frans-Willem]

I'm not so sure it's CMake's fault. Did the visualization of the second graph work for you ?
If not, paste the graph code in an online graphviz thingie, and take a peek.

What we're after, in essence, is to define a target CONAN_PKG::opencv that is for all intents and purposes equal to libopencv.lib;libopencv_a.lib;libopencv_b.lib, and then to define a dependency relationship from CONAN_PKG::opencv to CONAN_PKG::zlib. However, INTERFACE IMPORTED targets do not offer this functionality. All they do is define an (empty) target that you can define dependency relationships under. So instead of the above, what you're actually telling CMake is to define an empty target CONAN_PKG::opencv, that depends on libopencv.lib;libopencv_a.lib;libopencv_b.lib;CONAN_PKG::zlib. The dependency between libopencv.lib and CONAN_PKG::zlib (as an example) is not defined.

IMPORTED targets, on the other hand, are closer to what we're after: they allow you to define an equivalency (e.g. CONAN_PKG::opencv == libopencv.lib), as well as to define dependency relationships. The downside with IMPORTED targets, though, is that the equivalency relationship is one-on-one, we can't define CONAN_PKG::opencv == libopencv.lib;libopencv_a.lib;libopencv_b.lib.

What we could do is the following:

(INTERFACE IMPORTED) CONAN_PKG::opencv depends on CONAN_PKG::opencv_libopencv CONAN_PKG::opencv_libopencv_a CONAN_PKG::opencv_libopencv_b CONAN_PKG::zlib
(IMPORTED) CONAN_PKG::opencv_libopencv = libopencv.lib and depends on CONAN_PKG::zlib
(IMPORTED) CONAN_PKG::opencv_libopencv_a = libopencv_a.lib and depends on CONAN_PKG::zlib
(IMPORTED) CONAN_PKG::opencv_libopencv_b = libopencv_b.lib and depends on CONAN_PKG::zlib

It's very verbose, but it does capture all dependencies we're after, and it will result in a proper link order.

I really do not think this is a CMake issue or bug, but rather a slight misunderstanding of CMake's INTERFACE IMPORTED targets. I really struggled to understand this at first, and I'm not very confident that I was able to clearly explain it in writing. I think the best bet is to actually see the generated graphviz images.

[memsharded]

Yes, yes, I have checked the graph. But I still think that having:

A->a
A->B
B->b

Shouldn't get the order A, B, b, a, even if it is consistent with the graph definition, it should be easy to fix just applying node relative degree.

Yes, your idea might work, will check its viability, thanks!!

[memsharded]

Just in case, reported in: https://gitlab.kitware.com/cmake/cmake/issues/17416

[memsharded]

Trying approach: https://github.com/conan-io/conan/pull/1973/files

[lewie-donckers]

I'm not sure if I should make a new issue but I just stumbled into a very similar problem.

Using the TARGETS approach, Conan and CMake do not create the correct linker command; the libraries are listed in an incorrect order.

When using the 'old' approach, they do generate the correct linker command.

Old ${CONAN_LIBS} approach:
conanbuildinfo.cmake.txt
link.txt

New ${CONAN_TARGETS} approach:
conanbuildinfo.cmake.txt
link.txt

[lasote]

Thanks for reporting. We will review it.

[memsharded]

Theoretically, the PR https://github.com/conan-io/conan/pull/1973 should solve it. It would be great if you could double check, running conan from sources from my origin branch for that PR for your code, before it is being merged and released.

[lewie-donckers]

Took me some time to figure out how to run Conan from source from your PR but it worked. The link order was correct.

[memsharded]

Excellent!! Thanks very much for testing this. Merged to develop, will be in conan 0.29.

[theodelrieu]

I've applied the patch locally, and it works fine!

[memsharded]

Excellent! It is already merged in develop, so it will be in next 0.29. Thanks for telling!