You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/02/13 21:23:00 UTC
[GitHub] [arrow] kou commented on a diff in pull request #34159: GH-34157: [C++] Configure bundled AWS SDK to use aws-lc instead of OpenSSL
kou commented on code in PR #34159:
URL: https://github.com/apache/arrow/pull/34159#discussion_r1105007159
##########
ci/scripts/java_jni_macos_build.sh:
##########
@@ -135,6 +136,7 @@ archery linking check-dependencies \
--allow libncurses \
--allow libobjc \
--allow libplasma_java \
+ --allow libprotobuf \
Review Comment:
We should not allow `libprotobuf` because `libprotobuf` isn't a system library.
Which library requires `libprotobuf`?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4779,9 +4805,22 @@ macro(build_awssdk)
aws-c-event-stream
aws-c-io
aws-c-cal
- s2n-tls
aws-checksums
aws-c-common)
+
+ # aws-lc needs to be installed on a separate folder to hide from unintended use
+ set(AWS_LC_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/aws_lc_ep-install")
+ set(AWS_LC_INCLUDE_DIR "${AWS_LC_PREFIX}/include")
+
+ if(UNIX AND NOT APPLE) # aws-lc and s2n-tls only needed on linux
+ file(MAKE_DIRECTORY ${AWS_LC_INCLUDE_DIR})
+ list(APPEND
+ _AWSSDK_LIBS
+ s2n-tls
+ crypto
+ ssl)
Review Comment:
Can we use `aws-lc` instead of `crypto` and `ssl`?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4727,20 +4735,38 @@ macro(build_awssdk)
set(AWSSDK_BUILD_TYPE release)
endif()
+ # provide hint for AWS SDK to link with the already located openssl
+ get_filename_component(OPENSSL_ROOT_HINT "${OPENSSL_INCLUDE_DIR}" DIRECTORY)
+ if(APPLE AND NOT OPENSSL_ROOT_HINT)
+ find_program(BREW brew)
+ if(BREW)
+ execute_process(COMMAND ${BREW} --prefix "openssl@1.1"
+ OUTPUT_VARIABLE OPENSSL11_BREW_PREFIX
+ OUTPUT_STRIP_TRAILING_WHITESPACE)
+ if(OPENSSL11_BREW_PREFIX)
+ set(OPENSSL_ROOT_HINT ${OPENSSL11_BREW_PREFIX})
+ else()
+ execute_process(COMMAND ${BREW} --prefix "openssl"
+ OUTPUT_VARIABLE OPENSSL_BREW_PREFIX
+ OUTPUT_STRIP_TRAILING_WHITESPACE)
+ if(OPENSSL_BREW_PREFIX)
+ set(OPENSSL_ROOT_HINT ${OPENSSL_BREW_PREFIX})
+ endif()
+ endif()
+ endif()
+ endif()
Review Comment:
Do we need this?
`OPENSSL_INCLUDE_DIR` must be set by `find_package(OpenSSL)`.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4827,37 +4881,41 @@ macro(build_awssdk)
DEPENDS aws_c_common_ep)
add_dependencies(AWS::aws-checksums aws_checksums_ep)
- set(S2N_TLS_CMAKE_ARGS ${AWSSDK_COMMON_CMAKE_ARGS})
- if(APPLE AND NOT OPENSSL_ROOT_DIR)
- find_program(BREW brew)
- if(BREW)
- execute_process(COMMAND ${BREW} --prefix "openssl@1.1"
- OUTPUT_VARIABLE OPENSSL11_BREW_PREFIX
- OUTPUT_STRIP_TRAILING_WHITESPACE)
- if(OPENSSL11_BREW_PREFIX)
- set(OPENSSL_ROOT_DIR ${OPENSSL11_BREW_PREFIX})
- else()
- execute_process(COMMAND ${BREW} --prefix "openssl"
- OUTPUT_VARIABLE OPENSSL_BREW_PREFIX
- OUTPUT_STRIP_TRAILING_WHITESPACE)
- if(OPENSSL_BREW_PREFIX)
- set(OPENSSL_ROOT_DIR ${OPENSSL_BREW_PREFIX})
- endif()
- endif()
- endif()
- endif()
- if(OPENSSL_ROOT_DIR)
- # For Findcrypto.cmake in s2n-tls.
- list(APPEND S2N_TLS_CMAKE_ARGS -DCMAKE_PREFIX_PATH=${OPENSSL_ROOT_DIR})
+ if(UNIX AND NOT APPLE) # aws-lc and s2n-tls only needed on linux
+ set(AWS_LC_C_FLAGS ${EP_C_FLAGS})
+ list(APPEND AWS_LC_C_FLAGS "-Wno-error=overlength-strings")
+
+ set(AWS_LC_CMAKE_ARGS ${AWSSDK_COMMON_CMAKE_ARGS})
+ list(APPEND AWS_LC_CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${AWS_LC_PREFIX}
+ -DCMAKE_C_FLAGS=${AWS_LC_C_FLAGS})
+ externalproject_add(aws_lc_ep
+ ${EP_COMMON_OPTIONS}
+ URL ${AWS_LC_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_AWS_LC_BUILD_SHA256_CHECKSUM}"
+ CMAKE_ARGS ${AWS_LC_CMAKE_ARGS}
+ BUILD_BYPRODUCTS ${SSL_STATIC_LIBRARY} ${CRYPTO_STATIC_LIBRARY})
+ add_dependencies(AWS::crypto aws_lc_ep)
+ add_dependencies(AWS::ssl aws_lc_ep)
+
+ set(S2N_TLS_CMAKE_ARGS ${AWSSDK_COMMON_CMAKE_ARGS})
+ list(APPEND
+ S2N_TLS_CMAKE_ARGS
+ -DS2N_INTERN_LIBCRYPTO=ON # internalize libcrypto to avoid name conflict with openssl
+ -DCMAKE_PREFIX_PATH=${AWS_LC_PREFIX}) # path to find crypto provided by aws-lc
+
+ externalproject_add(s2n_tls_ep
+ ${EP_COMMON_OPTIONS}
+ URL ${S2N_TLS_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_S2N_TLS_BUILD_SHA256_CHECKSUM}"
+ CMAKE_ARGS ${S2N_TLS_CMAKE_ARGS}
+ BUILD_BYPRODUCTS ${S2N_TLS_STATIC_LIBRARY}
+ DEPENDS aws_lc_ep)
+ add_dependencies(AWS::s2n-tls s2n_tls_ep)
endif()
- externalproject_add(s2n_tls_ep
- ${EP_COMMON_OPTIONS}
- URL ${S2N_TLS_SOURCE_URL}
- URL_HASH "SHA256=${ARROW_S2N_TLS_BUILD_SHA256_CHECKSUM}"
- CMAKE_ARGS ${S2N_TLS_CMAKE_ARGS}
- BUILD_BYPRODUCTS ${S2N_TLS_STATIC_LIBRARY})
- add_dependencies(AWS::s2n-tls s2n_tls_ep)
+ set(AWS_C_CAL_CMAKE_ARGS ${AWSSDK_COMMON_CMAKE_ARGS})
+ # Because aws-c-cal doesn't provide ability to hide libcrypto, we have to always use openssl
+ list(APPEND AWS_C_CAL_CMAKE_ARGS -DUSE_OPENSSL=ON)
Review Comment:
Could you use this in `externalproject_add(aws_c_cal_ep)`?
Or is it needless?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4985,6 +5047,14 @@ macro(build_awssdk)
set_property(TARGET CURL::libcurl
APPEND
PROPERTY INTERFACE_LINK_LIBRARIES OpenSSL::SSL)
Review Comment:
This is not related to this pull rquest but we should do this in `find_curl()`.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4796,6 +4835,11 @@ macro(build_awssdk)
"${AWSSDK_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}s2n${CMAKE_STATIC_LIBRARY_SUFFIX}"
)
endif()
+ if(${_AWSSDK_LIB} STREQUAL "crypto" OR ${_AWSSDK_LIB} STREQUAL "ssl")
Review Comment:
```suggestion
elseif(${_AWSSDK_LIB} STREQUAL "crypto" OR ${_AWSSDK_LIB} STREQUAL "ssl")
```
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4827,37 +4881,41 @@ macro(build_awssdk)
DEPENDS aws_c_common_ep)
add_dependencies(AWS::aws-checksums aws_checksums_ep)
- set(S2N_TLS_CMAKE_ARGS ${AWSSDK_COMMON_CMAKE_ARGS})
- if(APPLE AND NOT OPENSSL_ROOT_DIR)
- find_program(BREW brew)
- if(BREW)
- execute_process(COMMAND ${BREW} --prefix "openssl@1.1"
- OUTPUT_VARIABLE OPENSSL11_BREW_PREFIX
- OUTPUT_STRIP_TRAILING_WHITESPACE)
- if(OPENSSL11_BREW_PREFIX)
- set(OPENSSL_ROOT_DIR ${OPENSSL11_BREW_PREFIX})
- else()
- execute_process(COMMAND ${BREW} --prefix "openssl"
- OUTPUT_VARIABLE OPENSSL_BREW_PREFIX
- OUTPUT_STRIP_TRAILING_WHITESPACE)
- if(OPENSSL_BREW_PREFIX)
- set(OPENSSL_ROOT_DIR ${OPENSSL_BREW_PREFIX})
- endif()
- endif()
- endif()
- endif()
- if(OPENSSL_ROOT_DIR)
- # For Findcrypto.cmake in s2n-tls.
- list(APPEND S2N_TLS_CMAKE_ARGS -DCMAKE_PREFIX_PATH=${OPENSSL_ROOT_DIR})
+ if(UNIX AND NOT APPLE) # aws-lc and s2n-tls only needed on linux
Review Comment:
```suggestion
if("s2n-tls" IN_LIST _AWSSDK_LIBS)
```
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4867,13 +4925,17 @@ macro(build_awssdk)
DEPENDS aws_c_common_ep)
add_dependencies(AWS::aws-c-cal aws_c_cal_ep)
+ set(AWS_C_IO_DEPENDS aws_c_common_ep aws_c_cal_ep)
+ if(UNIX AND NOT APPLE)
Review Comment:
```suggestion
if(TARGET s2n_tls_ep)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org