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

[libxml2] Improve pc file #17747

Merged
merged 5 commits into from
May 19, 2021
Merged

[libxml2] Improve pc file #17747

merged 5 commits into from
May 19, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 8, 2021

  • What does your PR fix?

    The pc file did not list any extra libraries needed for static linking. This PR adds them, e.g. for x64-linux-dbg:
    Libs.private: -pthread -lm -L/vcpkg/installed/x64-linux/debug/lib/pkgconfig/../../lib -llzmad -pthread -lz
    This helps with fixes the libxml2 test in gdal configure.

    In addition, the PR also shortcuts a copying step introduced in my previous PR.

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

    yes

  • Remarks

    • Similar configuration is done in upstream HEAD's CMakeLists.txt. CC @JackBoosY
    • Some requirements are collected via x_vcpkg_pkgconfig_get_modules, in order to easily to resolve recursively and with debug postfixes as needed. CC @Neumann-A

@dg0yt
Copy link
Contributor Author

dg0yt commented May 8, 2021

Probably there is also a dependency on CMAKE_DL_LIBS, given that HAVE_DLOPEN is defined for linux and osx. Not sure if this desired for static builds at all.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 8, 2021

x64_windows_static_md: paraview: FAILED: Qt/Components/pqComponents_autogen/timestamp - seems unrelated.

@Neumann-A
Copy link
Contributor

Neumann-A commented May 8, 2021

x64_windows_static_md: paraview: FAILED: Qt/Components/pqComponents_autogen/timestamp - seems unrelated.

@dg0yt: Fixed by #17706. Is a CMake autogen regression

@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label May 10, 2021
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 10, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented May 10, 2021

Thanks for reviewing.

After submitting the PR, I started wondering whether this usage of x_vcpkg_pkgconfig_get_modules isn't a anti-pattern:
The result is only used to insert Libs into the pc file. This result can probably also be achieved just by adding corresponding Requires: (or Requires.private: ?) modules to the the pc file template. This would avoid the expensive extra acquisition and run of pkg-config when building the port.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 12, 2021

This result can probably also be achieved just by adding corresponding Requires: (or Requires.private: ?) modules to the the pc file template.

Confirmed. I will update the PR unless someone objects.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 16, 2021

adding corresponding Requires: (or Requires.private: ?) modules to the the pc file template.

Done: Requires.private.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 16, 2021

#17945 will also solve the pc file problem. If that PR is ready for merge soon, this PR becomes obsolete.

@JackBoosY
Copy link
Contributor

I prefer to merge this PR first because my PR needs to update cmake to the latest.

@dan-shaw dan-shaw merged commit 5554e3f into microsoft:master May 19, 2021
@dg0yt dg0yt deleted the libxml2 branch May 29, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants