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.

Sign up for GitHub

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

Jump to bottom

Include libxml2 and zlib as required libraries #847

Merged
merged 1 commit into from Mar 20, 2018
Merged

Include libxml2 and zlib as required libraries #847

merged 1 commit into from Mar 20, 2018

Conversation

walac
Copy link
Contributor

@walac walac commented Mar 20, 2018 •
edited

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@walac
Include libxml2 and zlib as required libraries
543952e 
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 changed the title Include libxml2 as a required package Include libxml2 and zlib as required libraries Mar 20, 2018
@walac
Copy link
Contributor Author

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
bnoordhuis approved these changes Mar 20, 2018
View reviewed changes
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM FWIW

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

thanks!

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

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@walac @bnoordhuis @andrewrk