Skip to content

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