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

[docs] Enforce usage of targets #7153

Merged
merged 4 commits into from
Sep 8, 2021
Merged

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Sep 2, 2021

Conan v2 is approaching fast with radical improvements around build helpers. Most of the new functionality is being backported to Conan v1.x in an effort to provide a baseline of compatible features and syntax that will work with both versions and make the migration path as smooth as possible.

You can read about these new features in the tribe repository and in the Conan documentation. The changelog is also a good place to keep updated, there you can find an exhaustive list of new features together with a link to the documentation that was changed related to it.

Regarding conan-center-index, one of the main changes will be the deprecation of (the functionality provided by) the cmake generator. This functionality will be superseded by a much more convenient approach: CMake toolchains (the new CMake build-helper will take care to inject it and it would be the same if using CMake from CLI). It means that some variables will no longer be accessible, among them it's important to notice that CONAN_LIBS will no longer be available.

We are already doing a huge effort (thanks to everyone) trying to mimic in our package_info() the information provided by the modules offered by CMake and the config files that some libraries provide. We are mimic target names, and thanks to the build-modules we can create aliases and provide extra variables.

We believe that targets are more convenient for consumers and there is no reason to keep using only the cmake generator and raw variables instead of the targets that Conan is already providing. We think that ConanCenter itself can suggest and enforce targets when they are not already enforced.

Following this lead, the mini-project in test_package can become an example of the best practices to consume the library, and not just a test for the Conan package itself. It will also align with the following releases of Conan and will make it easier to use new Conan versions and, eventually, make it possible to use the same recipes with Conan v1 and v2.

Of course, we will keep trying to mimic existing targets when they are already available and spread in the C++ ecosystem, and we will change Conan ones and modify the cpp_info information if some library starts to recommend different names and/or targets.

@conan-center-bot

This comment has been minimized.

@SSE4
Copy link
Contributor

SSE4 commented Sep 2, 2021

docs/reviewing.md Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

We think that ConanCenter itself can suggest and enforce targets when they are not already enforced.

The C++ developer in my loves the freedom to do anything, so there's a little resistance for me, but after using Golang and Node ecosystems and the position of

Conan, the C/C++ Package Manager

I have no issue with enforcing this rule. However I think it would be a very nice hook 😉

docs/reviewing.md Outdated Show resolved Hide resolved
Comment on lines 114 to 123
When using CMake to test a package, the information should be consumed using the **targets provided by `cmake_find_package_multi` generator**. We
enforce this generator to align with the upcoming
[Conan's new `CMakeDeps` generator](https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmakedeps.html?highlight=cmakedeps)
and it should help in the migration (and compatibility) with Conan v2.

* use `cmake_find_package` if library has an [official](cmake.org/cmake/help/latest/manual/cmake-modules.7.html#find-modules) CMake module emulated in the recipe.
* use `cmake_find_package_multi` if library provides an official cmake config file emulated in the recipe. If there are more than one target, try to use all of them, or add another executable linking to the global (usually unofficial) target. There may additionally be variables that are emulated by the recipe which should be used as well. There are some ways to identify when to use it:
* Usually, project installs its cmake files into `package_folder/lib/cmake`. The folder is removed from package folder by calling `tools.rmdir(os.path.join(self.package_folder), "lib", "cmake")`
* Also, the library's CMake scripts can use [install(EXPORT ..)](https://cmake.org/cmake/help/latest/command/install.html#export) and/or may install [package configuration helpers](https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html)
to indicate an exported CMake config file.
* When `self.cpp_info.filenames["cmake_find_package_multi"]`, `self.cpp_info.names["cmake_find_package_multi"]` are declared
* otherwise, use [cmake generator](https://docs.conan.io/en/latest/reference/generators/cmake.html) to not suggest an unofficial cmake target in test package.
In ConanCenter we try to mimic the names of the targets and the information provided by CMake's modules and config files that some libraries
provide. If CMake or the library itself don't enforce any target name, the ones provided by Conan should be recommended. The minimal project
in the `test_package` folder should serve as an example of the best way to consume the package, and targets are preferred over raw variables.

This rule applies for the _global_ target and for components ones. The following snippet should serve as example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest:

  • cmake_find_package_multi if there is an official config file
  • cmake_find_package if there is an official Find module file and no official config file
  • pkg_config if there is an official .pc file and no official cmake Find/config files.
  • cmake_find_package_multi otherwise

Copy link
Contributor

@SpaceIm SpaceIm Sep 3, 2021

Choose a reason for hiding this comment

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

I would also warn against relying on unofficial CMake Find/config and pkg_config files in recipes themselves, it can break at anytime (upstream suddenly decides to provide a CMake config file, and it's different than default CMake config file generated by conan, so we update this recipe accordingly, and it breaks all downstream recipes) while CONAN_PKG targets are robust.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'ld also prefer using CONAN_PKG:: targets or similar. Something in a conan namespace.
I would consider it unfriendly if some package manager would start dictating cmake targets to my project.

At least, there should be some comment added that a target is conan specific and prone to breakage.

Copy link
Member

Choose a reason for hiding this comment

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

To give a heads-up of what is happening in Conan 2.0:

  • The only CMake generator that remains is CMakeDeps and CMakeToolchain. They generate pkg::pkg and pkg::component targets, not any CONAN_LIB or CONAN_PKG targets.
  • The idea is to provide a fully transparent integration with CMake, the push for that has been massive over the years.
  • Also, to provide 1 integration for CMake, supporting many ones is is big overhead in support in maintenance, we need to converge to 1 solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Will CMakeDeps be able to generate both module and configs?
If not, that will break a lot of recipes at cci

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is already able in Conan 1.40

Copy link
Contributor

Choose a reason for hiding this comment

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

They tagged us in a PR with the feature asking for feedback :)

Copy link
Contributor

@czoido czoido Sep 6, 2021

Choose a reason for hiding this comment

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

@jgsogo
Copy link
Contributor Author

jgsogo commented Sep 6, 2021

Here we are trying to maximize compatibility with the following Conan versions. At some point in time, we will need to introduce Conan v2.x in CCI and we should try to make the recipes as compatible as possible with both versions, trying to keep if conditions at a minimum.

The cmake_find_package_multi will be the one that accommodates better to the new toolchains/helpers, that's why it should be used by default. We should also probably move to the cpp_info.set_property that will set some properties for all cmake-related generators, and the same for pkg-config one. Up to now, this should cover your first two bullet points when there is one and only one source of truth:

  • cmake_find_package_multi if there is an official config file
  • cmake_find_package if there is an official Find module file and no official config file

It doesn't really matter if we use cmake_find_package_multi or cmake_find_package if all the properties are set to both generators. Both will create the same targets with the same properties. The only difference would be the find_package(XXXXX REQUIRED CONFIG/MODULE) if it's hardcoded in the CMakeLists.txt.

When there is nothing official:

New Conan versions won't provide CONAN_PKG:: targets or ${CONAN_LIBS} variable (if we want to argue about this, let's open an issue in conan-io/conan repo), and the consumer experience will be more similar to the cmake_find_package_multi one, that's why I suggest using that one. And we will try to promote ConanCenter as a source of truth for target names (like CMake does already). ConanCenter will dictate targets to projects the same way CMake already does, and we will do it only if there is nothing already available.

At least, there should be some comment added that a target is conan specific and prone to breakage.

Can you figure out a mark we can add to recipes/cpp_info? Something that the generator can inform the consumer about?

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 6, 2021

It doesn't really matter if we use cmake_find_package_multi or cmake_find_package if all the properties are set to both generators. Both will create the same targets with the same properties. The only difference would be the find_package(XXXXX REQUIRED CONFIG/MODULE) if it's hardcoded in the CMakeLists.txt.

It depends whether you use CMakeToolchain (to set CMAKE_FIND_PACKAGE_PREFER_CONFIG ON), and whether there is a Find module provided in CMake or not. For example, for zlib recipe, you don't want to use cmake_find_package_multi because generated config file will be ignored by find_package(ZLIB) (without CMakeToolchain).

And for CCI, we package many recipes with legacy CMakeLists (sometimes it doesn't even deserve the name legacy, just that official Find file has introduced targets in very recent CMake versions, e.g CURL::libcurl in FindCURL.cmake introduced in CMake 3.12), relying on CMake variables which can't be emulated in config files due to its "multi" nature, so there is still a strong advantage of cmake_find_package to avoid patches.

@madebr
Copy link
Contributor

madebr commented Sep 6, 2021

I searched through the repo and found that some recipes generate different file names for cmake_find_package and cmake_find_package_multi: freetype, glew and protobuf.
So I hope CMakeDeps has functionality to account for this.
Both may also generate different target names.

Projects might do find_package(XXX REQUIRED CONFIG/MODULE).
I believe conan should have functionality to switch between CONFIG and MODULE transparently, eventually in the toolchain method. (added by conan 1.40.0 apparently)

At least, there should be some comment added that a target is conan specific and prone to breakage.

Can you figure out a mark we can add to recipes/cpp_info? Something that the generator can inform the consumer about?

I think conan already knows when to inform the consumer about this.
If nothing is set, then conan knows that it depends on default values.

First, conan should be able to detect whether an option is explicitly set or has a default value.
Then, something like the following logic can be added to the CMakeDeps generator:

default_values = []
for dep in conanfile.deps:
    if conanfile.deps_cpp_info[dep].names["CMakeDeps"].default_value:  # Assume every item in the names list to be a wrapper object with extra functionality
        default_values.append(dep)
    cmakedeps_generate(dep, conanfile.deps_cpp_info[dep])
if default_values and tools.get_env("CONAN_WARNING_CMAKEDEPS", False):  # Only warn about default targets when the user cares about it. Perhaps conan needs functionality comparable to `-Wall -Weverything`?
    conanfile.output.warning("The following CMakeDeps modules are not explicitly defined and are prone to breakage: {}".format(default_values)

@prince-chrismc
Copy link
Contributor

I think we can summarize the concern for

CMakeDeps and CMakeToolchain [...] generate pkg::pkg and pkg::component targets

Might not cover enough cases?

We are trying to tame the lawless C++ landscape so there are more combinations that exist.

  • What about projects installing more than on find config?
  • what about targets with different namespace?
  • what about custom variables in config files?

➡️ Are we okay not supporting all projects?

@conan-center-bot
Copy link
Collaborator

Updating docs!

@SSE4 SSE4 mentioned this pull request Sep 7, 2021
4 tasks
@prince-chrismc prince-chrismc mentioned this pull request Sep 8, 2021
4 tasks
@conan-center-bot conan-center-bot merged commit 038e1ae into conan-io:master Sep 8, 2021
@jgsogo jgsogo deleted the doc/targets branch September 10, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants