search

conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.27k stars 981 forks source link

[feature] CMakeDeps generates incorrect CMake variables #14788

Closed hekrdla closed 6 months ago

hekrdla commented 1 year ago

What is your suggestion?

Hi, I found issue with opencv package using openjpeg (opencv:with_jpeg2000=openjpeg). After my investigation, I have suggestion to new feature into Conan. Here are both issue and my suggestion in details.

Environment

Conan 1.61.0 Windows msvc builds

Issue

Official OpenJPEGConfig.cmake is using UPPERCASE naming (OPENJPEG) for odlschool CMake variables like OPENJPEG_INCLUDE_DIRS even when package name is in CamelCase (OpenJPEG). This is pretty common style, even for CMake built-in finders So packages like opencv (and a lot more) using these oldschool cmake variables, expect official naming.

But Conan CMakeDeps use file_name aka CMake PackageName to generate these variables. So they not match with official Config.cmake

set({{ file_name }}_VERSION_STRING "{{ version }}")
set({{ file_name }}_INCLUDE_DIRS {{ '${' + pkg_name + '_INCLUDE_DIRS' + config_suffix + '}' }} )
set({{ file_name }}_INCLUDE_DIR {{ '${' + pkg_name + '_INCLUDE_DIRS' + config_suffix + '}' }} )
set({{ file_name }}_LIBRARIES {{ '${' + pkg_name + '_LIBRARIES' + config_suffix + '}' }} )
set({{ file_name }}_DEFINITIONS {{ '${' + pkg_name + '_DEFINITIONS' + config_suffix + '}' }} )

Conan recipes deal with this, by patching sources in consuming packages, like in opencv. This complicates these recipes, it probably cost a lot effort to do it, will cost a lot effort to maintain this within updates to new versions and mainly it's easy to miss something, like in example bellow.

Conan workaroud recipes/opencv/4.x/conanfile.py

tc.variables["OPENJPEG_MAJOR_VERSION"] = openjpeg_version.major
tc.variables["OPENJPEG_MINOR_VERSION"] = openjpeg_version.minor
tc.variables["OPENJPEG_BUILD_VERSION"] = openjpeg_version.patch

for this opencv/CMakeLists.txt

if(HAVE_OPENJPEG)
  status("    JPEG 2000:" OpenJPEG_FOUND
      THEN "OpenJPEG (ver ${OPENJPEG_MAJOR_VERSION}.${OPENJPEG_MINOR_VERSION}.${OPENJPEG_BUILD_VERSION})"
      ELSE "build (ver ${OPENJPEG_VERSION})"
  )

But there is no patch for usage in opencv/modules/imgcodecs/CMakeLists.txt

if(HAVE_OPENJPEG)
  ocv_include_directories(${OPENJPEG_INCLUDE_DIRS})
  list(APPEND GRFMT_LIBS ${OPENJPEG_LIBRARIES})
endif()

like it is for jasper

replace_in_file(self, os.path.join(self.source_folder, "modules", "imgcodecs", "CMakeLists.txt"), "JASPER_", "Jasper_")

So OPENJPEG_INCLUDE_DIRS and OPENJPEG_LIBRARIES not exists

I think that Conan needs to address root cause of this issue, not patching every package affected by this. So CMakeDeps should be enhanced to generate proper variables. There is my proposal

What is your suggestion?

New CMake tools already support property for <PackageName>Config.cmake and/or Find<PackageName>.cmake file name:

self.cpp_info.set_property("cmake_file_name", "OpenJPEG")

So there should be also something like: - name reflect CMake Result Variables chapter in documentation

self.cpp_info.set_property("cmake_result_variables_name", "OPENJPEG")

with usage like this:

result_variables_prefix = self.get_property("cmake_result_variables_name", dep) or self.get_property("cmake_file_name", dep)

This will work for most packages, but there are exceptions like FindPythonLibs, where is used PYTHON_ vs PYTHONLIBS_

PYTHONLIBS_FOUND           - have the Python libs been found
PYTHON_LIBRARIES           - path to the python library
PYTHON_INCLUDE_PATH        - path to where Python.h is found (deprecated)
PYTHON_INCLUDE_DIRS        - path to where Python.h is found
PYTHON_DEBUG_LIBRARIES     - path to the debug library (deprecated)
PYTHONLIBS_VERSION_STRING  - version of the Python libs found (since CMake 2.8.8)

so property to define name for version related variables may be useful

self.cpp_info.set_property("cmake_version_variables_name", "PYTHONLIBS")

with usage like this:

version_variables_prefix = self.get_property("cmake_version_variables_name", dep) or result_variables_prefix

But because CMake find_package provides version related variables like <PackageName>_VERSION_MAJOR, packages like OpenJPEG also explicitely set uppercase versions like <PACKAGE_NAME>_VERSION_MAJOR. So in case that version_variables_prefix not match with PackageName (file_name) Conan should set these as well. And probably also <PACKAGE_NAME>_FOUND variant

I would also consider to add <PackageName>_LIBRARY. Some package set this one and if not, nobody will use it.

Last issue with this enchancement is, that there are already patches in conan recipes to deal with this and they will be invalid with fixed Config.cmake. Also depricated cmake tools will not support this. And there are also packages like libwebp that supports both naming styles. So current variables named based on cmake_file_name needs to be generated as well. I'm not sure, if this should be supported always or somehow configurable.

All toghether

generate_file_name_variables = True # TODO: optional?

New optional properies:

self.cpp_info.set_property("cmake_result_variables_name", "PYTHON")
self.cpp_info.set_property("cmake_version_variables_name", "PYTHONLIB")

With defaults like:

file_name = self.get_property("cmake_file_name", dep)
result_variables_prefix = self.get_property("cmake_result_variables_name", dep) or file_name
version_variables_prefix = self.get_property("cmake_version_variables_name", dep) or result_variables_prefix

And usage in CMakeDeps

{% if generate_file_name_variables and version_variables_prefix != file_name %}
set({{ file_name }}_VERSION_STRING "{{ version }}")
{% endif %}
{% if generate_file_name_variables and result_variables_prefix != file_name %}
set({{ file_name }}_INCLUDE_DIRS {{ '${' + pkg_name + '_INCLUDE_DIRS' + config_suffix + '}' }} )
set({{ file_name }}_INCLUDE_DIR {{ '${' + pkg_name + '_INCLUDE_DIRS' + config_suffix + '}' }} )
set({{ file_name }}_LIBRARIES {{ '${' + pkg_name + '_LIBRARIES' + config_suffix + '}' }} )
set({{ file_name }}_DEFINITIONS {{ '${' + pkg_name + '_DEFINITIONS' + config_suffix + '}' }} )
{% endif %}

set({{ version_variables_prefix }}_VERSION_STRING "{{ version }}")
{% if version_variables_prefix != file_name %}
set({{ version_variables_prefix }}_VERSION "{{ version }}")
set({{ version_variables_prefix }}_VERSION_MAJOR "{{ version.major or 0 }}")
set({{ version_variables_prefix }}_VERSION_MINOR "{{ version.minor or 0 }}")
set({{ version_variables_prefix }}_VERSION_PATCH "{{ version.patch or 0 }}")
set({{ version_variables_prefix }}_VERSION_TWEAK "{{ version.micro or 0 }}")
set({{ version_variables_prefix }}_VERSION_COUNT "{{ len(version.items) }}")
set({{ version_variables_prefix }}_FOUND 1)
{% endif %}
set({{ result_variables_prefix }}_INCLUDE_DIRS {{ '${' + pkg_name + '_INCLUDE_DIRS' + config_suffix + '}' }} )
set({{ result_variables_prefix }}_INCLUDE_DIR {{ '${' + pkg_name + '_INCLUDE_DIRS' + config_suffix + '}' }} )
set({{ result_variables_prefix }}_LIBRARIES {{ '${' + pkg_name + '_LIBRARIES' + config_suffix + '}' }} )
set({{ result_variables_prefix }}_LIBRARY {{ '${' + pkg_name + '_LIBRARIES' + config_suffix + '}' }} )
set({{ result_variables_prefix }}_DEFINITIONS {{ '${' + pkg_name + '_DEFINITIONS' + config_suffix + '}' }} )

I not check how Conan generates Find<PackageName>.cmake files, but this should be considered for this case as well.

Usage in conan recipes

PackeName == Resutl Variables == Version Variables: no change

    name = "postgre-sql"

    def package_info(self):
        self.cpp_info.set_property("cmake_file_name", "PostgreSQL")

PackeName != Resutl Variables == Version Variables: use cmake_result_variables_name

    name = "openjpeg"

    def package_info(self):
        self.cpp_info.set_property("cmake_file_name", "OpenJPEG")
        self.cpp_info.set_property("cmake_result_variables_name", "OPENJPEG")

PackeName != Resutl Variables != Version Variables: use cmake_result_variables_name and cmake_version_variables_name

    name = "pythonlib"

    def package_info(self):
        self.cpp_info.set_property("cmake_file_name", "Python")
        self.cpp_info.set_property("cmake_result_variables_name", "PYTHON")
        self.cpp_info.set_property("cmake_version_variables_name", "PYTHONLIB")

PackeName != Resutl Variables == Version Variables + extra variables: use cmake_result_variables_name + conan-official-{name}-variables.cmake

    name = "openssl"

    def package_info(self):
        self.cpp_info.set_property("cmake_file_name", "Python")
        self.cpp_info.set_property("cmake_result_variables_name", "OPENSSL")

    def _create_cmake_module_variables(self, module_file):
        content = textwrap.dedent("""\
            if(DEFINED OpenSSL_Crypto_LIBS)
                set(OPENSSL_CRYPTO_LIBRARY ${OpenSSL_Crypto_LIBS})
                set(OPENSSL_CRYPTO_LIBRARIES ${OpenSSL_Crypto_LIBS}
                                             ${OpenSSL_Crypto_DEPENDENCIES}
                                             ${OpenSSL_Crypto_FRAMEWORKS}
                                             ${OpenSSL_Crypto_SYSTEM_LIBS})
            elseif(DEFINED openssl_OpenSSL_Crypto_LIBS_%(config)s)
                set(OPENSSL_CRYPTO_LIBRARY ${openssl_OpenSSL_Crypto_LIBS_%(config)s})
                set(OPENSSL_CRYPTO_LIBRARIES ${openssl_OpenSSL_Crypto_LIBS_%(config)s}
                                             ${openssl_OpenSSL_Crypto_DEPENDENCIES_%(config)s}
                                             ${openssl_OpenSSL_Crypto_FRAMEWORKS_%(config)s}
                                             ${openssl_OpenSSL_Crypto_SYSTEM_LIBS_%(config)s})
            endif()
            if(DEFINED OpenSSL_SSL_LIBS)
                set(OPENSSL_SSL_LIBRARY ${OpenSSL_SSL_LIBS})
                set(OPENSSL_SSL_LIBRARIES ${OpenSSL_SSL_LIBS}
                                          ${OpenSSL_SSL_DEPENDENCIES}
                                          ${OpenSSL_SSL_FRAMEWORKS}
                                          ${OpenSSL_SSL_SYSTEM_LIBS})
            elseif(DEFINED openssl_OpenSSL_SSL_LIBS_%(config)s)
                set(OPENSSL_SSL_LIBRARY ${openssl_OpenSSL_SSL_LIBS_%(config)s})
                set(OPENSSL_SSL_LIBRARIES ${openssl_OpenSSL_SSL_LIBS_%(config)s}
                                          ${openssl_OpenSSL_SSL_DEPENDENCIES_%(config)s}
                                          ${openssl_OpenSSL_SSL_FRAMEWORKS_%(config)s}
                                          ${openssl_OpenSSL_SSL_SYSTEM_LIBS_%(config)s})
            endif()
        """% {"config":str(self.settings.build_type).upper()})
        save(self, module_file, content)

Have you read the CONTRIBUTING guide?

jcar87 commented 1 year ago

Hi @hekrdla - thank you for raising this issue.

Unfortunately there isn't a uniform convention with regards to these variables - so no matter which solution we go for, some packages may end up with variables names that do not patch pre-existing conventions.

Conan tries to follow the conventions specified in the CMake documentation here: https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#standard-variable-names

which mentions the following:

Note that all variables start with Xxx_, which (unless otherwise noted) must match exactly the name of the FindXxx.cmake file, including upper/lowercase. This prefix on the variable names ensures that they do not conflict with variables of other Find modules. The same pattern should also be followed for any macros, functions and imported targets defined by the Find module.

This convention is not always followed by library maintainers, and I suspect it may not have been uniformly followed by CMake-provided find modules.

CMake guidelines also overwhelmingly recommend the use of imported targets - so beyond the fact that the variables don't follow convention, it is also unfortunate that they are used at all.

self.cpp_info.set_property("cmake_result_variables_name", "OPENJPEG")

I think this is a good idea and something for us to consider - I would probably opt for a different property name, such as cmake_legacy_variable_prefix, where you could have:

self.cpp_info.set_property("cmake_file_name", "OpenJPEG")
self.cpp_info.set_property("cmake_legacy_variable_prefix", "OPENJPEG")

To achieve the desired result.

I don't think more complexity is needed. For example:

Please also note that sometimes the same package is used differently by consumers (different variable names, internal find modules), and that that the generator can be tweaked on the consumer side (not just in the package_info()), precisely in order to avoid patches (Which we all agree are cumbersome to maintain): https://docs.conan.io/2/reference/tools/cmake/cmakedeps.html#overwrite-properties-from-the-consumer-side-using-cmakedeps-set-property

hekrdla commented 1 year ago

@jcar87 I agree with you that all of that should be deprecated since CMake 3.0.0 2014 in favor of targets. But here we are in 2023 and still deal with it :-(

I know that this will not cover everything, but this should cover a lot. And if not, there is still possibility to use conan-official-{name}-variables.cmake like openssl do. This should be preferred way, instead of patching usage (if usage expect official naming). Because as I see, these patches miss a lot and even if they cover everything they will require a lot more maintenance in feature.

I don't check all CMake built-in finders, but what I see they respect these inconsistencies and deprecated thinks, because they should. Even <PackageName>_LIBRARY or VERISON_... related variables are there, because if they don't, these finders will not work as someone may expect.

edit: sad example how <PackageName>_LIBRARY is not intended to be used externally: https://github.com/conan-io/conan-center-index/blob/master/recipes/leptonica/all/patches/fix-find-modules-variables.patch

I agree that FindPythonLibs is weird one and maybe only one.

List of variables I propose are most common, as I know. And my premise is that defining variable that not exists in original Config.cmake is no issue, because nobody should expect it, so it will not used. But not defining variable that should exists may be problem, because it may mean different behavior instead of error.

I hope something like that will be added in whatever way.

SpaceIm commented 1 year ago

I've submitted one month ago a fix for openjpeg recipe (https://github.com/conan-io/conan-center-index/pull/19230), providing these variables in file generated by CMakeDeps, and fixing opencv issue when jpeg2000 backend is openjpeg (issue reported in https://github.com/conan-io/conan-center-index/issues/19219). It's just waiting for one team review.

And FYI, regarding extra CMake variables, there was https://github.com/conan-io/conan/issues/7691, but it has been closed since it was referring to legacy cmake_find_package* generators.