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

Include libxml2 and zlib as required libraries #847

Merged
merged 1 commit into from Mar 20, 2018

Conversation

Projects
None yet
3 participants
@walac
Contributor

walac commented Mar 20, 2018

libxml2 is a required library, but we only find out that when the build
fails to link against it if it is not present. The same for zlib.

We use find_library to find these two libraries and print nice fail
messages if they are not found.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Mar 20, 2018

Seems to fail on Windows because of this:

CMake Error at C:/Program Files (x86)/CMake/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find LibXml2 (missing: LIBXML2_LIBRARY LIBXML2_INCLUDE_DIR)
Call Stack (most recent call first):
  C:/Program Files (x86)/CMake/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files (x86)/CMake/share/cmake-3.10/Modules/FindLibXml2.cmake:85 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:51 (find_package)

LLVM's Windows builds probably link statically to libxml2.

There's a target_link_libraries(zig LINK_PUBLIC xml2) on line 713 guarded on if(NOT MSVC) that can probably be removed in this PR. If you also add detection for zlib, then the note about zlib and xml2 in the readme can be removed.

@walac

This comment has been minimized.

Contributor

walac commented Mar 20, 2018

There's a target_link_libraries(zig LINK_PUBLIC xml2) on line 713 guarded on if(NOT MSVC) that can probably be removed in this PR. If you also add detection for zlib, then the note about zlib and xml2 in the readme can be removed.

Hrm, I am afraid I didn't follow the logic here. Shouldn't I add an if (NOT MSVC) clause to avoid the find_package(LibXml2) call on Windows?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Mar 20, 2018

Yes, and when you do, the target_link_libraries stanza can probably go.

Include libxml2 and zlib as required libraries
libxml2 is a required library, but we only find out that when the build
fails to link against it, if it is not present. The same for zlib.

We use find_library to find these two libraries and print nice fail
messages if they are not found.

@walac walac force-pushed the walac:master branch from de1d4ec to 543952e Mar 20, 2018

@walac walac changed the title from Include libxml2 as a required package to Include libxml2 and zlib as required libraries Mar 20, 2018

@walac

This comment has been minimized.

Contributor

walac commented Mar 20, 2018

Yes, and when you do, the target_link_libraries stanza can probably go.

I realized that the build doesn't need the header files, so instead of find_package, I changed it to use find_library.

@bnoordhuis

LGTM FWIW

@andrewrk andrewrk merged commit 1369fec into ziglang:master Mar 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andrewrk

This comment has been minimized.

Member

andrewrk commented Mar 20, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment