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

How to create imported targets in FindXXX.cmake #2125

Closed
wpalfi opened this issue Dec 7, 2017 · 27 comments
Closed

How to create imported targets in FindXXX.cmake #2125

wpalfi opened this issue Dec 7, 2017 · 27 comments
Milestone

Comments

@wpalfi
Copy link

wpalfi commented Dec 7, 2017

In http://docs.conan.io/en/latest/integrations/cmake/find_packages.html?highlight=find_package
you use old-style variable based findXXX:

find_package("MyLib")
include_directories(${MyLib_INCLUDE_DIRS})
target_link_libraries(MyApp ${MyLib_LIBRARIES})

The better way for find_package is to use imported targets. I want to create an imported target with the same name as the original cmake target name of the library to be able to use

find_package("MyLib")
target_link_libraries(MyApp MyLib)

Then we can easily switch between imported conan package and building from source with add_subdirectory without any changes in the consuming CMakeLists.txt. Best solution would something like

add_library(MyApp ALIAS CONAN_PKG::MyApp)

Unfortunately ALIAS of IMPORTED is not supported by CMake. My current workaround is


function(add_cloned_imported_target dst src)
	get_property(_INTERFACE_LINK_LIBRARIES TARGET ${src} PROPERTY INTERFACE_LINK_LIBRARIES )
	get_property(_INTERFACE_INCLUDE_DIRECTORIES TARGET ${src} PROPERTY INTERFACE_INCLUDE_DIRECTORIES )
	get_property(_INTERFACE_COMPILE_DEFINITIONS TARGET ${src} PROPERTY INTERFACE_COMPILE_DEFINITIONS )
	get_property(_INTERFACE_COMPILE_OPTIONS TARGET ${src} PROPERTY INTERFACE_COMPILE_OPTIONS )

	add_library(${dst} INTERFACE IMPORTED)
	set_property(TARGET ${dst} PROPERTY INTERFACE_LINK_LIBRARIES ${_INTERFACE_LINK_LIBRARIES})
	set_property(TARGET ${dst} PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${_INTERFACE_INCLUDE_DIRECTORIES})
	set_property(TARGET ${dst} PROPERTY INTERFACE_COMPILE_DEFINITIONS ${_INTERFACE_COMPILE_DEFINITIONS})
	set_property(TARGET ${dst} PROPERTY INTERFACE_COMPILE_OPTIONS ${_INTERFACE_COMPILE_OPTIONS})
endfunction()

add_cloned_imported_target(MyLib CONAN_PKG::MyLib)
set(MyLib_FOUND TRUE)

Any other solutions? Maybe some helper function in conanbuildinfo.cmake to create imported targets?

@memsharded
Copy link
Member

Yes, I know, actually there was a previous attempt to be able to rename targets at user will. But because of this ALIAS limitation, it was a nightmare in the end, and lots of extra code to maintain, so finally we didn't add it.

Your implementation sounds quite reasonable, clear, easy to read & understand... I think it is ok. Actually you can reduce it looping over the properties (INTERFACE_LINK_LIBRARIES, etc). In any case, I think it would be enough to add it to the docs, adding it to conanbuildinfo.cmake might have more cons (maintenance, future upgrades, etc) than pros. That, or I would just do the link conditionally:

if (CONAN)
     target_link_libraries(myapp CONAN_PKG::MyPkg)
else()
     target_link_libraries(myapp MyLib)
endif()

@wpalfi
Copy link
Author

wpalfi commented Dec 7, 2017

The reason for not linking conditionally is that the linking happens in a library that is not related to conan.

@memsharded
Copy link
Member

Yes, that is fine, but even if that linking is in a library not related to conan, you still need to load conanbuildinfo.cmake and define the targets, don't you? Or maybe you are adding that code the a FindXXX.cmake file inside the package, and then manually defining the CMAKE_MODULE_PATH so the find works first in the conan package directory?

@wpalfi
Copy link
Author

wpalfi commented Dec 8, 2017

The library that calls findXXX.cmake is in its own repository. Its up to the user of the lib to provide the external requirements by manually installing them or using some package manager. e.g.:

  • patch the CMakeLists.txt of the lib (in the conanfile)
  • provide findXXX.cmake and call conan_basic_setup(TARGETS) from a parent CMakeLists.txt before adding the lib with add_subdirectory

@memsharded
Copy link
Member

Ah, ok, now I get it. Yes, those are exactly the 2 typical ways to package a library without modifying the existing CMakeLists script: either patching or using a parent CMakeLists to wrap it.

We have thought about it a bit, and adding convenience functions in conanbuildinfo.cmake that are not directly used by conan has more cons than pros. Then, yes, I would say that the best approach for your use case is use your above add_cloned_imported_target () function (maybe use a for loop to iterate properties), which would be a bit shorter, in your own CMake script.

@wpalfi
Copy link
Author

wpalfi commented Dec 12, 2017

Packaging is not the problem. We can use patching for packaging. But if we develop a library we want to clone the repo and use it without modifications. Then we need the find_package solution to use conan from outside.

Here is my improved version:

function(add_cloned_imported_target dst src)
    add_library(${dst} INTERFACE IMPORTED)
    foreach(name INTERFACE_LINK_LIBRARIES INTERFACE_INCLUDE_DIRECTORIES INTERFACE_COMPILE_DEFINITIONS INTERFACE_COMPILE_OPTIONS)
        get_property(value TARGET ${src} PROPERTY ${name} )
        set_property(TARGET ${dst} PROPERTY ${name} ${value})
    endforeach()
endfunction()

if(NOT TARGET LibA::LibA)
    add_cloned_imported_target(LibA::LibA CONAN_PKG::LibA)
endif()
set(LibA_FOUND TRUE)

@memsharded
Copy link
Member

Thanks for updating with the improved version, indeed looks better. Still, I don't see it in the generated conanbuildinfo.cmake file, as long as it is not being used by it, and otherwise conanbuildinfo.cmake would easily become a library of cmake utilities that would be very difficult to maintain and break easily, being auto-generated and serving so many users.

Unless I am missing something, which is totally possible :)
Let me review your use case:

  • You have a library LibA, that can be installed from conan, with conan install, or it can be obtained by some other means (cmake subdirectory, for example)
  • You have a consumer library LibB, that needs/consumes/link with LibA. You want to keep your LibB build script CMakeLists.txt agnostic about LibA origin, then, it will only have one find_package("LibA")
  • You are providing yourself for the case of conan your own find_liba.cmake script, that contains the above add_cloned_imported_target, and which also does the include(.../conanbuildinfo.cmake)

I have some missing pieces, like where are you putting this find_liba.cmake, how do you manage it relatively to the installed conanbuildinfo.cmake, or how do you switch between configurations, modifying the CMAKE_MODULE_PATH?

@wpalfi
Copy link
Author

wpalfi commented Dec 13, 2017

You explained it quite well.

  • We have LibA and LibB, both don't know anything about conan. LibB requires LibA.
  • We have LibA and LibB conan recipes in separate repos, each with a FindLibX.cmake module. FindLibX.cmakes are exported to the conan packages, conan sets the required cmake module paths automatically.
  • In LibB (and in all consumers of LibA) we just use find_package(LibA), same with LibB
  • Now we are free to decide for each Lib if we want to add the source with add_subdirectory or use the imported target (conan package).

Here is a simple example of a consuming app:

project(App1 CXX)
cmake_minimum_required (VERSION 3.9)

include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup(TARGETS) #this will add the LibX conan packages to CMAKE_MODULE_PATH

set(BUILD_SHARED_LIBS ON)

if(...)
    find_package("LibA" REQUIRED) #this will use FindLibA.cmake in LibA conan package
else()
    add_subdirectory(../lib-a lib-a)
endif()

if(...)
    find_package("LibB" REQUIRED)  #this will use FindLibB.cmake in LibB conan package
else()
    add_subdirectory(../lib-b lib-b) #ready to use find_package("LibA") inside
endif()

add_executable(App1 main.cpp)
target_link_libraries(App1 LibA::LibA)
target_link_libraries(App1 LibB::LibB)

The only thing to take care of, is that we dont use LibB from conan and build LibA from source. Then we would have two versions of LibA in the project.

@wpalfi
Copy link
Author

wpalfi commented Dec 14, 2017

Another use case is when you have an external library and the standard FindXxx.cmake does not work with the conan package(s): bincrafters/community#26

@helmesjo
Copy link

helmesjo commented Dec 23, 2017

Just to tap in on the discussion:
So using cmake and linking packages retrieved by conan, they all require to be prefixed with CONAN_PKG::, instead of with the respective packagename? So I can't use conan to install boost and then link using for example Boost::thread?

If so, this is kind of a huge problem. Requirement number 1 for any package manager should be that consuming libraries are agnostic to where the packages originates from...

@memsharded
Copy link
Member

If so, this is kind of a huge problem. Requirement number 1 for any package manager should be that consuming libraries are agnostic to where the packages originates from...

Is it really a huge problem? As described above by @wpalfi you can rename the target with 8 lines of cmake code. Also, any idea how packages can be consumed by any build system, being completely agnostic to where the packages originates from, in all OSs and platforms?

