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

add_android_openssl_libraries: Automatically link OpenSSL on argument… #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aambrosano
Copy link

… targets

@bog-dan-ro bog-dan-ro requested a review from Issam-b January 10, 2025 09:47
@aambrosano
Copy link
Author

For the record, OpenSSL::OpenSSL is not really the right target. FindOpenSSL will actually create OpenSSL::crypto and OpenSSL::ssl, however linking against those targets is still generating errors at the CMake generation step. Couldn't figure out why yet.

@@ -1,26 +1,30 @@
function(add_android_openssl_libraries)
if(CMAKE_BUILD_TYPE STREQUAL "Debug")
set(ssl_root_path ${CMAKE_CURRENT_FUNCTION_LIST_DIR}/no-asm)
set(SSL_ROOT_PATH ${CMAKE_CURRENT_FUNCTION_LIST_DIR}/no-asm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

keep local variable as lowercase

${SSL_ROOT_PATH}/ssl_1.1/${CMAKE_ANDROID_ARCH_ABI}/libssl_1_1.so)
set(OPENSSL_CRYPTO_LIBRARY ${SSL_ROOT_PATH}/ssl_1.1/${CMAKE_ANDROID_ARCH_ABI}/libcrypto_1_1.so)
set(OPENSSL_SSL_LIBRARY ${SSL_ROOT_PATH}/ssl_1.1/${CMAKE_ANDROID_ARCH_ABI}/libssl_1_1.so)
set(OPENSSL_INCLUDE_DIR ${SSL_ROOT_PATH}/ssl_1.1/include)
endif()

set_target_properties(${ARGN} PROPERTIES QT_ANDROID_EXTRA_LIBS
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can also be under the ${ARGN} loop

endif()

if(Qt6_VERSION VERSION_GREATER_EQUAL 6.5.0)
if(NOT OPENSSL_ROOT_DIR)
set(OPENSSL_ROOT_DIR ${SSL_ROOT_PATH}/ssl_3/${CMAKE_ANDROID_ARCH_ABI})
endif()
list(APPEND android_extra_libs
Copy link
Collaborator

Choose a reason for hiding this comment

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

we no longer need to assign this android_extra_libs list if we're doing it for OPENSSL_SSL_LIBRARY and OPENSSL_INCLUDE_DIR, we can use those latter two variables directly for QT_ANDROID_EXTRA_LIBS

else()
set(ssl_root_path ${CMAKE_CURRENT_FUNCTION_LIST_DIR})
set(SSL_ROOT_PATH ${CMAKE_CURRENT_FUNCTION_LIST_DIR})
endif()

if(Qt6_VERSION VERSION_GREATER_EQUAL 6.5.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit simpler re-write for this could be:
`
if(Qt6_VERSION VERSION_GREATER_EQUAL 6.5.0)
set(OPENSSL_CRYPTO_LIBRARY ${ssl_root_path}/ssl_3/${CMAKE_ANDROID_ARCH_ABI}/libcrypto_3.so)
set(OPENSSL_SSL_LIBRARY ${ssl_root_path}/ssl_3/${CMAKE_ANDROID_ARCH_ABI}/libssl_3.so)
set(OPENSSL_INCLUDE_DIR ${ssl_root_path}/ssl_3/include)
else()
set(OPENSSL_CRYPTO_LIBRARY ${ssl_root_path}/ssl_1.1/${CMAKE_ANDROID_ARCH_ABI}/libcrypto_1_1.so)
set(OPENSSL_SSL_LIBRARY ${ssl_root_path}/ssl_1.1/${CMAKE_ANDROID_ARCH_ABI}/libssl_1_1.so)
set(OPENSSL_INCLUDE_DIR ${ssl_root_path}/ssl_1.1/include)
endif()

find_package(OpenSSL REQUIRED)

foreach(TARGET ${ARGN})
if (TARGET ${TARGET})
set_target_properties(${TARGET} PROPERTIES APPEND
QT_ANDROID_EXTRA_LIBS ${OPENSSL_CRYPTO_LIBRARY} ${OPENSSL_SSL_LIBRARY})
target_link_libraries(${TARGET} PUBLIC OpenSSL::OpenSSL)
endif()
endforeach()
`

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually the set_target_properties() line can be changed to
set_property(TARGET ${TARGET} APPEND PROPERTY QT_ANDROID_EXTRA_LIBS ${OPENSSL_CRYPTO_LIBRARY} ${OPENSSL_SSL_LIBRARY})

Copy link
Collaborator

@Issam-b Issam-b left a comment

Choose a reason for hiding this comment

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

For the record, OpenSSL::OpenSSL is not really the right target. FindOpenSSL will actually create OpenSSL::crypto and OpenSSL::ssl, however linking against those targets is still generating errors at the CMake generation step. Couldn't figure out why yet.

This doesn't work for me, I get an error:

CMake Error at ~/Library/Android/sdk/android_openssl/android_openssl.cmake:24 (target_link_libraries):
Target "tst_openssl" links to:

OpenSSL::OpenSSL

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.

Call Stack (most recent call first):
CMakeLists.txt:17 (add_android_openssl_libraries)

@Issam-b
Copy link
Collaborator

Issam-b commented Feb 2, 2025

@aambrosano btw the commit message doesn't explain why such patch is needed, is it trying to fix some bug?

@Issam-b
Copy link
Collaborator

Issam-b commented Feb 2, 2025

@aambrosano on this

For the record, OpenSSL::OpenSSL is not really the right target. FindOpenSSL will actually create OpenSSL::crypto and OpenSSL::ssl, however linking against those targets is still generating errors at the CMake generation step. Couldn't figure out why yet.

It doesn't work because the targets should be OpenSSL::Crypto and OpenSSL::SSL not OpenSSL::crypto and OpenSSL::ssl.

@aambrosano
Copy link
Author

aambrosano commented Feb 19, 2025

@Issam-b it just made sense that we linked against OpenSSL in the targets we passed to add_android_openssl_libraries, which we weren't doing. Otherwise we'd need to look up for those libraries twice and that would be redundant. All other changes were just small adjustments.

@aambrosano btw the commit message doesn't explain why such patch is needed, is it trying to fix some bug?

@aambrosano
Copy link
Author

aambrosano commented Feb 19, 2025

@aambrosano on this

For the record, OpenSSL::OpenSSL is not really the right target. FindOpenSSL will actually create OpenSSL::crypto and OpenSSL::ssl, however linking against those targets is still generating errors at the CMake generation step. Couldn't figure out why yet.

It doesn't work because the targets should be OpenSSL::Crypto and OpenSSL::SSL not OpenSSL::crypto and OpenSSL::ssl.

I was just typing targets names off the top of my head when commenting, but was using the right names when coding. That will not work anyway because for some reason targets found within a function are not available outside of the scope of the function. Trying to link against OpenSSL::SSL and OpenSSL::Crypto will result in an error message like this one:

CMake Error in CMakeLists.txt:
  The dependency target "OpenSSL::SSL" of target
  "mytarget_autogen_timestamp_deps" does not exist.

Even when converting the function to a macro (which has its own caveats anyway) the listing will produce the same error. Will have to think about this one a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants