kou commented on code in PR #33808:
URL: https://github.com/apache/arrow/pull/33808#discussion_r1083330380
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4728,8 +4729,9 @@ macro(build_awssdk)
"10")
# Workaround for https://github.com/aws/aws-sdk-cpp/issues/1750
set(AWSSDK_PATCH_COMMAND "sed" "-i.bak" "-e" "s/\"-Werror\"//g"
- "<SOURCE_DIR>/cmake/compiler_settings.cmake")
+ "<SOURCE_DIR>/cmake/compiler_settings.cmake" "&&")
endif()
+ list(APPEND AWSSDK_PATCH_COMMAND
"${AWSSDK_SOURCE}/prefetch_crt_dependency.sh")
Review Comment:
Can we use `<SOURCE_DIR>`?
```suggestion
list(APPEND AWSSDK_PATCH_COMMAND "<SOURCE_DIR>/prefetch_crt_dependency.sh")
```
See https://cmake.org/cmake/help/latest/module/ExternalProject.html for
`<SOURCE_DIR>`.
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4728,8 +4729,9 @@ macro(build_awssdk)
"10")
# Workaround for https://github.com/aws/aws-sdk-cpp/issues/1750
set(AWSSDK_PATCH_COMMAND "sed" "-i.bak" "-e" "s/\"-Werror\"//g"
- "<SOURCE_DIR>/cmake/compiler_settings.cmake")
+ "<SOURCE_DIR>/cmake/compiler_settings.cmake" "&&")
Review Comment:
Can we remove this patch?
It seems that 1.10.55 includes the fix for
https://github.com/aws/aws-sdk-cpp/issues/1750 .
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4728,8 +4729,9 @@ macro(build_awssdk)
"10")
# Workaround for https://github.com/aws/aws-sdk-cpp/issues/1750
set(AWSSDK_PATCH_COMMAND "sed" "-i.bak" "-e" "s/\"-Werror\"//g"
- "<SOURCE_DIR>/cmake/compiler_settings.cmake")
+ "<SOURCE_DIR>/cmake/compiler_settings.cmake" "&&")
endif()
+ list(APPEND AWSSDK_PATCH_COMMAND
"${AWSSDK_SOURCE}/prefetch_crt_dependency.sh")
Review Comment:
BTW, I think that we should not use this. If we use this, this code doesn't
work on Windows. (Windows doesn't provide `sh`.)
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4643,7 +4644,7 @@ macro(build_awssdk)
set(AWSSDK_CMAKE_ARGS
${AWSSDK_COMMON_CMAKE_ARGS}
-DOPENSSL_ROOT_DIR=${OPENSSL_ROOT_HINT}
- -DBUILD_DEPS=OFF
+ -DBUILD_DEPS=ON # We need to build aws-crt-cpp from source
Review Comment:
Can we build `aws-crt-cpp` by ourselves instead of using this and
`prefetch_crt_dependency.sh` like other dependencies such as `aws-c-common` for
portability?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
