GH-50194: [C++] Move S3 and AWS-SDK to its own libarrow_s3.so - #50195
GH-50194: [C++] Move S3 and AWS-SDK to its own libarrow_s3.so#50195raulcd wants to merge 10 commits into
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
74a0d20 to
07fa9cc
Compare
|
@pitrou @kou I've been working on splitting the S3 library (and the AWS SDK) outside $ ls -lhL libarrow.so libarrow_s3.so
-rwxrwxr-x 1 raulcd raulcd 59M Jun 22 19:30 libarrow_s3.so
-rwxrwxr-x 1 raulcd raulcd 317M Jun 22 19:29 libarrow.soAnd we can see AWS symbols aren't present on libarrow.so $ nm -C libarrow.so | grep -c "Aws::"
0
$ nm -C libarrow_s3.so | grep -c "Aws::"
33991With current main libarrow.so size and it contains AWS SDK symbols: $ ls -lhL libarrow.so
-rwxrwxr-x 1 raulcd raulcd 368M Jun 22 19:45 libarrow.so
$ ls -lhL libarrow_s3.so
ls: cannot access 'libarrow_s3.so': No such file or directory
$ nm -C libarrow.so | grep -c "Aws::"
33991Those are debug builds but as a summary: |
|
I think that we should use I think that bindings can provide convenient API to use the S3 module even if we use |
With conda this isn't necessary, we already ship all the With wheels this is another different beast and I have to explore a little further. A related issue: The original problem we had with wheels is that there's no mechanism to share dependencies between wheels. Auditwheel/delvewheel/delocate mangle the .so name to avoid other wheels clashing with other dependencies symbols. The problem is that As a note, I've just validated we don't mangle libarrow (or any of our .so) on the wheels. I am going to start exploring this a little further to see if I can come up with something even though I am still unclear about some of the questions above, like version matching to avoid ABI problems. Related: @amol- who worked on And some Python PEP attempts to define some external dependencies for wheels are on discussion: What I am saying is that using cc @h-vetinari who knows this space and might shed some light |
|
For the PyPI side, you might be able to do something similar to what numpy/scipy are doing with openblas as a wheel. |
| target_link_libraries(arrow_s3fs PRIVATE ${AWSSDK_LINK_LIBRARIES} arrow_shared) | ||
| set_source_files_properties(filesystem/s3fs.cc filesystem/s3fs_module.cc | ||
| PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON) | ||
| if(ARROW_BUILD_STATIC AND WIN32) |
There was a problem hiding this comment.
The AND WIN32 isn't useful, right?
There was a problem hiding this comment.
We use the same pattern on other places:
if(ARROW_BUILD_STATIC AND WIN32)
target_compile_definitions(arrow_compute_static PUBLIC ARROW_COMPUTE_STATIC)
endif()
or
if(ARROW_BUILD_STATIC AND WIN32)
target_compile_definitions(arrow_static PUBLIC ARROW_STATIC)
endif()
Taking a look at the definition on visibility.h of ARROW_S3_STATIC is already guarded for WIN32:
#if defined(_WIN32) || defined(__CYGWIN__)
So it will only be used on WIN32, it does not seem necessary on others so I would say the AND WIN32 does nothing but it's hygiene?
|
So, this is as if |
Yes but with a small caveat. |
|
Oh, great, thank you! |
|
Revision: 55a438d Submitted crossbow builds: ursacomputing/crossbow @ actions-db96d2f799
|
There was a problem hiding this comment.
🔵 Needs a closer look
It changes cross-platform C++ linkage/visibility and multiple packaging/binding integration points, so a human should validate downstream build/runtime behavior across supported platforms.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
uf, grande changes, thanks @raulcd! at line 784 -- Providing CMake module for FindAWSSDKAlt as part of Arrow CMake package-> A) It looks like AWS dependency remains with core Arrow instead of "ArrowS3 CMake package" during configure there? at 2454 -- Installing: /opt/conda/envs/arrow/lib/cmake/Arrow/FindAWSSDKAlt.cmake-> B) Here at 2472 -- Installing: /opt/conda/envs/arrow/lib/cmake/ArrowS3/ArrowS3Config.cmake
-- Installing: /opt/conda/envs/arrow/lib/cmake/ArrowS3/ArrowS3ConfigVersion.cmake
-- Installing: /opt/conda/envs/arrow/lib/cmake/ArrowS3/ArrowS3Targets.cmakeC) One more issue - installed grep -c INTERFACE_LINK_LIBRARIES /opt/conda/envs/arrow/lib/cmake/ArrowS3/ArrowS3Targets.cmake
|
…row and static build
|
@github-actions crossbow submit test-conda-cpp |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core C++ build/link/export semantics and downstream packaging/bindings across multiple ecosystems, so it needs a final human validation pass even after addressing the Meson issue.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
|
Revision: 142e154 Submitted crossbow builds: ursacomputing/crossbow @ actions-0ed0055ecc
|
Maybe we could try to expand the following to cover static builds and other Arrow modules? But that's probably better done as a standalone issue / PR: Lines 120 to 156 in a91ae52 |
|
Trying to test this it seems we might be missing Azure SDK libraries as a dependency for |
O, that's a much better location indeed! If it's not too late, here's a draft PR on cpp/examples/minimal_build/example.cc I'd try adding Azure:: fix there too. |
There was a problem hiding this comment.
Should this remaining ARROW_S3 section get an update now? I've tried it locally on top of PR changes, seems OK;
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
@@ -111,22 +111,6 @@ if(ARROW_USE_GLOG)
endif()
endif()
-if(ARROW_S3)
- if(AWSSDK_SOURCE STREQUAL "SYSTEM")
- list(APPEND
- ARROW_STATIC_INSTALL_INTERFACE_LIBS
- aws-cpp-sdk-identity-management
- aws-cpp-sdk-sts
- aws-cpp-sdk-cognito-identity
- aws-cpp-sdk-s3
- aws-cpp-sdk-core)
- elseif(AWSSDK_SOURCE STREQUAL "BUNDLED")
- if(UNIX)
- list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS CURL::libcurl)
- endif()
- endif()
-endif()
if(ARROW_WITH_OPENTELEMETRY)
@@ -1070,6 +1054,10 @@ if(ARROW_FILESYSTEM)
if(AWSSDK_SOURCE STREQUAL "SYSTEM")
list(APPEND ARROW_S3_STATIC_INSTALL_INTERFACE_LIBS ${AWSSDK_LINK_LIBRARIES})
+ elseif(AWSSDK_SOURCE STREQUAL "BUNDLED")
+ if(UNIX)
+ list(APPEND ARROW_S3_STATIC_INSTALL_INTERFACE_LIBS CURL::libcurl)
+ endif()
endif()
add_arrow_lib(arrow_s3There was a problem hiding this comment.
🔵 Needs a closer look
It changes core C++ library partitioning and downstream packaging/linkage across multiple build systems and languages, which warrants final human validation for ABI/packaging compatibility.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
(Sorry for CI re-run and void comments, I added my commits from #51280 onto branch |
Rationale for this change
Trying to reduce the size of
libarrow.soand remove AWS SDK on some builds. Allow for users to plug and play based on requirements and divide our functionality into cleaner modules.What changes are included in this PR?
Unconditionally build S3 and the AWS SDK into a different module
libarrow_s3.sooutside oflibarrow.so.Update bindings to link against the new
libarrow_s3.solibrary.Update the Linux Package jobs to have the new module into a different package.
Are these changes tested?
Yes via CI
Are there any user-facing changes?
Yes, users will need to either link against
libarrow_s3.soor register usingLoadFileSystemFactories