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

cmake_paths generator broken for transitive dependencies #4467

Closed
NewProggie opened this issue Feb 5, 2019 · 3 comments
Closed

cmake_paths generator broken for transitive dependencies #4467

NewProggie opened this issue Feb 5, 2019 · 3 comments

Comments

@NewProggie
Copy link
Contributor

@NewProggie NewProggie commented Feb 5, 2019

The cmake_paths generator currently doesn't handle transitive dependencies for Conan packages. I am not talking about the requires option, but for transitive dependencies in a package itself.

Consider the following simple conanfile.txt configuration:

[requires]
opencv/4.0.1@conan/stable

[generators]
cmake_paths

Since the cmake_paths generator only declares the CMAKE_MODULE_PATH and CMAKE_PREFIX_PATH variables, a client is currently forced to call conan as follows:

$ mkdir build && cd build
$ conan install --build=missing ..
$ cmake \
    -DCMAKE_TOOLCHAIN_FILE=conan_paths.cmake \
    -DCMAKE_BUILD_TYPE=Release \
    -DCONAN_LIBJPEG_ROOT:PATH=/Users/NewProggie/.conan/data/libjpeg/9c/bincrafters/stable/package/560b128b7f18cdc8f77f23c6834ffad4a/ \
    -DCONAN_ZLIB_ROOT:PATH=/Users/NewProggie/.conan/data/zlib/1.2.11/conan/stable/package/560b128b7f18cdc8ff080cad77f34ffad4a/ \
    -DCONAN_LIBPNG_ROOT:PATH=/Users/NewProggie/.conan/data/libpng/1.6.34/bincrafters/stable/package/2fcedcd518d74d31f30df6a210eeb54d/ \
    -DCONAN_LIBTIFF_ROOT:PATH=/Users/NewProggie/.conan/data/libtiff/4.0.9/bincrafters/stable/package/2fcedcd518d74d31f90df6a210eeb54d/ \
    ..

If a client doesn't add the CONAN_*_ROOT variables, this setup leads to linker problems, since the OpenCVConfig CMake module is cluttered with those variables and they will simple be empty otherwise (resulting in linker errors such as: cannot find /lib/libjpeg.a. Note the missing leading path here).

A rather simple and appropriate fix to this would be to extend the cmake_paths generator with all those custom and necessary CONAN_*_ROOT variables as follows:

 # as before
set(CMAKE_MODULE_PATH ...)
set(CMAKE_PREFIX_PATH ...)

# new and mandatory for transitive package dependencies
set(CONAN_LIBJPEG_ROOT ...) 
set(CONAN_ZLIB_ROOT ...)
set(CONAN_LIBPNG_ROOT ...)
set(CONAN_LIBTIFF_ROOT) ...

Since the paths to those transitive dependencies already get appended to the CMAKE_MODULE_PATH and the CMAKE_PREFIX_PATH respectively, I guess this is simply a matter of extracting those out of DepsCppCMake:

deps = DepsCppCmake(self.deps_build_info)
# We want to prioritize the FindXXX.cmake files:
# 1. First the files found in the packages
# 2. The previously set (by default CMAKE_MODULE_PATH is empty)
# 3. The "install_folder" ones, in case there is no FindXXX.cmake, try with the install dir
# if the user used the "cmake_find_package" will find the auto-generated
# 4. The CMake installation dir/Modules ones.
return """set(CMAKE_MODULE_PATH {deps.build_paths} ${{CMAKE_MODULE_PATH}} ${{CMAKE_CURRENT_LIST_DIR}})

Please let me know, if I am doing something wrong here, or if the proposed solution is feasible.

@lasote lasote self-assigned this Feb 5, 2019
@lasote

This comment has been minimized.

Copy link
Contributor

@lasote lasote commented Feb 5, 2019

I'm not sure I understand.
The cmake_paths generator is intended to be used only when a CMakeLists.txt file is using find_package to locate ALL the dependencies. I don't know the details of the OpenCV recipe implementation but the CONAN_*_ROOT variables are generated by the cmake generator, not the cmake_paths generator. So probably you need to change the generator and include the conanbuildinfo.cmake file.

@NewProggie

This comment has been minimized.

Copy link
Contributor Author

@NewProggie NewProggie commented Feb 5, 2019

So probably you need to change the generator and include the conanbuildinfo.cmake file.

No, this is exactly what I want to avoid, since I don't want my package manager to interfere with my build system configuration. I've set up a working example on this issue here [1].

In a nutshell:
In my CMakeLists.txt I declare find_package(OpenCV REQUIRED) and inside my conanfile.txt I write

[requires]
opencv/4.0.1@conan/stable

[generators]
cmake_paths

Since, the OpenCV package itself depends on other dependencies such as libjpeg, zlib etc. it needs to be pointed to the dependencies Conan has already downloaded and cached for exactly this package.

Typically, this is done via CMake calls to find_dependency(...). However, it seems that the OpenCV Conan package is suboptimal here as it makes use of the previously mentioned CONAN variables. I'd like to stress that this information can be stored in the conan_paths.cmake file, since the information is already included.

I think, I've demonstrated a very simple use case of Conan where I have a small CMake-based project with a single dependency and want to use Conan in a non-intrusive way. Currently this is not possible. CMake's find_package() behaviour is well defined here, as it should resolve all dependencies of a given package without forcing a client to list all transitive dependencies him/herself [2].

[1] https://github.com/NewProggie/Non-Intrusive-Conan-CMake
[2] https://cmake.org/cmake/help/v3.13/module/CMakeFindDependencyMacro.html

@lasote

This comment has been minimized.

Copy link
Contributor

@lasote lasote commented Feb 5, 2019

Thanks for the info, now I understand what you mean. I forgot we have the cmake.patch_config_paths() helper to replace the absolute paths with "ROOT" folders.
I think it makes a lot of sense what you are requesting.

@lasote lasote removed their assignment Feb 5, 2019
@lasote lasote added this to the 1.13 milestone Feb 5, 2019
NewProggie added a commit to NewProggie/conan that referenced this issue Feb 16, 2019
This commit fixes an issue when using the cmake_paths generator with
packages that itself have one or more transitive package dependencies.
Currently only the CMake variables CMAKE_MODULE_PATH and
CMAKE_PREFIX_PATH get populated in the conan_paths.cmake toolchain file.

However, this information is not sufficient for packages that have been
generated using the cmake generator as they may use Conan specific
variables in the form of CONAN_*_ROOT that are populated with the paths
pointing to its own, transitive dependencies.

Since this information is already stored in |DepsCppCmake| when creating
the conan_paths.cmake toolchain file, this commit just populates the custom
Conan variables in this file as well.
@lasote lasote modified the milestones: 1.13, 1.14 Mar 5, 2019
@lasote lasote self-assigned this Mar 8, 2019
@ghost ghost added stage: review and removed stage: in-progress labels Mar 12, 2019
@lasote lasote closed this in #4719 Mar 15, 2019
@ghost ghost removed the stage: review label Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.