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

[question] Mimicking CMake Find Modules #419

Closed
Morwenn opened this issue Dec 3, 2019 · 12 comments
Closed

[question] Mimicking CMake Find Modules #419

Morwenn opened this issue Dec 3, 2019 · 12 comments
Labels
question Further information is requested

Comments

@Morwenn
Copy link
Contributor

Morwenn commented Dec 3, 2019

When creating a package that already exists in standard CMake Find Modules it is advised to have a cpp_info.name corresponding to the name expected by the equivalent call to find_package to make sure that the cmake_find_package* generators work smoothly.

However some of these standard CMake recipes provide more CMake variables than those provided by the Conan generators, and libraries using them through find_package tend to make use of these additional variables. The example I had in mind was HDF5 for which I added a recipe to conan-center-index a few weeks ago: it provides more CMake variables that the bare minimum such as HDF5_HL_LIBRARIES or HDF5_IS_PARALLEL.

When adding recipes to conan-center-index for such packages that already standard in CMake, should we also provide those additional variables through cpp_info.build_modules to make sure that the cmake_* generators work with already existing projects in the wild to the best of our ability?

@Morwenn Morwenn added the question Further information is requested label Dec 3, 2019
@madebr madebr mentioned this issue Dec 7, 2019
4 tasks
@madebr
Copy link
Contributor

madebr commented Dec 7, 2019

Other packages have the same problem:

  • openssl: missing OpenSSL::Crypto and OpenSSL::SSL targets, missing OPENSSL_CRYPTO_LIBRARY, OPENSSL_CRYPTO_LIBRARIES, OPENSSL_SSL_LIBRARY and OPENSSL_SSL_LIBRARIES variable
  • libxml2: missing LIBXML2_XMLLINT_EXECUTABLE and LIBXML2_VERSION_STRING variable (if the executable is shipped)
  • zlib: missing *_VERSION_* variables

@madebr
Copy link
Contributor

madebr commented Dec 9, 2019

I've created a PR for adding the variables for libpng: #434

@jgsogo
Copy link
Contributor

jgsogo commented Dec 12, 2019

As we were discussing here, sometimes it would be better not to provide all the CMake variables if they are not consistent, it can be better to patch the build scripts of the consumers (and work to improve the C++ ecosystem).

Other variables could go in the cpp_info.defines and others will be available with the components feature (getting delayed more than expected). IMO the build_modules should only be used as a last resort (or for famous libraries where it is not possible to patch/educate all the consumers).

@Morwenn
Copy link
Contributor Author

Morwenn commented Dec 12, 2019

I wish education would be enough, but on the other hand I have to redefine the exact same variable in the CMake wrapper whenever I package a new library that uses HDF5 through CMake and can't just rely on cmake_find_package :/

@madebr
Copy link
Contributor

madebr commented Dec 12, 2019

Imho, the FindXXX.cmake documentation at cmake.org should be considered as an API.
Any deviation of it must be considered a bug.

The recipes created by this repo are targeted to the complete conan community, not only us (=packagers).
If some conan find_package script behaves different then the cmake one,
users would consider conan as non-functional or broken.
The conan find_package script should just work interchangeably with the cmake one.

@gocarlos
Copy link
Contributor

Any deviation of it must be considered a bug.

agree, currently we build with conan for development and with yocto for production (regulated industries...)
that we cannot interchangeable use the standard find_package() functions makes a lot of issues...

actually its not only the imported targets ala OpenSSL::SSL vs. OpenSSL::OpenSSL but also result variables e.g. https://cmake.org/cmake/help/v3.9/module/FindGTest.html#result-variables defines:

GTEST_LIBRARIES, conan defines GTest_LIBRARIES

those smaller bugs make the adoptions in some big coorporations pretty hard/slow

@madebr
Copy link
Contributor

madebr commented Feb 25, 2020

I've opened a RFC at conan-io/conan#6587
Please comment

@kyllingstad
Copy link
Contributor

Any deviation of it must be considered a bug.

agree, currently we build with conan for development and with yocto for production (regulated industries...)
that we cannot interchangeable use the standard find_package() functions makes a lot of issues...

I agree, and would like to add that using Conan for development but not for production is going to be a very common use case. For example, I don't foresee that any of the big Linux distros are going to adopt Conan for dependency management in the near future, so anyone who wants their software to be distributed with Debian, Red Hat, etc. are going to be in this situation.

Any deviation from the CMake-bundled FindXxx.cmake modules or upstream XxxConfig.cmake files is effectively locking developers into the Conan ecosystem or forcing them to jump through hoops to break out of it.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 30, 2020

Linking here this issue (conan-io/conan#7254), we will be adding to 1.28 a way to define the name of the FindXXXX.cmake file which should provide the last bit needed to mimic existing CMake modules. It will, for example, allow the protobuf package to provide targets like protobuf:: from a FindProtobuf.cmake file.

@jgsogo
Copy link
Contributor

jgsogo commented Jul 17, 2020

And finally this one: conan-io/conan#7261, it should be possible to make any adjustments needed.

Anyone is missing something?

@ericLemanissier
Copy link
Contributor

qt (since 5.15.0) creates targets both in Qt5 and Qt namespace : https://doc.qt.io/qt-5/cmake-get-started.html#imported-library-targets. I guess this is not possible to mimic this yet ?

@franramirez688
Copy link
Contributor

This is already solved by the generator CMakeDeps.

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

Successfully merging a pull request may close this issue.

7 participants