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 9 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! |
There was a problem hiding this comment.
🟡 Changes recommended
There are build-configuration issues in updated consumers (notably Windows R and PyArrow static/shared target selection) that can break linking in supported configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR modularizes Arrow C++ S3 support by moving the S3 filesystem (and its AWS SDK dependency surface) out of libarrow.so into a dedicated libarrow_s3.so (ArrowS3 CMake package / arrow-s3 pkg-config), and updates downstream consumers (R, Python, C GLib, Debian packaging, CI) accordingly.
Changes:
- Introduce a standalone Arrow S3 library (
arrow_s3_{shared,static}) with its own CMake/pkg-config discovery and symbol visibility. - Update bindings/build integrations (PyArrow, R) and GLib Meson to link/discover the new S3 library.
- Update tests and Linux packaging to account for the new
libarrow_s3artifacts and option surface (removeARROW_S3_MODULE).
File summaries
| File | Description |
|---|---|
| r/configure.win | Link Windows R bindings against arrow_s3 and expose S3-related flags/pkg-config names. |
| r/configure | Add arrow-s3 to feature-driven pkg-config names and fallback link flags. |
| python/CMakeLists.txt | Discover ArrowS3 and link the _s3fs extension against the new S3 library; bundle the S3 shared lib when packaging. |
| dev/tasks/linux-packages/apache-arrow/debian/libarrow-s3-dev.install | New Debian -dev install manifest for Arrow S3 CMake config, pkg-config, and libs. |
| dev/tasks/linux-packages/apache-arrow/debian/libarrow-s3-2600.install | New Debian runtime install manifest for libarrow_s3.so.*. |
| dev/tasks/linux-packages/apache-arrow/debian/control.in | Add Debian package stanzas for libarrow-s3-2600 and libarrow-s3-dev. |
| cpp/src/arrow/filesystem/util_internal.h | Export additional filesystem internals needed across shared-library boundaries. |
| cpp/src/arrow/filesystem/s3fs.h | Switch S3 public API exports from ARROW_EXPORT to ARROW_S3_EXPORT. |
| cpp/src/arrow/filesystem/s3fs_module_test.cc | Adjust module-loading test setup in line with new S3 library/module behavior. |
| cpp/src/arrow/filesystem/s3_visibility.h | New visibility header defining ARROW_S3_EXPORT/ARROW_S3_STATIC semantics. |
| cpp/src/arrow/filesystem/filesystem.h | Export EnsureFinalized() as part of the filesystem API surface. |
| cpp/src/arrow/filesystem/CMakeLists.txt | Update S3 test/benchmark link behavior to use arrow_s3_{shared,static}. |
| cpp/src/arrow/CMakeLists.txt | Build/install the new arrow_s3 library and export it as ArrowS3/arrow-s3. |
| cpp/src/arrow/ArrowS3Config.cmake.in | New CMake package config for ArrowS3 targets and compatibility glue. |
| cpp/src/arrow/arrow-s3.pc.in | New arrow-s3.pc template for pkg-config consumers. |
| cpp/cmake_modules/DefineOptions.cmake | Remove ARROW_S3_MODULE and update S3-related option definitions/docs. |
| ci/scripts/cpp_build.sh | Drop ARROW_S3_MODULE plumbing from CI C++ build script. |
| ci/docker/conda-cpp.dockerfile | Drop ARROW_S3_MODULE env var from conda-cpp image build. |
| c_glib/meson.build | Add optional discovery of arrow_s3 via pkg-config/CMake or direct library lookup. |
| c_glib/arrow-glib/meson.build | Conditionally link arrow-glib against ArrowS3 when found. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces arrow_s3 as a new exported component, but there are build/packaging correctness gaps (notably header installation for Meson consumers and static-link dependency exposure via pkg-config) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
cpp/src/arrow/filesystem/s3fs.h:27
s3fs.hnow includesarrow/filesystem/s3_visibility.h, but the Meson header install list for the filesystem module doesn’t install this new header. That will result in installeds3fs.hhaving a missing include for Meson-based consumers.
Please add s3_visibility.h to the Meson install_headers() list (and any other explicit header-install lists, if applicable) so installed headers are self-contained.
cpp/src/arrow/filesystem/CMakeLists.txt:124
ARROW_S3_TEST_EXTRA_LINK_LIBSalready includes${AWSSDK_LINK_LIBRARIES}(line 80), but thes3fs_benchmarktarget adds${AWSSDK_LINK_LIBRARIES}again. This is redundant and can make link lines noisier / harder to reason about.
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The Windows R build script prepends -larrow_s3 before the -L... search paths, which can break linking by preventing the linker from finding libarrow_s3.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently introduce and integrate the new arrow_s3 library across build systems and packaging, with only minor documentation/metadata nits to address.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
cpp/src/arrow/filesystem/CMakeLists.txt:76
- The comment above the S3 test link ordering still refers to explicitly linking arrow_shared/arrow_static, but the code now links arrow_s3_shared/arrow_s3_static. Updating the comment will prevent confusion about why arrow_s3_* is added here and what ordering is being enforced.
dev/tasks/linux-packages/apache-arrow/debian/control.in:260 - This -dev package description is inconsistent with the other module *-dev packages in this file (they describe providing C++ header files). Consider aligning the wording for consistency.
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@github-actions crossbow submit -g r |
|
Revision: 76310ae Submitted crossbow builds: ursacomputing/crossbow @ actions-76c0268f55 |
It makes sense. Let's consider it as a separated task.
I see. Anyway, the Ruby bindings (or the GLib bindings) will use |
Do you want me to open an issue and try to work on using |
|
@github-actions crossbow submit test-conda-cpp test-conda-cpp-valgrind |
It's helpful! But I can do it later. :-) So you can focus on the Python bindings for now. |
|
Revision: 76310ae Submitted crossbow builds: ursacomputing/crossbow @ actions-e85449efd0
|
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