Packages are quite agnostic of their contents (besides the package_info() method). They are created in the consumer side, irrespective of the build system of the package itself, which can be different (or even non-existing, if pre-built binares are being directly packaged).

In any case, it is important to highlight that the CONAN_PKG:: scope is because that target is NOT a target of a specific library like "thread". It is a IMPORTED INTERFACE target for the whole package. Then you have different CONAN_LIB:: targets, for each library within the package, but without dependency relations between them. If you want such relations, per library, you might be interested in the bincrafters modular version of boost: https://bincrafters.github.io/

@helmesjo
Copy link

helmesjo commented Dec 24, 2017

Just to be clear, and to avoid misunderstanding each other, in my case I'm specifically talking about CMake & the expected targets for that generator. But I'm sure this example applies to others as well. Also, my current knowledge of conan is limited to Getting started & a webinar.

The problem arises when you mix pre-installed packages, eg. using conan, and packages built within the same repo/solution, eg. using add_library. I might not be (and most of the time is not) in charge of all targets/projects built within a "solution".
In my case, I'm building cpprestsdk as part of my solution, but cpprestsdk depends upon zlib, a few boost components & openssl. Making internal changes to external libraries like cpprestsdk is of course not an option.
My first approach was to build all of these as well, but as most CMake-users know the quality of CMake-scripts vary widely between projects, and rarely follow "modern" guidelines. Thus, downstream targets trying to link these more than always fail to do so. My current approach is a middle-layer CMakeLists.txt for each external dependency that does the necessary set_property-magic to try achieve a properly setup target & abstract away complexity so that linking targets just do target_link_libraries & all is good, but it really is a nightmare & sometimes just not enough (then again, most projects still use since long deprecated include_directories and the like so it fails regardless).

So, naturally Conan sounded like an excellent option, and to be clear it really is. It has recipe for all but cpprestsdk, great! However, if you don't follow the naming conventions used for CMake-targets, then as far as cpprestsdk is concerned, you have not supplied a single dependency.

I'm glancing at the "8-line fix" but not sure I fully grasp how he ends up with LibA::LibA (might be my lacking knowledge of conan), but TBH it sounds like a lot more than 8 lines of code (now imagine 4 targets, 10, 20... not really scalable).
But sure, there probably are ways to go around this but really why should we have to? What is the real reason to prefixing targets with CONAN_PKG:: & CONAN_LIB:: when naming conventions already exist in CMake? Is there a difference between a conan-imported package and an "ordinary" installed one retrieved with for example FindBoost.cmake? If not, then I fail to see why to differentiate them at all and again would argue that it is a huge problem.

IMO, I really hope you make this fix sooner than later, because I have no doubt that Conan will be a (if not thee) go-to package manager for C/C++. Hoping that CMake enables ALIAS for IMPORTED targets is kind of a long shot.

@helmesjo
Copy link

helmesjo commented Dec 24, 2017

Actually, after trying conan for my particular case, find_package works just as expected and everything else just... works. I somehow got confused and thought conan actually manipulated what was returned by find_package, causing dependent libs to fail since they don't prefix with CONAN_PKG::.

Since this is the case, no, it is not a huge problem! (though it would be more consistent)
Sorry for wasting your time, but at least you now have a quicker answer next time someone is confused... :)

@wpalfi
Copy link
Author

wpalfi commented Jan 8, 2018

@helmesjo It's not a problem at all. Creating imported targets with library specific namespaces is in general a project specific task. The "correct" CMake-way to do that is to provide a custom findXxx.cmake. It is really easy to do that with conan. (Having imported alias targets in cmake would make it even easier, but this is a cmake issue.) The only useful thing that conan could do here is maybe to provide a default findXxx.cmake in the conan new command.

@lasote
Copy link
Contributor

lasote commented Jan 8, 2018

So, I think the function by @wpalfi can be something to add to the conanbuildinfo.cmake. Right? Maybe rename it to something like copy_conan_target?

function(copy_conan_target dst src)
    add_library(${dst} INTERFACE IMPORTED)
    foreach(name INTERFACE_LINK_LIBRARIES INTERFACE_INCLUDE_DIRECTORIES INTERFACE_COMPILE_DEFINITIONS INTERFACE_COMPILE_OPTIONS)
        get_property(value TARGET ${src} PROPERTY ${name} )
        set_property(TARGET ${dst} PROPERTY ${name} ${value})
    endforeach()
endfunction()

@lasote lasote added this to the 1.1 milestone Jan 8, 2018
@wpalfi
Copy link
Author

wpalfi commented Jan 8, 2018

So, I think the function by @wpalfi can be something to add to the conanbuildinfo.cmake. Right?

That would be convenient, though not really necessary. And of course it's just a workaround for the function missing in cmake.

@memsharded
Copy link
Member

memsharded commented Jan 8, 2018

@lasote well, I was considering to add it to conanbuildinfo.cmake, but I think we need a criteria for this, or conanbuildinfo.cmake can grow without limits to contain a miriad of useful functions that users can call from their CMakeLists. But this is dangerous, because in the long term is difficult to maintain: every change you do on such generated code is potentially breaking. Also, it is something that it is typically under-tested, as it is not in a main conanbuildinfo.cmake execution path.

But I understand the convenience of having them there too, it is a trade-off, as usual.

@himikof
Copy link

himikof commented Jan 9, 2018

FYI, the following is implemented in the CMake master and should be released in CMake 3.11: https://gitlab.kitware.com/cmake/cmake/merge_requests/1264. In short, IMPORTED GLOBAL libraries can now be used in an ALIAS.

If conanbuildinfo.cmake gets support for marking the imported targets as GLOBAL, the original issue should be easily solved.

@memsharded
Copy link
Member

Thats a good tip, @himikof. We will try to do that, see if it doesn't break anything. Thanks!

@wpalfi
Copy link
Author

wpalfi commented Jan 23, 2018

Good news! https://gitlab.kitware.com/cmake/cmake/merge_requests/1264 should also enable target_link_libraries for imported targets. This could be very useful for non-conan requirements. Conan adds dependencies to required conan packages. But if a target depends on some system installed library, we can add it in findXxx.cmake:

add_library(MyLib::MyLib INTERFACE IMPORTED)

#conan dependencies
target_link_libraries(MyLib::MyLib CONAN_PKG::MyLib)

#system dependencies
find_package(SomeSystemLib REQUIRED)
target_link_libraries(MyLib::MyLib SomeSystemLib::SomeSystemLib)

@pvicente pvicente added the to do label Jan 31, 2018
@memsharded
Copy link
Member

So, if I understood correctly, there is nothing to do on the conan side, and this issue could be considered solved? (given you use latest cmake, of course).

Thanks!

@wpalfi
Copy link
Author

wpalfi commented Feb 22, 2018

Just tested with CMake 3.11.0-rc1 and it works:-)

Needed to add INTERFACE to target_link_libraries:

add_library(MyLib::MyLib INTERFACE IMPORTED)

#conan dependencies
target_link_libraries(MyLib::MyLib INTERFACE CONAN_PKG::MyLib)

#system dependencies
find_package(SomeSystemLib REQUIRED)
target_link_libraries(MyLib::MyLib INTERFACE SomeSystemLib::SomeSystemLib)

@wpalfi wpalfi closed this as completed Feb 22, 2018
@ghost ghost removed the to do label Feb 22, 2018
@memsharded
Copy link
Member

Nice!! Thanks for trying and reporting

@himikof
Copy link

himikof commented Feb 23, 2018

@wpalfi This is a very interesting use of INTERFACE IMPORTED library to emulate ALIAS for non-GLOBAL imported targets (which Conan currently generates), thanks!

But maybe Conan should still just add GLOBAL option for all generated imported targets, so that simple
add_library(MyLib::MyLib ALIAS CONAN_PKG::MyLib) would work instead?
As far as I understand, considering that conan-generated targets are usually included from the root of the build system, there are no semantic differences for defining GLOBAL vs non-GLOBAL imported targets, so there should be no downsides for that.

@memsharded
Copy link
Member

Not sure if global is safe here. There are users running a super-project that will do an add_subdirectory for different sub-projects, each one being a conan-package. Each one might have its own conanfile.py, and will install its dependencies. Will they scape the sub-project scope and be visible to siblings if using GLOBAL?

@himikof
Copy link

himikof commented Feb 23, 2018

Yes, if this is a supported use case (just add_subdirectory, not a full-blown ExternalProject_Add with a local SOURCE_DIR, which shares no CMake state), GLOBAL could be problematic. My thought was that there are so many things in CMake that share a global namespace (any cache variables, most non-imported targets, etc) that the Conan already does not support this use case, as combining multiple independent subprojects with add_subdirectory is already fragile enough without Conan.

@wpalfi
Copy link
Author

wpalfi commented Mar 22, 2018

My thought was that there are so many things in CMake that share a global namespace (any cache variables, most non-imported targets, etc) that the Conan already does not support this use case

Absolutely right. But its not conan's fault. CMake is just not designed to support that. Most findXxx modules are using cache variables, so using multiple packages with conflicting names simply does not work in CMake.

Btw: It is the same problem that you run into when you switch conan settings/options without clearing the CMake cache. Even if you only switch between Debug/Release. It would be good to have a warning about that somewhere in the conan docs.

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

No branches or pull requests

6 participants