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

[vcpkg script] Test pkg-config integration #25531

Merged
merged 5 commits into from
Jul 7, 2022

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jul 2, 2022

  • What does your PR fix?

    Test user-perceived pkg-config integration in vcpkg (find_package(PkgConfig), pkg_check_modules(...)).

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    not needed.

Related

#22392 (comment)
#25529

github-actions[bot]
github-actions bot previously approved these changes Jul 2, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 2, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 2, 2022

Host pkgconf can be found reliably with CMake 3.22 FindPkgConfig.cmake (but only after system pkg-config). But there is room for problems in user projects:

  • Our tests do set VCPKG_HOST_TRIPLET. Users might miss this parameter, so that CMAKE_PROGRAM_PATH wouldn't be set to the host tools. Solution for manifest mode: Extend triplet detection in CMake toolchain #25529.
  • User's CMake before 3.22 doesn't look for pkgconf. Possible solutions:
    • Rename pkgconf[.exe] to pkg-config[.exe]. This would bring vcpkg's port in front of system tools, for all triplets, increasing decoupling from system libraries.
    • Add a pkgconfig.bat wrapper which forwards to pkgconf, for Windows.

@Neumann-A
Copy link
Contributor

User's CMake before 3.22

Don't need to care about it. System pkg-config is ok on !Windows. Windows users should update cmake any way.

Main problem with current FindPkgConfig in vcpkg is that it doesn't support multi-config generators yet. We could do that with a wrapper installed alongside with pkgconf which does overrides the functions cmake normally uses and setup the targets correctlyy dependening on config.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 3, 2022

User's CMake before 3.22

Don't need to care about it. System pkg-config is ok on !Windows. Windows users should update cmake any way.

I mostly agree, but I don't want to decide for all users.
A possible issue with system pkg-config on !windows is that it can hide problems with vcpkg pc modules.

Main problem with current FindPkgConfig in vcpkg is that it doesn't support multi-config generators yet. We could do that with a wrapper installed alongside with pkgconf which does overrides the functions cmake normally uses and setup the targets correctlyy dependening on config.

The problem with wrappers vs. port pkgconf is that wrappers are loaded for the target triplet, but pkgconf is used as a host dependency.
More generally, the problem is that you cannot know how the results of pkg_check_modules are used. As with CMake find modules (and wrappers), it is probably easy to carry multi-config with imported targets, but it is error-prone to handle multi-config for the variables. A modified behaviour wouldn't only affect the current project, but also its transitive dependencies.

(I have a local branch which additonally uses the Ninja multi-config generator for this test-port, but I didn't see a benefit for find_package tests.)

@LilyWangLL LilyWangLL added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Jul 4, 2022
@LilyWangLL LilyWangLL changed the title Test pkg-config integration [vcpkg script] Test pkg-config integration Jul 4, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 5, 2022
@dg0yt dg0yt marked this pull request as ready for review July 5, 2022 07:48
LilyWangLL
LilyWangLL previously approved these changes Jul 6, 2022
@LilyWangLL LilyWangLL requested a review from JackBoosY July 6, 2022 06:33
@dg0yt dg0yt dismissed stale reviews from LilyWangLL and github-actions via f31a6ef July 7, 2022 03:20
@LilyWangLL LilyWangLL requested a review from JackBoosY July 7, 2022 03:47
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 7, 2022
@vicroms vicroms merged commit fb3190d into microsoft:master Jul 7, 2022
@dg0yt dg0yt deleted the cmake-user-pkgconfig branch July 7, 2022 21:32
@JackBoosY
Copy link
Contributor

I think vcpkg has the ability to provide usage information for pkg-config by now.
Next we should add usage printing code about pkg-config to vcpkg-tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants