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] Update to latest commit, remove wrapper #14588

Closed
wants to merge 26 commits into from

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Nov 16, 2020

  • Update to latest commit to use the internal CMakeLists.txt
  • Remove vcpkg-cmake-wrapper and usage since the official already provide the cmake target

Replaced PR #14478.
Fixes #14468.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Nov 16, 2020
@JackBoosY JackBoosY requested a review from strega-nil November 16, 2020 08:30
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Nov 16, 2020

cc @Neumann-A for review this PR.
cc @jmelas for test this PR.

@traversaro
Copy link
Contributor

FYI the upstream libxml2 project now has an official CMake build system (see https://gitlab.gnome.org/GNOME/libxml2/-/blob/master/CMakeLists.txt), and even the autotools build system installs a CMake config file (see https://gitlab.gnome.org/GNOME/libxml2/-/blob/master/libxml2-config.cmake.in). I am a bit afraid that adding a third vcpkg-specific variant will complicate life of libraries maintainers that need to support all this variants.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Nov 16, 2020

@traversaro That's awesome, I will try to update libxml2 to use that (but it's not included in official release).

@JackBoosY JackBoosY marked this pull request as draft November 16, 2020 09:08
@Neumann-A
Copy link
Contributor

is there a reason not to switch to vcpkg_configure_make ? (Assuming there are problems with upstreams CMakeLists.txt)

@JackBoosY
Copy link
Contributor Author

@Neumann-A If CMakelists.txt has no errors, we prefer to use cmake to build all ports.

@JackBoosY JackBoosY changed the title [libxml2] Export unofficial target, correct wrapper and usage [libxml2] Update to latest commit, remove wrapper and usage Nov 16, 2020
@JackBoosY JackBoosY changed the title [libxml2] Update to latest commit, remove wrapper and usage [libxml2] Update to latest commit, remove wrapper Nov 17, 2020
@Neumann-A
Copy link
Contributor

no no no..... don't do that. You are patching the hell out of downstream ports for some unnecessary reason. Fix the CMake FindLibXml2.cmake module with a vcpkg-cmake-wrapper.cmake and be done with it!

You can override all manual find_library calls by doing a find_library call yourself before the find_package() call since find_library sets a CACHE variable.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Nov 18, 2020

@Neumann-A I've already realized that...
The official provided cmake is incomplete, I think we should wait for upstream fix these issues.

Will open another PR to fix the related bug.

@JackBoosY JackBoosY added the depends:upstream-changes Waiting on a change to the upstream project label Nov 18, 2020
@JackBoosY JackBoosY closed this Nov 19, 2020
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 depends:upstream-changes Waiting on a change to the upstream project info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libxml2 vcpkg-cmake-wrapper.cmake bugs
4 participants