[vcpkg] Avoid duplication of Libs in vcpkg_fixup_pkgconfig

[dg0yt]

• What does your PR fix?

  This PR modifies vcpkg_fixup_pkgconfig, static linkage, to not duplicate Libs.private in Libs if its already present there.

  E.g. for curl, x64-linux, instead of
  Libs: -L"${libdir}" -lcurl-d -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz
  curl.pc is now:
  Libs: -L"${libdir}" -lcurl-d -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz

  This helps when reviewing build logs (as I'm doing for gdal at the moment).

• 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?

  -/-

[NancyLi1013]

cc @Neumann-A Could you please help review this PR?

Thanks.

[NancyLi1013]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ras0219-msft]

Edit: I wrote this looking at the output in your top comment, not actually looking at the implementation. Looking at the implementation, this may be fine.

I think you need to keep the last instance of a link -- consider the following:

A -> B, C
B -> D
C -> D

the duplicated list would look like

A B D C D

With keeping the first instance, you have

A B D C

and any symbols from D needed by C will not be found (because D isn't linked after it).

If instead the last instance is kept,

A B C D

We get the desired behavior

[ras0219-msft]

This assumes a space will be present after the _libs_private sequence in Libs:

[dg0yt]

Note that there is an extra space before and after ${CMAKE_MATCH_1} here, so the assumption will hold.

[mcmtroffaes]

Sorry for the late comment here. I think this work is great, and I very much appreciate any improvements to the pkgconfig situation in vcpkg.

However, I wonder if it's worth trying to patch the curl .pc file for vcpkg and/or get this patched upstream, as I cannot think of a reason why any flag would ever need to be specified in both Libs and Libs.private (although see curl/curl#5373 which seems to be the origin of the duplication). An educated guess tells me that only -lcurl needs to be listed in Libs, with the rest suitable for Libs.private. Either way, proper tagging of the libraries would help reducing the number of linked libraries in dynamic builds.

This could also be related: https://curl.se/docs/knownbugs.html#curl_config_libs_contains_priv

[dg0yt]

@mcmtroffaes https://curl.se/docs/knownbugs.html#curl_config_libs_contains_priv is unrelated because it is about LDFLAGS feed into curl configuration being carried through into the pc file AFAIU.

curl is doing its attributes right already, and that's why I don't want to patch curl.
vcpkg_fixup_pkgconfig is the offender which appends private attributes to default attributes when building with static linkage.

I don't want to claim this is the only possible approach.
Edit: It is for the lazy/casual portfile author who doesn't know or care.

There are alternatives:

• Add an option to vcpkg_fixup_pkgconfig to stop it from touchingcopying private attributes. For ports where the software does it right.
  Edit: Needs action from the portfile author.
• Never touchcopy private attributes, but rely on proper use of pkg-config with --static argument.
  Edit: Should be the case in vcpkg, but might need patches for some ports.
• Start using per-triplet pkg-config wrappers which ensure the right environment variables and arguments for the actual pkg-config tool, independent of invoking script.
  What is to keep in mind is that vcpkg aims to support non-default linkage for custom ports.

[mcmtroffaes]

Many thanks for the explanation and educating me, and I apologize if I'm a bit slow on the uptake. I can see now why having identical Libs and Libs.private entries for statically built libraries makes sense, say if you're building an application which intends to link dynamically against its dependencies (thus, might call pkg-config without --static), but must still link against some static dependency, which will use its Libs and not its Libs.private. I don't think most projects even try to support this scenario: the assumption appears to be that static libraries are only used when the entire project is built statically.

Now, what vcpkg is trying to do is to get rid of Libs.private entirely, and just have the correct link flags specified in Libs for the triplet being used. Since there are distinct triplets and thus distinct .pc files for static and dynamic builds anyway, this works just fine.

I hope I'm summarizing this correctly, and would be happy to stand corrected.

[dg0yt]

My explanations here are also written down for review. I have difficulties to find good examples of how to use pkg-config correctly in some cases. At least when mixing static and dynamic linkage, I would love to find a good reference.

[Neumann-A]

Never touchcopy private attributes, but rely on proper use of pkg-config with --static argument.
Edit: Should be the case in vcpkg, but might need patches for some ports.

the idea is to never have to pass --static in vcpkg since a consumer cannot know if a dependent lib has been built statically. The only reason --static even exists is due to linux distros often building shared&static libraries at the same time.

[dg0yt]

the idea is to never have to pass --static in vcpkg since a consumer cannot know if a dependent lib has been built statically. The only reason --static even exists is due to linux distros often building shared&static libraries at the same time.

Thanks for clarification. This seem to be a conscious decision for vcpkg, not just an idea. I would like to see this written down in a reference document, at least in the vcpkg_fixup_pkgconfig documentation.

At the moment, vcpkg_configure_make.cmake still adds --static. Without reference documentation, it is impossible to know it the idea is still valid or if it was abandoned.

[Neumann-A]

At the moment, vcpkg_configure_make.cmake still adds --static. Without reference documentation, it is impossible to know it the idea is still valid or if it was abandoned.

It was implemented with --static before the changes to vcpkg_fixup_pkgconfig to change the pc files to not pass --static.

Be aware most of vcpkg_configure_make.cmake as well as vcpkg_configure_meson.cmake was obtained by try and error form #9966

[dg0yt]

Be aware most of vcpkg_configure_make.cmake as well as vcpkg_configure_meson.cmake was obtained by try and error form #9966

Yeah, I digged the history a few times. It is a little bit hard to contribute because of competing PRs and long CI runs. But it is getting off-topic.

[NancyLi1013]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[NancyLi1013]

spdlog failed on x64-osx with the following errors:

CMake Error at scripts/cmake/vcpkg_fixup_pkgconfig.cmake:79 (message):
  pkg-config error output:Package 'spdlog' has no Name: field
Call Stack (most recent call first):
  scripts/cmake/vcpkg_fixup_pkgconfig.cmake:187 (vcpkg_fixup_pkgconfig_check_files)
  ports/spdlog/portfile.cmake:28 (vcpkg_fixup_pkgconfig)
  scripts/ports.cmake:139 (include)

[dg0yt]

As far as curl is concerned, the duplication is going to be resolved by #18971.

[dg0yt]

The curl particularities are solved now in port curl. I don't think this PR is still needed. Most ports seem to need pc file patching anyway. Better contribute back upstream than micro-optimize here.