Uh oh!
There was an error while loading. Please reload this page.
GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds - #40578
Conversation
| string(APPEND ARROW_TESTING_PC_CFLAGS "${ARROW_TESTING_PC_CFLAGS_PRIVATE}") | ||
| set(ARROW_TESTING_PC_CFLAGS_PRIVATE "") | ||
| string(APPEND ARROW_PC_LIBS_PRIVATE " -l${CMAKE_DL_LIBS}") |
There was a problem hiding this comment.
If we put this to this block, this flag isn't used when -DARROW_BUILD_SHARED=ON -DARROW_BUILD_STATIC=ON is used. Does this work?
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index c5449d9956..4329d66821 100644
--- a/cpp/src/arrow/CMakeLists.txt+++ b/cpp/src/arrow/CMakeLists.txt@@ -140,9 +140,10 @@ if(ARROW_ENABLE_THREADING)
list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS Threads::Threads)
endif()
-if(NOT MSVC_TOOLCHAIN)+if(CMAKE_DL_LIBS)
list(APPEND ARROW_SHARED_INSTALL_INTERFACE_LIBS ${CMAKE_DL_LIBS})
list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS ${CMAKE_DL_LIBS})
+ string(APPEND ARROW_PC_LIBS_PRIVATE " -l${CMAKE_DL_LIBS}")
endif()
set(ARROW_TEST_LINK_TOOLCHAIN ${ARROW_GTEST_GMOCK} ${ARROW_GTEST_GTEST_MAIN})There was a problem hiding this comment.
That seems reasonable. Out of curiosity, what's the utility of building both shared and static and libraries?
There was a problem hiding this comment.
If we build Apache Arrow C++ for single propose (e.g. creating a PyArrow wheel), we don't need to build shared and static libraries. But if we want to use multiple proposes (e.g. using it from Python and R), we may need shared and static libraries.
Most packaging systems (deb, rpm and so on) provide both of shared and static libraries to support multiple proposes. If Apache Arrow C++ supports building both shared and static library, they don't need to build Apache Arrow C++ twice to build shared and static libraries.
There was a problem hiding this comment.
If Apache Arrow C++ supports building both shared and static library, they don't need to build Apache Arrow C++ twice
I think you'd only be able to reuse the object files, which should be doable by having adjacent build directories both using ccache. We can't even reuse object files on WIN32 because we add -DARROW_STATIC there and so the object files linked into arrow_static are different from those linked into arrow_shared.
There was a problem hiding this comment.
As a packager view, reusability of object files isn't so important. Composing shared/static libraries to one package family (libarrow15 and libarrow-dev for .deb) is important. Building twice for shared and static libraries isn't easy to compose...
For example:
- Both of
cmake --installfor shared library andcmake --installfor static library install not only library files but also header files, data files and so on. Most of header files will be same but some files may be different. In our case,arrow/util/config.hmay be different if we have some#cmakedefines that depend on library type. If there are different files, package for shared library and package for static library will be conflicted. - CMake packages for shared library and static library are different. We need to use different CMake package names (e.g.
ArrowSharedfor shared library andArrowStaticfor static library) to avoid conflicting them. - ...
GoogleTest is a product that doesn't support building both of shared and static libraries at once.
MSYS2 package of it building GoogleTest twice:
- https://github.com/msys2/MINGW-packages/blob/ad99b751d11d027345f6da4c91b4c33d5ad1ac7d/mingw-w64-gtest/PKGBUILD#L36-L60
- https://github.com/msys2/MINGW-packages/blob/ad99b751d11d027345f6da4c91b4c33d5ad1ac7d/mingw-w64-gtest/PKGBUILD#L64-L68
The package provides only CMake package for shared library: https://github.com/msys2/MINGW-packages/blob/ad99b751d11d027345f6da4c91b4c33d5ad1ac7d/mingw-w64-gtest/PKGBUILD#L70-L71
It means that users can't use static library version via its CMake package.
assignUser
left a comment
There was a problem hiding this comment.
We should really at some point move a way from the multiple list with dozens of targets or other lists in them ^_^ but that's of course a topic for some other time.
assignUser
commented
Mar 16, 2024
@github-actions crossbow submit example-cpp-minimal-build-static* |
Revision: 00c4b06 Submitted crossbow builds: ursacomputing/crossbow @ actions-fa21affe42
|
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 2224a29. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 67 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
When linking statically, pkg-config doesn't pick up the new dependency on libdl introduced by #39067
This produces unresolved symbol errors
What changes are included in this PR?
Addition of
-ldltoARROW_PC_LIBS_PRIVATEto ensure linkage to the necessary libraryAre these changes tested?
yes
Are there any user-facing changes?
no