-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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 iconv and charset linkage in vcpkg-cmake-wrapper on osx #11072
[libxml2] Add iconv and charset linkage in vcpkg-cmake-wrapper on osx #11072
Conversation
find_library(CHARSET_LIBRARY_RELEASE NAMES charset libcharset NAMES_PER_DIR PATH_SUFFIXES lib PATHS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}" NO_DEFAULT_PATH) | ||
find_library(CHARSET_LIBRARY_RELEASE NAMES charset libcharset NAMES_PER_DIR PATH_SUFFIXES lib) | ||
select_library_configurations(ICONV) | ||
select_library_configurations(CHARSET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this due to we might use system iconv
or within vcpkg on different platforms?
So we don't use find_package(unofficial-iconv
to find iconv
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to avoid unofficial packages. The correct way would probably be to have pkg-config for iconv here but thats not yet integrated into vcpkg.
if(CMAKE_SYSTEM_NAME STREQUAL "Linux") | ||
list(APPEND LIBXML2_LIBRARIES m) | ||
else() | ||
list(APPEND LIBXML2_LIBRARIES ${ICONV_LIBRARIES} ${CHARSET_LIBRARIES}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is iconv
only required on Windows and osx platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least it looks like libxml is not linking against iconv on Linux or is linking against a shared version which does not require propagation. The correct fix would probably to make iconv build on all platforms and linking correctly against that.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@Neumann-A |
…c_linkage # Conflicts: # scripts/ci.baseline.txt
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This looks great! Thanks @Neumann-A :) |
closes #11001
makes #10770 work on osx