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

[feature] cmake_find_generator[multi]: add more "alias" variables #7691

Closed
1 task done
SpaceIm opened this issue Sep 9, 2020 · 13 comments
Closed
1 task done

[feature] cmake_find_generator[multi]: add more "alias" variables #7691

SpaceIm opened this issue Sep 9, 2020 · 13 comments

Comments

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 9, 2020

Some official CMake module files have uppercase variables (most of them actually), even if filename or namespace is not uppercase, but cmake_find_generator and cmake_find_generator_multi generate variables like OpenSSL_INCLUDE_DIRS.

So let's look at official FindOpenSSL module file: https://cmake.org/cmake/help/latest/module/FindOpenSSL.html

Current openssl CCI recipe will generate FindOpenSSL.cmake file if consumer uses cmake_find_generator generator, which is fine.

But generated variables are OpenSSL_FOUND, OpenSSL_INCLUDE_DIR, OpenSSL_INCLUDE_DIRS, OpenSSL_LIBRARIES etc. Unfortunately, in official CMake module file, all these variables are fully uppercase, and CMake variables are case sensitive.

Also sometimes there is a variable like OPENSSL_LIBRARY (not in OpenSSL official CMake module, it's just an example), or JASPER_VERSION_STRING instead of JASPER_VERSION.

It would be nice to add those variables in generated module and config files:

  • ${names or filenames}_LIBRARY (clone of ${names or filenames}_LIBRARIES).
  • ${names or filenames}_VERSION_STRING (clone of ${names or filenames}_VERSION).
  • maybe others common variant.
  • clone of all generated CMake variables, fully uppercase.

Some recipes are hard to properly package with cmake_find_package, without introducing tedious patches applying non official CMake variables names. Yes, because some official CMake modules files don't define targets, even in CMake 3.18, so the proper and transparent way to consume these libraries is to use those variables...

@jgsogo jgsogo self-assigned this Sep 10, 2020
@jgsogo
Copy link
Contributor

jgsogo commented Sep 10, 2020

Hi, @SpaceIm

IMHO, we should limit Conan features to the common functionality shared by different build-systems, something that every one of them can take advantage of: names, filenames,... although some of them are already too CMake-centric (but it is ok, CMake is one of the biggest players if not the biggest one). Following this reasoning, I think that cpp_info is more-or-less feature complete regarding names and we should think about how to implement this very specific things.

For CMake, we are including the build_modeles into every CMake file generated by the generators. I think that these non-standard variables (because of the casing, because targets without namespaces,...) can and should be implemented there: Conan generates the canonical files, then the build_modules (specific to the recipe) provide these extra variables (probably we need #7261).

Of course, I'm open to other alternatives, but if we agree on this and the solution is as easy as moving the inclusion of the build_modules to the bottom of the FindXXXX.cmake files, then it is something for the next release.

wdyt? Do you really think we need more attributes in the package_info() function?

Thanks for your feedback


I know there is a big disadvantage: anything in the build_modules is not known to Conan (we are not going to parse those files) and we cannot show that information in the markdown generator or any other file/information generated by Conan. Nevertheless, I think there is a future where every recipe can provide a README.md and we can render that information in the ConanCenter web UI. 😊

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 10, 2020

My request was not very clear maybe.
I'm not asking for more feature in package_info (like injecting exotic variables, which should be done through build_modules), but to improve cmake_find_package and cmake_find_package_multi generators, so that they automatically provide common additional variables (without adding anything in package_info) in generated CMake module and config files.

So for example with jasper:

Official CMake module file: https://cmake.org/cmake/help/v3.18/module/FindJasper.html

JASPER_FOUND - system has Jasper
JASPER_INCLUDE_DIR - the Jasper include directory
JASPER_LIBRARIES - the libraries needed to use Jasper
JASPER_VERSION_STRING - the version of Jasper found (since CMake 2.8.8)

jasper conan recipe:

Content of package_info:

    def package_info(self):
        self.cpp_info.names["cmake_find_package"] = "Jasper"
        self.cpp_info.names["cmake_find_package_multi"] = "Jasper"
        self.cpp_info.names["pkg_config"] = "jasper"
        self.cpp_info.libs = ["jasper"]
        if self.settings.os == "Linux":
            self.cpp_info.system_libs.append("m")

Generated module file (just a subpart), if using cmake_find_package:

set(Jasper_INCLUDE_DIRS "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(Jasper_INCLUDE_DIR "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(Jasper_INCLUDES "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")

I would like that conan generator adds more variables commonly found in official module files:

set(Jasper_INCLUDE_DIRS "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(JASPER_INCLUDE_DIRS "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(Jasper_INCLUDE_DIR "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(JASPER_INCLUDE_DIR "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(Jasper_INCLUDES "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")
set(JASPER_INCLUDES "C:/Users/spaceim/.conan/data/jasper/2.0.19/_/_/package/5b4f006245c66309d6bcbdbb4fe6daef48420dbc/include")

Here I would like to also have:

  • Jasper_FOUND and JASPER_FOUND
  • Jasper_LIBRARIES, JASPER_LIBRARIES, Jasper_LIBRARY and JASPER_LIBRARY
  • Jasper_VERSION, JASPER_VERSION, Jasper_VERSION_STRING and JASPER_VERSION_STRING
  • and maybe also: Jasper_LIBRARY_DIRS and JASPER_LIBRARY_DIRS

Uppercase variables are common in official CMake module files (but for exemple https://cmake.org/cmake/help/v3.18/module/FindPostgreSQL.html or https://cmake.org/cmake/help/v3.18/module/FindXercesC.html are exceptions).

For version you will equally find these variants: ${libname}_VERSION_STRING and ${libname}_VERSION
Same for libraries: ${libname}_LIBRARIES and ${libname}_LIBRARY
where ${libname} is either equal to module filename (without the Found) or uppercase.

If conan made the decision in the first place to create ${libname}_VERSION_STRING in generated module/config files, I would have asked to add also ${libname}_VERSION, that's all.

@jgsogo
Copy link
Contributor

jgsogo commented Sep 10, 2020

There is also the cpp_info.filename, which is the one used right now for the xxx_FOUND and other variables... so the feature would generate the four combinations: upper/lowercase and names matching the cpp_info.names and cpp_info.filename... increasing the risk of collision.

As a C++ developer, I would love to see some standardization in all these naming, but I know the world is different. If I were to vote, I would say that Conan should provide the standard variables and any other variable should be provided by the recipe via build_modules. This is my motto, to improve the C++ ecosystem... but I'm not alone in this task.

I would really want to know what other people think about it. Maybe we should post this question in the conan-center-index repository, there we will find more people aware of this problem than here in the conan repo.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 17, 2020

I can open a discussion in conan-center-index, but I strongly believe that the new variables proposed in this PR are standard variables.
Here you can see some comments in a PR of @ohanar, where the module file generated by conan won't be correct because those variables are missing:
https://github.com/conan-io/conan-center-index/blob/d6aca94d6823d9fa439c616a2e7d070915c99fc1/recipes/openscenegraph/all/conanfile.py#L240
Here the recipe tries to mimic a module file generated by upstream, not by Kitware, but those variables are standard, you'll find them in many module files provided by Kitware.

There are a lot of recipes in CCI for which we have to patch a valid upstream CMakeLists because those variables are missing, or worse sometimes it silently fails in upstream CMakeLists at some point just because cmake_find_package has generated a module file with OpenSSL_FOUND instead of OPENSSL_FOUND.
So we end up by not using cmake_find_package generator, with the risk that official module file finds system lib instead of conan lib.

@jgsogo
Copy link
Contributor

jgsogo commented Sep 17, 2020

👍 Let's gather some attention on this issue, and see what others think about it.

@ohanar
Copy link
Contributor

ohanar commented Sep 17, 2020

I agree with @SpaceIm, there is a ton of inconsistency in the CMake ecosystem about whether to use {name}_FOUND vs {filename}_FOUND or {name}_LIBRARIES vs {filename}_LIBRARIES, etc. Rather than arbitrarily choosing things like {filename}_FOUND and {name}_INCLUDE_DIRS as conan currently does, I think we should just define both {name}_XXX and {filename}_XXX for all relevant XXX. These are so standard in package scripts that I don't think they should be relegated to build modules.

That said, I do agree with @jgsogo that cpp_info should be more or less done (mainly excluding #6125 and #7240), and that pretty much all other CMake specific stuff should go into build modules.

@memsharded
Copy link
Member

My personal opinion: I really hate how CMake is that much inconsistent, and still the community pushes Conan to conform to such bad practices instead of pushing CMake or accepting having to do some simple modifications in the consumer scripts.

If Conan introduces all the different casing for Jasper_INCLUDE_DIRS, JASPER_INCLUDE_DIRS, this will perpetuate the bad practice and extend it to all packages, even those that use the "standard" casing. This can make the issue actually worse than it is in the current situation.

If I understand it correctly, this would only apply when the variables are used instead of using the generated targets with is actually the recommended approach, isn't it? Shouldn't we be pushing towards the usage of targets as much as possible? Are there strong reasons to not use the targets? If we decided not to automatically generate those variables, how many packages in conan center would require custom build_modules to add those variables?

@ohanar
Copy link
Contributor

ohanar commented Sep 17, 2020

The problem is one of scale, we aren't really talking about 10s of packages, more like hundreds or thousands, so in my opinion Conan's goal should be first and foremost compatibility (not just for CMake, but also with pkg-config, qmake, meson, etc). I agree that there are a bunch of bad practices with CMake, but that is true for any heavily used piece of software -- I personally have to deal with 20+ year-old C++ code on a daily basis, so I certainly sympathize with pain of dealing with compatibility.

To answer your question, yes, using the generated targets is the recommended approach, but there are plenty of packages that don't use them, especially when it comes to older packages (both as consumers and producers). The example of Jasper in particular is a case where the official script does not provide any CMake target, so the entire CMake ecosystem uses variables when consuming Jasper.

As for what conan center packages would require the variables, at the very least it would include everything shipped with CMake (https://cmake.org/cmake/help/v3.18/manual/cmake-modules.7.html#find-modules). Some of these provide CMake targets, but many don't and only provide variables. My guess is that there wouldn't be many more packages outside of this list that really need the variables.

@ericriff
Copy link

I'll include my grain of salt to this issue.
FindBoost.cmake generates the following variables: Boost_<C>_FOUND, Boost_<C>_LIBRARY where <C> is each one of the components. These variables are not generated by cmake_find_package, which breaks some projects that rely on them, like GTSAM

Also FindProtobuf.cmake defines at least one variable (Protobuf_PROTOC_EXECUTABLE) that is not generated by conan generators, which makes conan integration not straightforward on projects that uses protoc through that variable.

I would like to see a way for conan recipes to specify custom variables for the cmake and cmake_find_package generators.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 17, 2021

If I understand it correctly, this would only apply when the variables are used instead of using the generated targets with is actually the recommended approach, isn't it? Shouldn't we be pushing towards the usage of targets as much as possible? Are there strong reasons to not use the targets? If we decided not to automatically generate those variables, how many packages in conan center would require custom build_modules to add those variables?

Built-in CMake modules don't always provide imported targets, and if they do it requires sometimes recent CMake versions that projects can't use as a minimum version:

I guess with conan 1.33.0, we may add custom CMake module files to properly set additional variables for cmake_find_package[_multi] generators, but it adds extra complexity in recipes and pollutes packages with strange module files.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 17, 2021

Here is an attempt to provide official variables in openssl recipe: conan-io/conan-center-index#4597

@mathbunnyru
Copy link

mathbunnyru commented Mar 19, 2021

List if libraries, which might be also affected:
geotiff
libunwind
zstd
gdal

https://github.com/conan-io/conan-center-index/blob/master/recipes/pdal/all/patches/0001-cmakelists.patch

@memsharded
Copy link
Member

This issue was reporting the legacy CMake integration, which has been superseded by the new one in Conan 1.X https://docs.conan.io/1/reference/conanfile/tools.html, and removed in Conan 2.0: https://docs.conan.io/2/reference/tools/cmake.html. Closing this ticket as outdated.

Please update to use the modern CMake integration, and please don't hesitate to open new tickets as necessary. Thanks!

@memsharded memsharded closed this as not planned Won't fix, can't repro, duplicate, stale Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants