Also generate cmake usages for pkgconfig files #1268

autoantwort · 2023-11-05T21:43:06Z

Before:

➜  vcpkg git:(test-features) ✗ vcpkg x-set-installed --enforce-port-checks 'icu'
Detecting compiler hash for triplet arm64-osx...
All requested packages are currently installed.
Total install time: 1.54 us

After:

➜  vcpkg git:(test-features) ✗ vcpkg x-set-installed --enforce-port-checks 'icu'
Detecting compiler hash for triplet arm64-osx...
All requested packages are currently installed.
Total install time: 1.58 us
icu can be imported via CMake FindPkgConfig module:

    find_package(PkgConfig)
    # International Components for Unicode: Internationalization library
    pkg_check_modules(icu-i18n REQUIRED IMPORTED_TARGET icu-i18n)
    target_link_libraries(main PkgConfig::icu-i18n)

    # International Components for Unicode: Stream and I/O Library
    pkg_check_modules(icu-io REQUIRED IMPORTED_TARGET icu-io)
    target_link_libraries(main PkgConfig::icu-io)

    # International Components for Unicode: Common and Data libraries
    pkg_check_modules(icu-uc REQUIRED IMPORTED_TARGET icu-uc)
    target_link_libraries(main PkgConfig::icu-uc)

Before: ``` ➜ vcpkg git:(test-features) ✗ vcpkg x-set-installed --enforce-port-checks 'icu' Detecting compiler hash for triplet arm64-osx... All requested packages are currently installed. Total install time: 1.54 us ``` After: ``` ➜ vcpkg git:(test-features) ✗ vcpkg x-set-installed --enforce-port-checks 'icu' Detecting compiler hash for triplet arm64-osx... All requested packages are currently installed. Total install time: 1.58 us icu can be imported via CMake FindPkgConfig module: find_package(PkgConfig) # International Components for Unicode: Internationalization library pkg_check_modules(icu-i18n REQUIRED IMPORTED_TARGET icu-i18n) target_link_libraries(main PkgConfig::icu-i18n) # International Components for Unicode: Stream and I/O Library pkg_check_modules(icu-io REQUIRED IMPORTED_TARGET icu-io) target_link_libraries(main PkgConfig::icu-io) # International Components for Unicode: Common and Data libraries pkg_check_modules(icu-uc REQUIRED IMPORTED_TARGET icu-uc) target_link_libraries(main PkgConfig::icu-uc) ```

dg0yt · 2023-11-07T08:36:25Z

pkg_check_modules(icu-uc REQUIRED IMPORTED_TARGET icu-uc)
target_link_libraries(main PkgConfig::icu-uc)

A good occasion to make a conscious decision: Choose the above, or choose:

pkg_check_modules(ICU_UC REQUIRED IMPORTED_TARGET icu-uc)
target_link_libraries(main PkgConfig::ICU_UC)

The first is more legible for the suggested usage. The second makes it clear where the target name comes from, and it is more consistent when also using the variables (ICU_UC_INCLUDE_DIRS).

BillyONeal · 2023-11-07T23:25:21Z

Last time the vcpkg maintainers discussed this topic, we opposed adding CMake instructions based on pkgconfig files. For pkgconfig, we should be providing pkgconfig instructions.

In particular, using pkgconfig dependencies like this from CMake can create problems for folks who themselves are trying to ship CMake configs, as the pkgconfig dependency space and the CMake dependency space are disjoint.

See previous discussion microsoft/vcpkg#33001 (comment)

@ras0219-msft suggests that the standard should be:

If there are CMake configs present, they should be displayed. (Even if there is pkgconfig)
If there are pkgconfig files present, they should be displayed. (Even if there are CMake configs)
We should not display instructions to try to adapt pkgconfig into CMake.

Also present in discussion: @JavierMatosD @vicroms @data-queue who agreed with the above.

Overall though we agree that helping folks use packages that provide pkgconfig is a good thing, thanks for your contribution there!

dg0yt · 2023-11-08T04:22:22Z

Does this imply that usage files should also consider both worlds independently?

dg0yt · 2023-11-08T04:23:40Z

Does this imply that unofficial cmake config is preferred over official pkgconfig?

JavierMatosD · 2023-11-08T20:13:10Z

Does this imply that usage files should also consider both worlds independently?

Yes

Does this imply that unofficial cmake config is preferred over official pkgconfig?

Sort of – since our tool is largely CMake-based, the unofficial CMake configs just fit into our workflow more smoothly than the official pkgconfig does. (generated usage should give neither preference over the other)

dg0yt · 2023-11-09T05:27:47Z

I'm just worried about the intrusiveness and lock-in effect of unofficial config, and the detrimental effect on installing and fixing official pkgconfig.
At least please push upstreaming.

BillyONeal · 2023-11-09T19:20:28Z

I'm just worried about the intrusiveness and lock-in effect of unofficial config, and the detrimental effect on installing and fixing official pkgconfig.

I think the intent is not 'burn pkgconfig', but 'if we are patching a downstream anyway, and we have the choice between unofficial cmake targets and pkgconfig, we want to choose the unofficial targets, because pkgconfig and cmake configs are separate universes that don't join together cleanly'.

At least please push upstreaming.

Absolutely!

BillyONeal

(See previous comment)

…es` themselves and can't blame vcpkg if `pkg_check_modules` does not work.

autoantwort · 2023-11-12T02:17:22Z

Output is now

➜  vcpkg git:(test-features) ✗ vcpkg x-set-installed --enforce-port-checks 'icu'
Detecting compiler hash for triplet arm64-osx...
All requested packages are currently installed.
Total install time: 1.67 us
icu provides pkg-config modules:

    # International Components for Unicode: Internationalization library
    icu-i18n

    # International Components for Unicode: Stream and I/O Library
    icu-io

    # International Components for Unicode: Common and Data libraries
    icu-uc

locales/messages.json

Thomas1664 · 2023-11-16T11:51:02Z

include/vcpkg/base/message-data.inc.h

@@ -482,6 +482,7 @@ DECLARE_MESSAGE(CISwitchOptSkipFailures,
                "Skips ports marked `=fail` in ci.baseline.txt")
 DECLARE_MESSAGE(CISwitchOptXUnitAll, (), "", "Reports unchanged ports in the XUnit output")
 DECLARE_MESSAGE(ClearingContents, (msg::path), "", "Clearing contents of {path}")
+DECLARE_MESSAGE(CMakePkgConfigTargetsUsage, (msg::package_name), "", "{package_name} provides pkg-config modules:")


Should pkg-config be localized as well?

Thomas1664 · 2023-11-16T11:52:00Z

src/vcpkg/commands.install.cpp

+                    {
+                        if (Strings::starts_with(line, "Description: "))
+                        {
+                            Strings::append(msg, "    # ", line.substr(StringLiteral("Description: ").size()), '\n');


Suggested change

Strings::append(msg, " # ", line.substr(StringLiteral("Description: ").size()), '\n');

Strings::append(msg, " # ", StringView(line).substr(StringLiteral("Description: ").size()), '\n');

Construction of a temporary std::string can be avoided.

Furthermore, I think it's worth converting the StringLiteral to a static constexpr variable.

BillyONeal · 2023-11-27T19:57:15Z

Can you add a test with a minimal test port that should engage this logic? (I'll push one for CMake configs here....)

…-pkgconfig

BillyONeal

I added a test for CMake configs; please copy pasta vcpkg-hello-world-1 or vcpkg-empty-port, make it write a .pc you're looking for here, and add to the lists in usage.ps1. Then this LGTM I think

The problems I enumerated before got fixed.

BillyONeal · 2023-12-05T21:19:53Z

Couple of nitpicks / questions:

I tried changing your example to wrong-pkgconfig[core,debug-good,header-only-good,lib-good]

and now it says:

wrong-pkgconfig provides pkg-config modules:

    # Test lib
    wrong-pkgconfig

    # Test lib
    wrong-pkgconfig

which seems wrong?

Should we say that this result is a heuristic?
Should the comment derived from 'Description' added after the name of the package rather than before?

autoantwort · 2023-12-05T21:25:51Z

which seems wrong?

Because the features of the port install pkgconfig files in share/pkgconfig and lib/pkgconfig which no normal port do (you get an postbuildcheck error).

Should we say that this result is a heuristic?

But this is not really a heuristic like the cmake one. That are the real pkgconfig modules.

Should the comment derived from 'Description' added after the name of the package rather than before?

I could do that. As long as the people don't thing that the # part is part of the module name :D

BillyONeal · 2023-12-05T21:36:45Z

Because the features of the port install pkgconfig files in share/pkgconfig and lib/pkgconfig which no normal port do (you get an postbuildcheck error).

Makes sense. (I kind of just assumed the feature was named good so it must be good)

But this is not really a heuristic like the cmake one. That are the real pkgconfig modules.

👍

I could do that. As long as the people don't thing that the # part is part of the module name :D

Up to you.

autoantwort · 2023-12-05T21:38:45Z

Up to you.

I think I prefer the current style :)

BillyONeal · 2023-12-05T22:00:03Z

Thanks for the feature!

dg0yt mentioned this pull request Nov 9, 2023

[luajit] add usage microsoft/vcpkg#34991

Closed

7 tasks

BillyONeal previously requested changes Nov 10, 2023

View reviewed changes

autoantwort marked this pull request as draft November 11, 2023 01:26

autoantwort added 3 commits November 12, 2023 02:38

Merge branch 'main' into explain-how-to-import-pkgconfig

7c84e48

Make message less helpful so that users have to find `pkg_check_modul…

3055c3e

…es` themselves and can't blame vcpkg if `pkg_check_modules` does not work.

Display cmake and pkgconfig usage

dff8f41

autoantwort marked this pull request as ready for review November 12, 2023 02:15

dg0yt reviewed Nov 12, 2023

View reviewed changes

locales/messages.json Outdated Show resolved Hide resolved

Message from dg0yt

7fe6db9

autoantwort requested a review from BillyONeal November 15, 2023 10:33

Thomas1664 reviewed Nov 16, 2023

View reviewed changes

autoantwort marked this pull request as draft November 27, 2023 20:59

BillyONeal added 6 commits November 27, 2023 13:40

dos2unix all the things except for those that should have CRLFs

00d3ecb

Fix version DB.

e5323b1

Merge branch 'dos2unix' into explain-how-to-import-pkgconfig

6645d40

Add e2e tests for CMake configs.

a731b20

Merge remote-tracking branch 'origin/main' into explain-how-to-import…

b645fe8

…-pkgconfig

Also add explicit usage text e2e test.

6bc0049

BillyONeal reviewed Nov 27, 2023

View reviewed changes

autoantwort added 2 commits December 2, 2023 22:19

Add e2e test for pkgconfig usage and detect pkgconfig in "share"

522f50f

Save

e77477c

autoantwort marked this pull request as ready for review December 2, 2023 21:22

BillyONeal merged commit 5fbcab7 into microsoft:main Dec 5, 2023
5 checks passed

This was referenced Jan 4, 2024

[duktape] Optimize build script and provide duktape targets microsoft/vcpkg#35813

Closed

[libofx] Fix opensp link method microsoft/vcpkg#35873

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Also generate cmake usages for pkgconfig files #1268

Also generate cmake usages for pkgconfig files #1268

autoantwort commented Nov 5, 2023

dg0yt commented Nov 7, 2023

BillyONeal commented Nov 7, 2023

dg0yt commented Nov 8, 2023

dg0yt commented Nov 8, 2023

JavierMatosD commented Nov 8, 2023

dg0yt commented Nov 9, 2023

BillyONeal commented Nov 9, 2023

BillyONeal left a comment

autoantwort commented Nov 12, 2023 •

edited

Loading

Thomas1664 Nov 16, 2023

Thomas1664 Nov 16, 2023

Thomas1664 Nov 16, 2023

BillyONeal commented Nov 27, 2023

BillyONeal left a comment

BillyONeal commented Dec 5, 2023

autoantwort commented Dec 5, 2023

BillyONeal commented Dec 5, 2023

autoantwort commented Dec 5, 2023

BillyONeal commented Dec 5, 2023

	Strings::append(msg, " # ", line.substr(StringLiteral("Description: ").size()), '\n');
	Strings::append(msg, " # ", StringView(line).substr(StringLiteral("Description: ").size()), '\n');

Also generate cmake usages for pkgconfig files #1268

Also generate cmake usages for pkgconfig files #1268

Conversation

autoantwort commented Nov 5, 2023

dg0yt commented Nov 7, 2023

BillyONeal commented Nov 7, 2023

dg0yt commented Nov 8, 2023

dg0yt commented Nov 8, 2023

JavierMatosD commented Nov 8, 2023

dg0yt commented Nov 9, 2023

BillyONeal commented Nov 9, 2023

BillyONeal left a comment

Choose a reason for hiding this comment

autoantwort commented Nov 12, 2023 • edited Loading

Thomas1664 Nov 16, 2023

Choose a reason for hiding this comment

Thomas1664 Nov 16, 2023

Choose a reason for hiding this comment

Thomas1664 Nov 16, 2023

Choose a reason for hiding this comment

BillyONeal commented Nov 27, 2023

BillyONeal left a comment

Choose a reason for hiding this comment

BillyONeal commented Dec 5, 2023

autoantwort commented Dec 5, 2023

BillyONeal commented Dec 5, 2023

autoantwort commented Dec 5, 2023

BillyONeal commented Dec 5, 2023

autoantwort commented Nov 12, 2023 •

edited

Loading