Skip to content

Conversation

@nealrichardson
Copy link
Member

I've tried enabling this in the R linux builds and have made some progress, but I'm hitting some issues that someone more experienced with cmake might be able to help with.

Solving this will also let us enable S3 in more Python wheels; cf. #7997

@github-actions
Copy link

@kou
Copy link
Member

kou commented Sep 22, 2020

For the aws-sdk headers, the following patch will fix them:

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 0180ef912..033e18491 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2629,10 +2629,19 @@ macro(build_awssdk)
     set(AWSSDK_BUILD_TYPE Release)
   endif()
 
+  set(AWSSDK_LIST_SEPARATOR "|")
+  string(JOIN
+         ${AWSSDK_LIST_SEPARATOR}
+         AWSSDK_BUILD_ONLY
+         s3
+         core
+         config
+         identity-management
+         sts)
   set(AWSSDK_CMAKE_ARGS
       -DCMAKE_BUILD_TYPE=Release
       -DCMAKE_INSTALL_LIBDIR=lib
-      -DBUILD_ONLY=s3;core;config;identity-management;sts
+      -DBUILD_ONLY=${AWSSDK_BUILD_ONLY}
       -DENABLE_UNITY_BUILD=on
       -DENABLE_TESTING=off
       "-DCMAKE_C_FLAGS=${EP_C_FLAGS}"
@@ -2661,7 +2670,8 @@ macro(build_awssdk)
                       ${EP_LOG_OPTIONS}
                       URL ${AWSSDK_SOURCE_URL}
                       CMAKE_ARGS ${AWSSDK_CMAKE_ARGS}
-                      BUILD_BYPRODUCTS ${AWSSDK_SHARED_LIBS})
+                      BUILD_BYPRODUCTS ${AWSSDK_SHARED_LIBS}
+                      LIST_SEPARATOR ${AWSSDK_LIST_SEPARATOR})
 
   file(MAKE_DIRECTORY ${AWSSDK_INCLUDE_DIR})

We need to escape ; in BUILD_ONLY.

@kou
Copy link
Member

kou commented Sep 23, 2020

It's not a strong opinion but I'm negative we bundle OpenSSL.
OpenSSL is a security related product. I'm not sure we can release a new version ASAP when bundled OpenSSL has a vulnerability.

@kou
Copy link
Member

kou commented Sep 23, 2020

@nealrichardson
Copy link
Member Author

Ok, with that SEMICOLON change, the Ubuntu R job successfully compiles the C++ library, but the R package can't use it because it created a shared library for aws-sdk-cpp (or several), and those are stuck inside the build/awssdk_ep directory: https://github.com/apache/arrow/pull/8243/checks?check_run_id=1152416386#step:9:61

I think this means the bundled ep needs to build aws-sdk-cpp static, like it does for e.g. jemalloc--right?

@kou
Copy link
Member

kou commented Sep 23, 2020

Right.
It seems that we need to specify -DBUILD_SHARED_LIBS=off.

@pitrou
Copy link
Member

pitrou commented Sep 23, 2020

OpenSSL is a security related product. I'm not sure we can release a new version ASAP when bundled OpenSSL has a vulnerability.

I agree, but I think we already do that with Flight...

@kou
Copy link
Member

kou commented Sep 23, 2020

Ah, wheel...

@nealrichardson
Copy link
Member Author

Closing in favor of #8304

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.

3 participants