-
Notifications
You must be signed in to change notification settings - Fork 361
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
uilianries
wants to merge
7
commits into
conan-io:release/2.6
Choose a base branch
from
uilianries:pkgconf-package
base: release/2.6
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c6c8f72
Document pkg-config running from package
uilianries 31b19ec
Grammar fix
uilianries 95bb6e5
Grammar fix
uilianries 9b1f9ef
Fix example code using tool_requires
uilianries e23b7c2
Do not recommend system package usage
uilianries c313918
Update pkg-config recommendation
uilianries a1141bb
Merge branch 'release/2.6' into pkgconf-package
czoido File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 thecpp_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.
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?
There was a problem hiding this comment.
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 insidepackage_info()
There was a problem hiding this comment.
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"
andbuild_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.
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 installpkg-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 doingtools.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Are you saying for any scenario? Should it be included by default in the documentation as well?
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.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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