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

examples: add zlib dependency #973

Closed
wants to merge 2 commits into from
Closed

examples: add zlib dependency #973

wants to merge 2 commits into from

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Oct 7, 2023

Resolves #972

@bradh bradh mentioned this pull request Oct 7, 2023
Closed
kmilos
kmilos reviewed Oct 8, 2023
View reviewed changes
Copy link
Contributor

@kmilos kmilos left a comment

Choose a reason for hiding this comment

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

Alternatively, maybe it's time to bump CMake minimum required version and simplifyby using imported targets ZLIB::ZLIB and PNG::PNG...

examples/CMakeLists.txt Outdated
@@ -119,8 +118,9 @@ if (PNG_FOUND)
../libheif/exif.cc
common.cc
common.h)
target_link_libraries(heif-thumbnailer heif ${PNG_LIBRARIES})
target_link_libraries(heif-thumbnailer heif ${PNG_LIBRARIES} ${ZLIB_LIBRARY})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target_link_libraries(heif-thumbnailer heif ${PNG_LIBRARIES} ${ZLIB_LIBRARY})
target_link_libraries(heif-thumbnailer heif ${PNG_LIBRARIES} ${ZLIB_LIBRARIES})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I copied-n-pasted from https://github.com/strukturag/libheif/blob/master/libheif/CMakeLists.txt#L117

Will fix both.

@farindk
Copy link
Contributor

farindk commented Oct 8, 2023

Alternatively, maybe it's time to bump CMake minimum required version and simplifyby using imported targets ZLIB::ZLIB and PNG::PNG...

Do you know what minimum cmake version is required for that? I think the current upper limit might be cmake v3.16.3 (Ubuntu 20.04).

@kmilos
Copy link
Contributor

kmilos commented Oct 8, 2023

3.1 for ZLIB, 3.5 for PNG

farindk added a commit that referenced this pull request Oct 9, 2023
@farindk
cmake: prefer ZLIB_LIBRARIES to ZLIB_LIBRARY (#973)
2d35b04
@farindk
Copy link
Contributor

farindk commented Oct 9, 2023

The #include <zlib.h> in #972 is actually not needed. There is no dependency on zlib (except when header compression is turned on). I have removed the unnecessary include, this PR is not needed. I have still changed ZLIB_LIBRARY to ZLIB_LIBRARIES.

@farindk farindk closed this Oct 9, 2023
@farindk farindk mentioned this pull request Oct 9, 2023
Closed
@bradh bradh deleted the zlib_examples branch October 9, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmilos kmilos kmilos left review comments

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

Successfully merging this pull request may close these issues.

1.16.2: build fails because missing zlib.h
3 participants
@bradh @farindk @kmilos