ARROW-17051: [C++] Link Flight/gRPC/Protobuf consistently #13599
Conversation
|
|
lidavidm
force-pushed
the
arrow-17051
branch
3 times, most recently
from
bf327cc to
0f3b848
Compare
cpp/src/arrow/flight/CMakeLists.txt
Outdated
| if(NOT ARROW_BUILD_STATIC) | ||
| message(STATUS "If static Protobuf or gRPC are used, Arrow must be built statically") | ||
| message(STATUS "(These libraries have global state, and linkage must be consistent)") | ||
| message(FATAL_ERROR "Must build Arrow statically to link Flight tests statically") |
docker-compose.yml
Outdated
| ARROW_BUILD_STATIC: "OFF" | ||
| # ARROW-17051: this build uses static Protobuf, so we must also | ||
| # use static Arrow to run Flight/Flight SQL tests | ||
| ARROW_BUILD_STATIC: "ON" |
There was a problem hiding this comment.
How about moving this to ci/docker/ubuntru-*-cpp.dockerfile? If newer Ubuntu provides enough recent Protobuf as libprotobuf-dev, we can disable static built only for the newer Ubuntu.
There was a problem hiding this comment.
I moved it. 22.04 is also too old (3.12, we want 3.15): https://packages.ubuntu.com/jammy/libprotobuf-dev
docker-compose.yml
Outdated
| @@ -1214,6 +1216,8 @@ services: | |||
| shm_size: *shm-size | |||
| environment: | |||
| <<: *ccache | |||
| ARROW_BUILD_STATIC: 'ON' | |||
There was a problem hiding this comment.
Do we need this? We don't need to build static library when we don't build tests.
There was a problem hiding this comment.
Ah, right. Removed, also updated Flight's CMakeLists.txt to not perform the check added above if we're not building tests.
There was a problem hiding this comment.
Should we also change the travis jobs that fail?
-- If static Protobuf or gRPC are used, Arrow must be built statically
-- (These libraries have global state, and linkage must be consistent)
CMake Error at src/arrow/flight/CMakeLists.txt:43 (message):
Must build Arrow statically to link Flight tests statically
https://github.com/apache/arrow/blob/master/.travis.yml#L96 and https://github.com/apache/arrow/blob/master/.travis.yml#L148
Updated, thanks for catching that |
|
As described at https://issues.apache.org/jira/browse/ARROW-16919?focusedCommentId=17566864&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17566864 I think this could also fix ARROW-16919, which appears to be due to double-free of a static due to both statically and dynamically linking libarrow.so. |
|
@github-actions crossbow submit -g nightly |
|
Revision: 488b70c Submitted crossbow builds: ursacomputing/crossbow @ actions-e15d0e5aae
|
|
Ok, I think this is ready. The CI failures seem to be incidental or addressed in other issues. Thanks Raúl and Kou for all the help here. |
|
It seems that e.g.: https://app.travis-ci.com/github/apache/arrow/jobs/576693121#L2786 Could you try this patch because diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index b67f90e0bd..945ff7b6f8 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -862,7 +862,7 @@ add_dependencies(arrow_test_dependencies toolchain-tests)
if(ARROW_STATIC_LINK_LIBS)
add_dependencies(arrow_dependencies ${ARROW_STATIC_LINK_LIBS})
- if(ARROW_ORC)
+ if(ARROW_HDFS OR ARROW_ORC)
if(NOT MSVC_TOOLCHAIN)
list(APPEND ARROW_STATIC_LINK_LIBS ${CMAKE_DL_LIBS})
list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS ${CMAKE_DL_LIBS}) |
|
Done - thanks for catching that! |
|
Remotely related: substrait-io/substrait#249 |
.travis.yml
Outdated
| -e ARROW_BUILD_STATIC=OFF | ||
| -e ARROW_BUILD_STATIC=ON |
There was a problem hiding this comment.
Can we remove this because we have ARROW_BUILD_STATIC=OFF in Dockerfile?
.travis.yml
Outdated
| @@ -145,7 +145,7 @@ jobs: | |||
| # aws-sdk-cpp. | |||
| DOCKER_RUN_ARGS: >- | |||
| " | |||
| -e ARROW_BUILD_STATIC=OFF | |||
| -e ARROW_BUILD_STATIC=ON | |||
There was a problem hiding this comment.
Can we remove this because we have ARROW_BUILD_STATIC=OFF in Dockerfile?
docker-compose.yml
Outdated
| @@ -1214,6 +1213,7 @@ services: | |||
| shm_size: *shm-size | |||
| environment: | |||
| <<: *ccache | |||
| ARROW_BUILD_TESTS: 'OFF' | |||
There was a problem hiding this comment.
Is this needed?
ci/docker/linux-apt-r.dockerfile has ARROW_BUILD_TESTS=OFF.
|
Seems CI is still pending, but any other feedback here? (Thanks Kou for the comments so far.) |
|
Benchmark runs are scheduled for baseline = 87cefe8 and contender = bbf249e. bbf249e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
If Protobuf/gRPC are used statically, Flight must be as well, or else we can get odd runtime behavior due to the global state in those libraries when Flight SQL is involved (as Flight SQL would then bundle a second copy of Protobuf into its shared library). Authored-by: David Li <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
If Protobuf/gRPC are used statically, Flight must be as well, or else we can get odd runtime behavior due to the global state in those libraries when Flight SQL is involved (as Flight SQL would then bundle a second copy of Protobuf into its shared library).