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

Document how to preserve PkgConfig data from package method #3811

Open
wants to merge 7 commits into
base: release/2.6
Choose a base branch
from
Open
Changes from 5 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
49 changes: 48 additions & 1 deletion reference/tools/gnu/pkgconfig.rst
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@ Read a ``pc`` file and access the information:
print(pkg_config.variables['prefix']) # something like'/usr/local'


Use the ``pc`` file information to fill a ``cpp_info`` object:
Using PkgConfig to fill a ``cpp_info`` object
---------------------------------------------

The ``PkgConfig`` class can be used to fill a ``CppInfo`` object with the information that will be consumed by the ``PkgConfigDeps`` generator later.
This is a useful feature when packaging a proprietary package has a build system that only outputs ``.pc`` files for a known environment.

.. code-block:: python

Expand All @@ -35,6 +38,50 @@ Use the ``pc`` file information to fill a ``cpp_info`` object:
pkg_config.fill_cpp_info(self.cpp_info, is_system=False, system_libs=["m", "rt"])


However, ``PkgConfig`` will invoke the ``pkg-config`` executable to extract the information from the ``.pc`` file.
The ``pkg-config`` executable must be available in the system path for this case, otherwise, it will fail when installing the consumed package.


Using pkg-config from Conan package instead of system
Copy link
Member

Choose a reason for hiding this comment

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

I'd say using pkg-config from a Conan tool_requires.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, changes addressed to the commit c313918

-----------------------------------------------------

.. include:: ../../../common/experimental_warning.inc

In case of not having ``pkg-config`` available in the system, it is possible to use the ``pkg-config`` executable provided by a Conan package:

.. code-block:: python

import os
from conan import ConanFile
from conan.tools.gnu import PkgConfig
from conan.tools import CppInfo

class Pkg(ConanFile):

def build_requirements(self):
self.tool_requires("pkgconf/[*]")

...

def package(self):
pkg_config = PkgConfig(self, "libastral", pkg_config_path=".")
cpp_info = CppInfo(self)
pkg_config.fill_cpp_info(cpp_info, is_system=False, system_libs=["m", "rt"])
cpp_info.save(os.path.join(self.package_folder, "cpp_info.json"))

def package_info(self):
self.cpp_info = CppInfo(self).load(os.path.join(self.package_folder, "cpp_info.json"))
Comment on lines +69 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a "system" requirement (rather than parsing the .pc output generated inside a package folder), this can cause issues on systems where the contents of .pc files can change with system updates - and I wouldn't recommend it. This could end up with a situation where the cpp_info.json inside a pacakge contains information about a system dependency that is wrong and out of sync with what the system actually has - and could be quite difficult to troubleshoot.

This is less likely to happens on distros that keep a closed set of versions for a given version of the distro, but it can be problematic on distros that have rolling version updates, or if a user generates a package today, and tries to use it after a dist-upgrade.

Remember that a call to pkg-config to locate "foo" may cause pkg-config to locate other libraries for transitive dependencies - and these can change across versions. I wouldn't recommend this for system dependencies.

On a Linux system, while inconvenient, I see no issue recommending that pkg-config is installed at the system level

Copy link
Member

Choose a reason for hiding this comment

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

I totally agree this feature should be exclusively for compiled packages, not system dependencies. What are the use cases in ConanCenter?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the motivating issue in Conan Center is precisely system packages (e.g. OpenGL). I can see 10 such cases in Conan Center where pkg_config.fill_cpp_info is called inside package_info()

Copy link
Member

Choose a reason for hiding this comment

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

Thats bad. The only way it could be allowed is by marking these packages as upload = "skip" and build_policy = "missing", to make sure they are always locally created in the target machine, but that .json file is never shared or uploaded.

Copy link
Contributor

@jcar87 jcar87 Aug 13, 2024

Choose a reason for hiding this comment

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

Ha! I would haver never thought of that - that would work - the dependency on the Conan-provided pkfcong would satisfy this at the time of use, I think.

My alterantive was more blunt: if a "system" recipe lists the system package manager requirements, and we need pkg-config to parse the .pc files... just install pkg-config as part of the system dependency set... in each of those 10 recipes. At the end of the day, if a user is completely happy doing tools.system.package_manager:mode=install, then I can't really see a scenario where they are happy doing that, but they draw the line at having pkg-config installed at the system level. I imagine that what we want here is convenience, but I don't think this is a pressing issue - we should do a better job documenting (in Conan Center, not necessarily in Conan) - which tools users are expected to have installed, and this is one of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this is a "system" requirement (rather than parsing the .pc output generated inside a package folder), this can cause issues on systems where the contents of .pc files can change with system updates

Agreed. Real example: CCI packages are using Ubuntu 16.04, the package vaapi uses the version 1.x. Ubuntu 22.04 supports the library 2.x. They have same library names etc, but are ABI incompatible.

The only way it could be allowed is by marking these packages as upload = "skip" and build_policy = "missing", to make sure they are always locally created in the target machine, but that .json file is never shared or uploaded.

Are you saying for any scenario? Should it be included by default in the documentation as well?

My alterantive was more blunt: if a "system" recipe lists the system package manager requirements, and we need pkg-config to parse the .pc files... just install pkg-config as part of the system dependency set... in each of those 10 recipes. At the end of the day, if a user is completely happy doing tools.system.package_manager:mode=install, then I can't really see a scenario where they are happy doing that, but they draw the line at having pkg-config installed at the system level.

Yes, totally true, for who is configuring a CI or build environment, is just one more command line to install it. Users have their excuses like: It's a dependency manager, but does not solve pkg-config as requirement, etc ...

In the past we needed to document about CMake not being present in every recipe as build requirement, because we had PRs adding it by default. I see pkg-config a very similar case. https://github.com/conan-io/conan-center-index/blob/f31b98bae9c200494240f9b475c16e055ff8077e/docs/faqs.md#why-recipes-that-use-build-tools-like-cmake-that-have-packages-in-conan-center-do-not-use-it-as-a-build-require-by-default

Copy link
Member

Choose a reason for hiding this comment

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

The main issue is that the upload_policy = "skip" is only Conan 2, so it will be incorrect in Conan 1.X, so we cannot allow this until we have a Conan 2-only pipeline.

Copy link
Member Author

Choose a reason for hiding this comment

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

But this documentation will be merged to Conan 2.x only (target is release/2.6). We have others features only compatible for Conan 2.x in docs. For CCI I got that we can not use it, because our main problem are system packages.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, as this was a ConanCenter driven thing, I was talking about CCI



The ``pkg-config`` executable provided by the Conan package ``pkgconf`` will be invoked only when creating the Conan binary package.
The ``.pc`` information will be extracted from the ``cpp_info.json`` file located in the package folder, it will fill the ``self.cpp_info`` object.
This way, the ``PkgConfig`` will not need to invoke the ``pkg-config`` executable again to extract the information from the ``.pc`` file,
when consuming the package.

.. warning::

It's not recommended to use this approach when creating a "system" package recipe, as the packaged information may not be compatible with the host system,
Copy link
Member

Choose a reason for hiding this comment

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

I would clarify:

  • not recommended -> forbidden to upload/reuse
  • It might be used in system packages with the upload_policy = "skip"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, changes addressed to the commit c313918

resulting in errors when consuming the package.

Reference
---------
Expand Down