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

[RFC] Feature: add cpp_info.generators #6587

Closed
wants to merge 1 commit into from

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Feb 25, 2020

Changelog: Feature: add self.cpp_info.generators
Docs: https://github.com/conan-io/docs/pull/XXXX (not yet)

Fixes #6069
Fixes conan-io/conan-center-index#419
Addresses #6329

This pull requests add a generators attribute to self.cpp_info in the package_info() method.
This allows to pass generator specific properties.
As a RFC, I have implemented adding targets.
This allows to create OpenSSL:Crypto and OpenSSL::SSL targets as documented here.

The syntax that I have implemented is:

def package_info(self):
    self.cpp_info.libs = ["crypto", "ssl"]
    # self.cpp_info.generators["cmake_find_package"].targets["OpenSSL::SSL"] has the same attributes as self.cpp_info
    self.cpp_info.generators["cmake_find_package"].targets["OpenSSL::Crypto"].libs = ["crypto"]
    self.cpp_info.generators["cmake_find_package"].targets["OpenSSL::SSL"].libs = ["ssl"]
  • This is very unfinished. But I put it here to receive comments on syntax/what/how/where.
  • For now, I have put a lot of logic in conans/model/gen_info.py,
    a good separation between model and client (generator) should be designed. There is already a loose connection, but I think that the _CMakeFindPackageTarget class that I created in gen_info.py has a lot of shared code with _CppInfo.
  • The cmake generator code should also be refactored a bit so code between creation of NAME::NAME and the specialized targets can be shared a bit more.
  • If some target has a property unspecialized, it uses the property from self.cpp_info.
    That's why I added a ModificationTrackingList. This object provides a default list and tracks modifications.

Todo's:

  • refactor

  • add cmake_find_package_multi support

  • add cmake variables to cmake_find_package and cmake_find_package_multi

  • add pkg_config support

  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.

  • I've read the Contributing guide.

  • I've followed the PEP8 style guides for Python code.

  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@Morwenn
Copy link
Contributor

Morwenn commented Feb 25, 2020

With the amount of nesting needed it would make sense to start using dictionaries:

self.cpp_info.generators["cmake_find_package"] = {
    "name": "OpenSSL",
    "targets": {
        "OpenSSL::Crypto": { "libs": ["crypto"] },
        "OpenSSL::SSL": { "libs": ["ssl"] }
    }
}

With the most recent Python version it should be possible to provide type hints to statically check the structure somehow. We also have hooks for that, so the structure checking could be done, but every generator would need to specify some structure format somehow.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My most important comment here, is that this looks too specific. What we are considering now is the definition of abstract "components", that will map to targets under cmake generators, but to other things in other generators.

This doesn't look the best approach. If you want very specific build system information, you could be creating the find_xxx.cmake files directly and using them. They are only restricted in ConanCenter. But precisely this new feature in ConanCenter will be problematic, because it will degenerate to something that works under one build system with one specific generator, but will likely fail with the others, and there is practically nothing we can do to automate checking that, and that will impose too much load on the review process.

My quick feedback, lets wait what the rest thinks.

conans/client/generators/cmake_common.py Outdated Show resolved Hide resolved
conans/client/generators/cmake_find_package_common.py Outdated Show resolved Hide resolved
conans/model/build_info.py Outdated Show resolved Hide resolved
conans/model/build_info.py Outdated Show resolved Hide resolved
conans/model/conan_generator.py Outdated Show resolved Hide resolved
@madebr
Copy link
Contributor Author

madebr commented Feb 25, 2020

My most important comment here, is that this looks too specific. What we are considering now is the definition of abstract "components", that will map to targets under cmake generators, but to other things in other generators.

I want to point out that a self.cpp_info.generators["cmake_find_package"].targets["OpenSSL::Crypto"] object is almost equivalent to self.cpp_info.
Both have the following attributes: libs, ìncludedirs, libdirs, bindirs, cxxflags, cflags, ... This object can thus be re-used for pkg_config`.
The only generator-specific thing is the generator name and the name of the target.
Another syntax might be useful.

Granted, adding cmake variables is generator specific.

This doesn't look the best approach.

A better syntax would help a lot, imho.

If you want very specific build system information, you could be creating the find_xxx.cmake files directly and using them.

Creating FindXX.cmake ourselves will work fine for simple cases, but will fail fast when using more complex generators such as cmake_find_package_multi.
Have a look at conan-io/conan-center-index#486 for a botched attempt.

They are only restricted in ConanCenter. But precisely this new feature in ConanCenter will be problematic, because it will degenerate to something that works under one build system with one specific generator, but will likely fail with the others, and there is practically nothing we can do to automate checking that, and that will impose too much load on the review process.

Isn't that the reason why we add test_package folders beside recipes?

As a side note, I want to point out that currently, the tests at CCI only test the cmake and cmake_find_package generator.

My quick feedback, lets wait what the rest thinks.

Thanks for the quick reply!

@gocarlos
Copy link

My most important comment here, is that this looks too specific. What we are considering now is the definition of abstract "components", that will map to targets under cmake generators, but to other things in other generators.

This doesn't look the best approach. If you want very specific build system information, you could be creating the find_xxx.cmake files directly and using them. They are only restricted in ConanCenter. But precisely this new feature in ConanCenter will be problematic, because it will degenerate to something that works under one build system with one specific generator, but will likely fail with the others, and there is practically nothing we can do to automate checking that, and that will impose too much load on the review process.

My quick feedback, lets wait what the rest thinks.

I think we need a solution for this soon and therefore pragmatic, a ton of people having issues with libs like openssl, protoc, reproc etc because there are multiple targets in the repo.
The solution from @madebr fix this without giving away the auto generated findPackage.cmake scripts

@madebr madebr closed this May 6, 2020
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

Successfully merging this pull request may close these issues.

[question] Mimicking CMake Find Modules [feature] CMake generator: custom target
4 participants