Document how to preserve PkgConfig data from package method #3811
Conversation
Signed-off-by: Uilian Ries <[email protected]>
Co-authored-by: Abril Rincón Blanco <[email protected]>
Co-authored-by: Abril Rincón Blanco <[email protected]>
Co-authored-by: Abril Rincón Blanco <[email protected]>
| 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")) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I totally agree this feature should be exclusively for compiled packages, not system dependencies. What are the use cases in ConanCenter?
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure, as this was a ConanCenter driven thing, I was talking about CCI
Signed-off-by: Uilian Ries <[email protected]>
|
@jcar87 @memsharded I just addressed your comments into the commit e23b7c2
Please, review it again. |
reference/tools/gnu/pkgconfig.rst
Outdated
|
|
||
| .. 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, |
There was a problem hiding this comment.
I would clarify:
- not recommended -> forbidden to upload/reuse
- It might be used in system packages with the
upload_policy = "skip"
There was a problem hiding this comment.
Done, changes addressed to the commit c313918
reference/tools/gnu/pkgconfig.rst
Outdated
| 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 |
There was a problem hiding this comment.
I'd say using pkg-config from a Conan tool_requires.
There was a problem hiding this comment.
Done, changes addressed to the commit c313918
Signed-off-by: Uilian Ries <[email protected]>
uilianries
force-pushed
the
pkgconf-package
branch
from
88e229c
to
c313918
Compare
Hello!
Originally we have few issues in ConanCenterIndex affected by the scenario where
pkg-configis not installed in the system, so users can not consume those packages that are simple wrappers for system libraries:Indeed I was caught by this error when using a raw docker image to validate a reported bug in Ubuntu 24.04. This is my motivation for writing this PR.
As possible alternative (experimental), we can run
PkgConfigfrompackage()and store its data in a json file, then, restore that information inself.cpp_info. This workaround was commented by Memsharded 1 year ago: conan-io/conan#14422 (comment)This scenario is tested already in Conan client, but not well explored, still, is working in minimal case: https://github.com/conan-io/conan/blob/c55ab23d01f1336c850816e07c68ae19704201d6/test/functional/toolchains/gnu/test_pkg_config.py#L99
Related to conan-io/conan#14422.
Current preview (commit c313918):
EDIT:
After those comments pointed at #3811 (review), I removed the part of "This is a useful feature when packaging a system library" and added an warning box about not using this new approach for system package due possible incompatibility.