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] Add link libraries for target LibXml2::LibXml2 #14478

Closed
wants to merge 5 commits into from

Conversation

LilyWangL
Copy link
Contributor

Describe the pull request

  • What does your PR fix? Fixes libxml2 vcpkg-cmake-wrapper.cmake bugs #14468
    Target LibXml2::LibXml2 is not linked against all necessary libraries in vcpkg-cmake-wrapper.cmake. Target LibXml2::LibXml2 should link with Iconv, Charset, lzma, zlib, ws2_32 (windows only) libraries.

@LilyWangL LilyWangL 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 9, 2020
@LilyWangL LilyWangL marked this pull request as ready for review November 10, 2020 01:52
@LilyWangL LilyWangL added the info:reviewed Pull Request changes follow basic guidelines label Nov 10, 2020
Copy link

@jmelas jmelas left a comment

Choose a reason for hiding this comment

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

I think still LIBXML2_LIBRARIES is not populated correctly.
The missing libraries should first be added to LIBXML2_LIBRARIES variable
and then target LibXml2::LibXml2 should link with this variable.

@LilyWangL
Copy link
Contributor Author

I think still LIBXML2_LIBRARIES is not populated correctly.
The missing libraries should first be added to LIBXML2_LIBRARIES variable
and then target LibXml2::LibXml2 should link with this variable.

Due to libiconv and libcharset don't create lib on Linux, so it need not link to them when CMAKE_SYSTEM_NAME STREQUAL "Linux". I think this handle is correct.

Copy link

@jmelas jmelas left a comment

Choose a reason for hiding this comment

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

Now it's broken on macOS.
This is my proposition:

if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
    list(APPEND LIBXML2_LIBRARIES m)
else()
    list(APPEND LIBXML2_LIBRARIES ${ICONV_LIBRARIES} ${CHARSET_LIBRARIES} ${LIBLZMA_LIBRARIES} ${ZLIB_LIBRARIES})
    if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
        list(APPEND LIBXML2_LIBRARIES ws2_32)
    endif()
endif()
if(TARGET LibXml2::LibXml2)
    target_link_libraries(LibXml2::LibXml2 INTERFACE ${LIBXML2_LIBRARIES})
endif()

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Nov 11, 2020
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 11, 2020
if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
list(APPEND LIBXML2_LIBRARIES ws2_32)
if(TARGET LibXml2::LibXml2)
target_link_libraries(LibXml2::LibXml2 INTERFACE ${LIBXML2_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply no. This is linking itself. LIBXML2_LIBRARIES is not empty after the find_package call but contains the libxml2 library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think I agree with this; could we switch to the old code, but add target_link_libraries below the second list(APPEND?

@strega-nil strega-nil added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Nov 12, 2020
@JackBoosY
Copy link
Contributor

Replaced by #14588.

@JackBoosY JackBoosY closed this Nov 16, 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 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
5 participants