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
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions docs/reviewing.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,20 +105,39 @@ def _configure_cmake(self):

### Minimalistic Source Code

The contents of `test_package.c` or `test_package.cpp` should be as minimal as possible, including a few headers at most with simple instantiation of objects to ensure linkage
and dependencies are correct.
The contents of `test_package.c` or `test_package.cpp` should be as minimal as possible, including a few headers at most with simple
instantiation of objects to ensure linkage and dependencies are correct. Any build system can be used to test the package, but
CMake or Meson are usually preferred.

### Verifying Components
### CMake targets

When components are defined in the `package_info` in `conanfile.py` the following conditions are desired
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.


**CMakeLists.txt**
```cmake
cmake_minimum_required(VERSION 3.1.2)
project(test_package CXX)

include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup(TARGETS)

find_package(package REQUIRED CONFIG)

add_executable(${PROJECT_NAME} test_package.cpp)
target_link_libraries(${PROJECT_NAME} package::package)
```

We encourage contributors to check that not only the _global_ target works properly, but also the ones for the components. It can be
done creating and linking different libraries and/or executables.

### Recommended feature options names
jgsogo marked this conversation as resolved.
Show resolved Hide resolved

Expand Down