Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-43218: [C++] Resolve Abseil like any other dependency in the build system #43219

Merged
merged 6 commits into from
Jul 12, 2024

Conversation

felipecrv
Copy link
Contributor

@felipecrv felipecrv commented Jul 11, 2024

Rationale for this change

The workarounds around Abseil resolution don't seem necessary anymore and they don't work on all possible configurations of the build.

What changes are included in this PR?

Removal of the ensure_absl macro and adding a call to resolve_dependency when depending on the Google Cloud SDK (a GCS filesystem dependency) or gRPC (a flight dependency).

Are these changes tested?

Yes, by me trying different build configurations on my macOS and existing builds in CI.

@felipecrv felipecrv requested a review from kou July 11, 2024 01:39
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 11, 2024
cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved
cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved
cpp/cmake_modules/ThirdpartyToolchain.cmake Show resolved Hide resolved
cpp/cmake_modules/ThirdpartyToolchain.cmake Show resolved Hide resolved
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 11, 2024
And don't pass a pkg-config parameter for absl as there isn't a single package but many.

Co-authored-by: Sutou Kouhei <[email protected]>
@assignUser
Copy link
Member

@github-actions crossbow submit -g cpp

Copy link

Revision: c43af58

Submitted crossbow builds: ursacomputing/crossbow @ actions-402e522d16

Task Status
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove absl_SOURCE=BUNDLED from Fedora 39's Dockerfile?

diff --git a/ci/docker/fedora-39-cpp.dockerfile b/ci/docker/fedora-39-cpp.dockerfile
index 8ecaa6c3ca..33d1182309 100644
--- a/ci/docker/fedora-39-cpp.dockerfile
+++ b/ci/docker/fedora-39-cpp.dockerfile
@@ -77,8 +77,7 @@ RUN /arrow/ci/scripts/install_sccache.sh unknown-linux-musl /usr/local/bin
 
 # PYARROW_TEST_GANDIVA=OFF: GH-39695: We need to make LLVM symbols visible in
 # Python process explicitly if we use LLVM 17 or later.
-ENV absl_SOURCE=BUNDLED \
-    ARROW_ACERO=ON \
+ENV ARROW_ACERO=ON \
     ARROW_AZURE=OFF \
     ARROW_BUILD_TESTS=ON \
     ARROW_DEPENDENCY_SOURCE=SYSTEM \

We may also need this to avoid an unexpected combination:

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index a973c47848..1d382c967f 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -4154,6 +4154,11 @@ if(ARROW_WITH_GRPC)
   endif()
 
   set(ARROW_GRPC_REQUIRED_VERSION "1.30.0")
+  if(absl_SOURCE STREQUAL "BUNDLED" AND NOT gRPC_SOURCE STREQUAL "BUNDLED")
+    # System gRPC can't be used with bundled Abseil
+    message(STATUS "Forcing gRPC_SOURCE to BUNDLED because absl_SOURCE is BUNDLED")
+    set(gRPC_SOURCE "BUNDLED")
+  endif()
   if(NOT Protobuf_SOURCE STREQUAL gRPC_SOURCE)
     # ARROW-15495: Protobuf/gRPC must come from the same source
     message(STATUS "Forcing gRPC_SOURCE to Protobuf_SOURCE (${Protobuf_SOURCE})")

cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 11, 2024
@felipecrv felipecrv requested a review from kou July 11, 2024 21:56
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 11, 2024
@kou
Copy link
Member

kou commented Jul 12, 2024

@github-actions crossbow submit -g cpp

Copy link

Revision: 164898f

Submitted crossbow builds: ursacomputing/crossbow @ actions-0dab8bdfd4

Task Status
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou kou changed the title GH-43218: [C++] Resolve abseil like any other dependency in the build system GH-43218: [C++] Resolve Abseil like any other dependency in the build system Jul 12, 2024
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 12, 2024
@kou kou merged commit e65c1e2 into apache:main Jul 12, 2024
54 checks passed
@kou kou removed the awaiting merge Awaiting merge label Jul 12, 2024
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit e65c1e2.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants