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] fix wrapper #19033

Merged
merged 3 commits into from Jul 21, 2021
Merged

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Jul 21, 2021

somehow LIBXML2_LIBRARY_DEBUG and LIBXML2_LIBRARY_RELEASE were the wrong way around

find_library(LIBXML2_LIBRARY_RELEASE NAMES xml2 libxml2 xml2s libxml2s xml2d libxml2d xml2sd libxml2sd NAMES_PER_DIR PATH_SUFFIXES lib PATHS "${_INSTALLED_DIR}/debug" NO_DEFAULT_PATH)
find_library(LIBXML2_LIBRARY_DEBUG NAMES xml2 libxml2 xml2s libxml2s NAMES_PER_DIR PATH_SUFFIXES lib PATHS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}" NO_DEFAULT_PATH)
find_library(LIBXML2_LIBRARY_DEBUG NAMES xml2 libxml2 xml2s libxml2s xml2d libxml2d xml2sd libxml2sd NAMES_PER_DIR PATH_SUFFIXES lib PATHS "${_INSTALLED_DIR}/debug" NO_DEFAULT_PATH)
find_library(LIBXML2_LIBRARY_RELEASE NAMES xml2 libxml2 xml2s libxml2s NAMES_PER_DIR PATH_SUFFIXES lib PATHS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}" NO_DEFAULT_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there is only a single candidate path, I feel like getting rid of NAMES_PER_DIR PATH_SUFFIXES lib would improve readability (and possibly facilitate spotting such errors).

find_library(LIBXML2_LIBRARY_DEBUG NAMES xml2 libxml2 xml2s libxml2s xml2d libxml2d xml2sd libxml2sd PATHS "${_INSTALLED_DIR}/debug/lib" NO_DEFAULT_PATH)
find_library(LIBXML2_LIBRARY_RELEASE NAMES xml2 libxml2 xml2s libxml2s PATHS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib" NO_DEFAULT_PATH)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.
the only thing fixing these problems would be a z_vcpkg_find_package function with the correct set of parameters automatically searching for both variants.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. This somewhat overlaps with my proposed vcpkg_find_libraries from #18914 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #18032

@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Jul 21, 2021
@BillyONeal BillyONeal merged commit c690966 into microsoft:master Jul 21, 2021
@BillyONeal
Copy link
Member

Thanks for your contribution!

@Neumann-A Neumann-A deleted the fix_libxml2_wrapper branch July 22, 2021 07:02
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Jul 22, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants