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

[curl] Fix android builds. Greatly simplify pkgconfig handling. #17418

Merged
merged 5 commits into from
May 13, 2021

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Apr 21, 2021

Includes changes from #17400. Tested using docker.io/menny/android_ndk:1.15.1.

https://github.com/menny/docker_android/tree/master/android_ndk
https://hub.docker.com/r/menny/android_ndk

$ docker pull menny/android_ndk:1.15.1

After launching the container, do:

export ANDROID_NDK_HOME=$ANDROID_HOME/ndk/22.0.7026061
git clone https://github.com/ras0219/vcpkg -b dev/roschuma/curl-android
cd vcpkg
./bootstrap-vcpkg.sh
echo "
set(VCPKG_TARGET_ARCHITECTURE arm)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE static)
set(VCPKG_CMAKE_SYSTEM_NAME Android)
" > triplets/community/arm-android.cmake
echo "
set(VCPKG_TARGET_ARCHITECTURE arm)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE dynamic)
set(VCPKG_CMAKE_SYSTEM_NAME Android)
" > triplets/community/arm-android-dynamic.cmake
./vcpkg install aws-sdk-cpp[core]:arm-android aws-sdk-cpp[core]:arm-android-dynamic --no-binarycaching

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@ahmedyarub
Copy link
Contributor

Tested on Windows and worked like a charm!

@JackBoosY JackBoosY added category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. info:internal This PR or Issue was filed by the vcpkg team. labels Apr 22, 2021
@talregev
Copy link
Contributor

talregev commented Apr 22, 2021

@ras0219 Hi Robert, your fix is working with pkg-config when we compile for android with libcurl.
curl-config is showing permission denied and may have the wrong flags.
We now switch to pkg-config compilation with vcpkg.
Thank you for this fix

@dg0yt
Copy link
Contributor

dg0yt commented Apr 25, 2021

curl-config is showing permission denied and may have the wrong flags.

The port file could set permissions with file(INSTALL ...).
IMO curl-config should go the the tools folder. It will be used by by build scripts on the host, not the target. (Example: gdal).

@talregev
Copy link
Contributor

talregev commented Apr 25, 2021

@dg0yt
Before the fix (current state of vcpkg) curl-config it didn't show permission denied. After the fix it is showing permission denied.
curl-config is also may give the wrong flags for libcurl compilation for android.
Also curl-config will not be relevant for us because we switching to compile libcurl vcpkg with android with pkg-config, as you can see on my PR that I mention this PR.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 28, 2021

Also curl-config will not relevant for us because we switching to compile libcurl vcpkg with android with pkg-config

I think it is relevant for fixing #9068 (GDAL on Linux), at least if using gdal configuration parameters.

@talregev
Copy link
Contributor

I think it is relevant for fixing #9068 (GDAL on Linux), at least if using gdal configuration parameters.

I knew that it might be relevant for other projects, so that why I also report about curl-config

@ras0219-msft ras0219-msft marked this pull request as ready for review April 28, 2021 22:24
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Apr 28, 2021

I found the issue -- I was trying to replace prefix=/absolute/path with prefix=$(get current script path), but messed up the replacement and just wrote back out $(get current script path).

With this, the PR is good to go on my side assuming it passes CI. @talregev I'd appreciate it if you wouldn't mind trying your previous code (using curl-config) on this new version, as some validation that the new curl-config works outside my machine.

@talregev
Copy link
Contributor

@ras0219 Good morning Robert. It is not my code, it is opensource code.
And sure. I am testing both curl-config and pkg-config with this PR.
Thank you.

@talregev
Copy link
Contributor

talregev commented Apr 29, 2021

@ras0219 Hi Robert, I open a two test branches as you can see. (I mention this PR on these branches).
All our tests are successful on both branches. For us the curl-config and pkg-config are good in this PR.
Thank you again for your time and this fix.

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please bump the port version. See documentation.
Also please run command vcpkg x-add-version --all then commit the changes.

@dg0yt
Copy link
Contributor

dg0yt commented May 1, 2021

@ras0219 Once again regarding curl-config: It is meant to be executed as a shell script. I would prefer to find this installed with executable bits set, and probably it belongs to /tools/ in this case. (I'm trying to use it for the gdal port on Unix. It gets harder when it is not executable. Gdal also wants geos-config which would be executable but is deleted...)

@talregev
Copy link
Contributor

talregev commented May 5, 2021

@ras0219 @ras0219-msft @JackBoosY any news?

@ras0219-msft
Copy link
Contributor

I hadn't incremented the version number, so it failed CI. If this passes, it's good to merge.

@dg0yt
Copy link
Contributor

dg0yt commented May 8, 2021

Can you have another look at the curl pc files? On x64-linux, with the curl from master I see a duplication of 'Libs'.

...
Libs: -L"${libdir}" -lcurl-d  -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz  -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz

In the buildtree, it looks a little bit different:

Libs: -L${libdir} -lcurl-d  -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz
Libs.private:  -lgcc -lgcc_s -lc -lgcc -lgcc_s -ldl -lpthread -lssl -lcrypto -lz

@Neumann-A
Copy link
Contributor

@dg0yt vcpkg_fixup_pkgconfig merges Libs.private into Libs if VCPKG_LIBRARY_LINKAGE is static

@dg0yt
Copy link
Contributor

dg0yt commented May 8, 2021

@dg0yt vcpkg_fixup_pkgconfig merges Libs.private into Libs if VCPKG_LIBRARY_LINKAGE is static

@Neumann-A Yes, garbage-in-garbage-out here. I want to ask if there could be an improvement on the input side (port curl).

@ras0219-msft
Copy link
Contributor

I'm going to merge this PR as-is for now and look at improving the .pc in a followup. Thanks everyone!

@ras0219-msft ras0219-msft merged commit e6dcc07 into microsoft:master May 13, 2021
@dg0yt
Copy link
Contributor

dg0yt commented May 13, 2021

👍
@ras0219-msft I'm fighting with looking at various pc files (towards fixing gdal for Unix). Cf. #17748 for a general approach to the duplication issue.

@ras0219-msft ras0219-msft deleted the dev/roschuma/curl-android branch May 20, 2021 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants