You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "bkietz (via GitHub)" <gi...@apache.org> on 2024/03/15 15:02:40 UTC
[PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
bkietz opened a new pull request, #40578:
URL: https://github.com/apache/arrow/pull/40578
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
When linking statically, pkg-config doesn't pick up the new dependency on libdl introduced by https://github.com/apache/arrow/pull/39067
This produces [unresolved symbol errors](https://github.com/apache/arrow/pull/39067#issuecomment-1999218559)
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
Addition of `-ldl` to `ARROW_PC_LIBS_PRIVATE` to ensure linkage to the necessary library
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
no
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40578:
URL: https://github.com/apache/arrow/pull/40578#discussion_r1527117999
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -968,6 +968,8 @@ if(NOT ARROW_BUILD_SHARED AND ARROW_BUILD_STATIC)
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}")
Review 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 --install` for shared library and `cmake --install` for 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.h` may be different if we have some `#cmakedefine`s 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. `ArrowShared` for shared library and `ArrowStatic` for 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #40578:
URL: https://github.com/apache/arrow/pull/40578#discussion_r1526477662
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -968,6 +968,8 @@ if(NOT ARROW_BUILD_SHARED AND ARROW_BUILD_STATIC)
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}")
Review Comment:
That seems reasonable. Out of curiosity, what's the utility of building both shared and static and libraries?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #40578:
URL: https://github.com/apache/arrow/pull/40578#discussion_r1526636738
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -968,6 +968,8 @@ if(NOT ARROW_BUILD_SHARED AND ARROW_BUILD_STATIC)
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}")
Review 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`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40578:
URL: https://github.com/apache/arrow/pull/40578#discussion_r1526448651
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -968,6 +968,8 @@ if(NOT ARROW_BUILD_SHARED AND ARROW_BUILD_STATIC)
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}")
Review Comment:
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
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
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})
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40578:
URL: https://github.com/apache/arrow/pull/40578#issuecomment-1999857463
:warning: GitHub issue #40577 **has no components**, please add labels for components.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz merged PR #40578:
URL: https://github.com/apache/arrow/pull/40578
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40578:
URL: https://github.com/apache/arrow/pull/40578#discussion_r1526448651
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -968,6 +968,8 @@ if(NOT ARROW_BUILD_SHARED AND ARROW_BUILD_STATIC)
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}")
Review 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
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})
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40578:
URL: https://github.com/apache/arrow/pull/40578#issuecomment-2002335423
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 2224a299f09d6c0ca3bd271346dca306f07d2792.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/22749877290) has more details. It also includes information about 67 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40578:
URL: https://github.com/apache/arrow/pull/40578#issuecomment-2000918103
Revision: 00c4b063f05e5b0e1fc073f7eba230e248e2da88
Submitted crossbow builds: [ursacomputing/crossbow @ actions-fa21affe42](https://github.com/ursacomputing/crossbow/branches/all?query=actions-fa21affe42)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/8304195749/job/22729506800)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/8304195743/job/22729506809)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40578:
URL: https://github.com/apache/arrow/pull/40578#discussion_r1526508211
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -968,6 +968,8 @@ if(NOT ARROW_BUILD_SHARED AND ARROW_BUILD_STATIC)
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}")
Review 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40577: [C++] Ensure pkg-config flags include -ldl for static builds [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #40578:
URL: https://github.com/apache/arrow/pull/40578#issuecomment-2000909223
@github-actions crossbow submit example-cpp-minimal-build-static*
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org