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

[libpng] Fix android build #11280

Merged
merged 1 commit into from
May 13, 2020
Merged

Conversation

huangqinjin
Copy link
Contributor

No description provided.

@pthom
Copy link
Contributor

pthom commented May 9, 2020

Hello, I tested this PR on my side, it fixes the issue I was complaining about here (when trying to build opencv4 for android, which require png).
Thanks!

@cenit
Copy link
Contributor

cenit commented May 9, 2020

when merged, I will cherry pick this commit into my PR so that @pthom can continue tests with opencv4 and png

@atkawa7
Copy link
Contributor

atkawa7 commented May 10, 2020

@huangqinjin @pthom @cenit Current patch on mine which doesn't affect the cmakelist

diff --git a/ports/libpng/portfile.cmake b/ports/libpng/portfile.cmake
index e9a200455..e89ff15cf 100644
--- a/ports/libpng/portfile.cmake
+++ b/ports/libpng/portfile.cmake
@@ -47,11 +47,16 @@ else()
     set(PNG_SHARED_LIBS OFF)
 endif()

+if(VCPKG_CMAKE_SYSTEM_NAME STREQUAL Android)
+    set(HAVE_LD_VERSION_OPTION "-DHAVE_LD_VERSION_SCRIPT=OFF")
+endif()
+
 vcpkg_configure_cmake(
     SOURCE_PATH ${SOURCE_PATH}
     PREFER_NINJA
     OPTIONS
         ${LIBPNG_APNG_OPTION}
+        ${HAVE_LD_VERSION_OPTION}
         -DPNG_STATIC=${PNG_STATIC_LIBS}
         -DPNG_SHARED=${PNG_SHARED_LIBS}
         -DPNG_TESTS=OFF

@huangqinjin
Copy link
Contributor Author

https://github.com/glennrp/libpng/blob/6dd99ca9c85a59cc8848553177b84abea228f82f/CMakeLists.txt#L425 generates libpng.vers if not (NOT AWK OR ANDROID), so I think it's a bug @atkawa7 .

@atkawa7
Copy link
Contributor

atkawa7 commented May 10, 2020

https://github.com/glennrp/libpng/blob/6dd99ca9c85a59cc8848553177b84abea228f82f/CMakeLists.txt#L425 generates libpng.vers if not (NOT AWK OR ANDROID), so I think it's a bug @atkawa7 .

There wasn't any need to include a patch as this option HAVE_LD_VERSION_SCRIPT option can be used to turn it off

@huangqinjin
Copy link
Contributor Author

It's better if we can fix it without patch from vcpkg's point of view. But I think it's a bug which shoule be fixed in upstream. Actually there is a cmake option ld-version-script can be used. I reworked this PR to use it.

@PhoebeHui PhoebeHui self-assigned this May 11, 2020
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label May 11, 2020
@strega-nil
Copy link
Contributor

This conflicts with #11162; can you update the version to -9, and fix the merge conflict?

@strega-nil strega-nil added waiting for response and removed info:reviewed Pull Request changes follow basic guidelines labels May 11, 2020
@huangqinjin
Copy link
Contributor Author

@strega-nil Done.

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels May 13, 2020
@strega-nil
Copy link
Contributor

Awesome, thanks @huangqinjin. This looks great :)

@strega-nil strega-nil merged commit 5970385 into microsoft:master May 13, 2020
@huangqinjin huangqinjin deleted the android-libpng branch May 14, 2020 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants