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

[aws-sdk-cpp] [openssl] Fix iOS build #17338

Merged
merged 49 commits into from
Jun 7, 2021

Conversation

ahmedyarub
Copy link
Contributor

Don't pass the the compiler to OpenSSL when building for iOS. That would end generating a MacOS library which would cause linking errors when the libraries are used. The OpenSSL configuration scripts know how to detect the target OS and compile using the correct command, example: xcrun -sdk iphonesimulator cc. Note that this change should fix building for all iOS targets, including both simulators and actual devices.
Add try_compile() parameters required to test the features of curl when cross-compiling AWS SDK for C++.

  • What does your PR fix?

    Fixes building OpenSSL and AWS SDK for C++ for iOS.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    All iOS triplets

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

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

@JackBoosY JackBoosY added the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label Apr 19, 2021
versions/a-/aws-sdk-cpp.json Outdated Show resolved Hide resolved
versions/o-/openssl.json Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

I think you should wait for my PR #17341 merge.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Apr 19, 2021
@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Apr 20, 2021
versions/a-/aws-sdk-cpp.json Outdated Show resolved Hide resolved
versions/o-/openssl.json Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Apr 22, 2021
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Generally, this smells like we simply aren't correctly propagating flags to the build -- we're missing some flags from the big list on lines 50-77 or so.

This file comes from a time before we had https://github.com/microsoft/vcpkg/blob/master/scripts/cmake/vcpkg_internal_get_cmake_vars.cmake, which is the new way of approaching this problem. See https://github.com/microsoft/vcpkg/blob/master/scripts/get_cmake_vars/CMakeLists.txt for the "modern" version.

I'm not opposed to this stop-gap for now as long as it's better integrated with the existing behavior, but the long term goal will be to move to vcpkg_internal_get_cmake_vars().

ports/openssl/unix/CMakeLists.txt Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 24, 2021
@ahmedyarub
Copy link
Contributor Author

Thanks for the PR!

Generally, this smells like we simply aren't correctly propagating flags to the build -- we're missing some flags from the big list on lines 50-77 or so.

This file comes from a time before we had https://github.com/microsoft/vcpkg/blob/master/scripts/cmake/vcpkg_internal_get_cmake_vars.cmake, which is the new way of approaching this problem. See https://github.com/microsoft/vcpkg/blob/master/scripts/get_cmake_vars/CMakeLists.txt for the "modern" version.

I'm not opposed to this stop-gap for now as long as it's better integrated with the existing behavior, but the long term goal will be to move to vcpkg_internal_get_cmake_vars().

Appreciate the feedback @ras0219-msft . Please give me a couple of days to redo this PR and the Emscripten/WASM32. I'll make the suggested modernization and test on Windows, Ubuntu 20.04 (WSL), MacOS Big Sur, latest Android NDK, iOS and Emscripten SDK on WSL.

@JackBoosY JackBoosY marked this pull request as draft April 25, 2021 02:37
@strega-nil-ms
Copy link
Contributor

@ahmedyarub could you rebase onto latest?

…sdk_ios

� Conflicts:
�	ports/openssl/unix/CMakeLists.txt
�	versions/o-/openssl.json
@ahmedyarub
Copy link
Contributor Author

@ahmedyarub could you rebase onto latest?

Please hold on merging I need to make more tests since the rebase...

@ahmedyarub
Copy link
Contributor Author

OK tested both on iOS and Emscripten on Ubuntu. Fixed a couple of bugs and not it should be good to go.

@PhoebeHui PhoebeHui removed the info:reviewed Pull Request changes follow basic guidelines label Jun 7, 2021
@strega-nil-ms strega-nil-ms merged commit c1c253f into microsoft:master Jun 7, 2021
@strega-nil-ms
Copy link
Contributor

Thanks @ahmedyarub :)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants