GH-40855: [C++][ORC] Fix `std::filesystem` related link error with ORC 2.0.0 or later #41023

kou · 2024-04-05T08:58:26Z

Rationale for this change

If we use std::filesystem, we need -lstdc++fs with GCC 8 and -lc++fs for clang 7. We don't want to maintain CMake code for GCC 8/clang 7.

What changes are included in this PR?

Don't use std::filesystem with ORC 2.0.0 or later.
Use missing -lstdc++fs/-lc++fs with bundled ORC 2.0.0.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

GitHub Issue: [C++][ORC] Need -lstdc++fs with GCC 8 #40855

kou · 2024-04-05T08:58:48Z

@github-actions crossbow submit almalinux-8-amd64

github-actions · 2024-04-05T09:01:12Z

Revision: 7e2e612

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

Task	Status
almalinux-8-amd64

kou · 2024-04-05T12:33:22Z

My guess was wrong...
ORC 2.0.0 or later uses std::filesystem. So we need to specify -lstdc++fs or -lc++fs to use liborc.a.

wgtmac · 2024-04-05T12:34:15Z

cpp/cmake_modules/FindorcAlt.cmake

@@ -29,6 +29,7 @@ endif()
 find_package(orc ${find_package_args})
 if(orc_FOUND)
  set(orcAlt_FOUND TRUE)
+  set(orcAlt_VERSION ${orc_VERSION})


Does this work with conan？

I hope so...
Do you know conan's ORC recipe provide valid(?) CMake package for ORC?

(BTW, is there a plan that ORC provides its CMake package?)

Do you know conan's ORC recipe provide valid(?) CMake package for ORC?

I suppose it does. It was added by me: https://github.com/conan-io/conan-center-index/blob/master/recipes/orc/all/conanfile.py#L145

is there a plan that ORC provides its CMake package?

I'm not sure if I understand the question correctly, do you mean the ORC community provides an official FindOrc.cmake file or something similar?

Yes.
(orcConfig.cmake will be better than FindOrc.cmake.)

FYI: Apache Arrow provides ArrowConfig.cmake, ArrowFlightConfig.cmake, ParquetConfig.cmake and so on.

OK, let me investigate how does it work and add it to ORC.

wgtmac · 2024-04-05T12:35:41Z

My guess was wrong... ORC 2.0.0 or later uses std::filesystem. So we need to specify -lstdc++fs or -lc++fs to use liborc.a.

Is any other module doing the same thing, or ORC is the only one that requires std::filesystem?

…r later If we use `std::filesystem`, we need `-lstdc++fs` with GCC 8 and `-lc++fs` for clang 7. We don't want to maintain CMake code for GCC 8/clang 7. So this avoids using `std::filesystem` with ORC 2.0.0 or later.

kou · 2024-04-05T13:32:04Z

@github-actions crossbow submit almalinux-8-amd64

github-actions · 2024-04-05T13:34:28Z

Revision: 9987fb6

Submitted crossbow builds: ursacomputing/crossbow @ actions-297049266f

Task	Status
almalinux-8-amd64

kou · 2024-04-05T13:36:49Z

Is any other module doing the same thing, or ORC is the only one that requires std::filesystem?

The latter.

wgtmac · 2024-04-05T13:44:37Z

Is any other module doing the same thing, or ORC is the only one that requires std::filesystem?

The latter.

I can fix that on the ORC side by removing std::filesystem dependency in 2.0.1 if you think appropriate and revert ORC 2.0.0 to 1.9.3.

kou · 2024-04-05T13:50:45Z

I can fix that on the ORC side by removing std::filesystem dependency in 2.0.1 if you think appropriate and revert ORC 2.0.0 to 1.9.3.

It'll be helpful. Could you do it? But we don't need to revert bundled ORC to 1.9.3. Time zone DB path check in ORC 2.0.0 will be useful. And this PR will fix this std::filesystem problem.

wgtmac · 2024-04-05T13:53:41Z

I can fix that on the ORC side by removing std::filesystem dependency in 2.0.1 if you think appropriate and revert ORC 2.0.0 to 1.9.3.

It'll be helpful. Could you do it? But we don't need to revert bundled ORC to 1.9.3. Time zone DB path check in ORC 2.0.0 will be useful. And this PR will fix this std::filesystem problem.

OK, I will remove std::filesystem dependency but keep the TZDB check on the ORC side.

kou · 2024-04-05T14:22:53Z

Thanks!

kou · 2024-04-05T14:23:26Z

@github-actions crossbow submit -g cpp

github-actions · 2024-04-05T14:25:56Z

Revision: 9987fb6

Submitted crossbow builds: ursacomputing/crossbow @ actions-45d2d9e22b

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

github-actions · 2024-04-05T14:42:19Z

⚠️ GitHub issue #40855 has been automatically assigned in GitHub to PR creator.

conbench-apache-arrow · 2024-04-06T09:06:13Z

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

There were no benchmark performance regressions. 🎉

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

### What changes were proposed in this pull request? Remove std::filesystem and use OS API instead. ### Why are the changes needed? Apache Arrow C++ library is required to support old compilers like clang8. Since Apache ORC C++ library has used std::filesystem to check TZDB availability since 2.0.0, Apache Arrow requires to add more linking options for std::filesystem. See apache/arrow#41023 for detail. ### How was this patch tested? Passing CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #1886 from wgtmac/ORC-1686. Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

### What changes were proposed in this pull request? Remove std::filesystem and use OS API instead. ### Why are the changes needed? Apache Arrow C++ library is required to support old compilers like clang8. Since Apache ORC C++ library has used std::filesystem to check TZDB availability since 2.0.0, Apache Arrow requires to add more linking options for std::filesystem. See apache/arrow#41023 for detail. ### How was this patch tested? Passing CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #1886 from wgtmac/ORC-1686. Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 695e0f3) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ith ORC 2.0.0 or later (apache#41023) ### Rationale for this change If we use `std::filesystem`, we need `-lstdc++fs` with GCC 8 and `-lc++fs` for clang 7. We don't want to maintain CMake code for GCC 8/clang 7. ### What changes are included in this PR? * Don't use `std::filesystem` with ORC 2.0.0 or later. * Use missing `-lstdc++fs`/`-lc++fs` with bundled ORC 2.0.0. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#40855 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou requested a review from wgtmac as a code owner April 5, 2024 08:58

github-actions bot added the awaiting committer review Awaiting committer review label Apr 5, 2024

wgtmac reviewed Apr 5, 2024

View reviewed changes

kou force-pushed the cpp-orc-filesystem branch from 7e2e612 to 9987fb6 Compare April 5, 2024 13:27

github-actions bot added the Component: C++ label Apr 5, 2024

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 5, 2024

kou changed the title ~~GH-40855: [C++][ORC] Don't use std::filesystem with ORC 2.0.0 or later~~ GH-40855: [C++][ORC] Fix std:;filesystem related link error ith ORC 2.0.0 or later Apr 5, 2024

kou changed the title ~~GH-40855: [C++][ORC] Fix std:;filesystem related link error ith ORC 2.0.0 or later~~ GH-40855: [C++][ORC] Fix std::filesystem related link error with ORC 2.0.0 or later Apr 5, 2024

wgtmac approved these changes Apr 5, 2024

View reviewed changes

kou merged commit df7da77 into apache:main Apr 5, 2024
41 of 43 checks passed

kou deleted the cpp-orc-filesystem branch April 5, 2024 23:13

kou removed the awaiting changes Awaiting changes label Apr 5, 2024

kou mentioned this pull request Apr 5, 2024

[C++][ORC] Need -lstdc++fs with GCC 8 #40855

Closed

wgtmac mentioned this pull request Apr 10, 2024

ORC-1686: [C++] Avoid using std::filesystem apache/orc#1886

Closed

kou mentioned this pull request Jul 4, 2024

GH-42149: [C++] Use FetchContent for bundled ORC #43011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-40855: [C++][ORC] Fix `std::filesystem` related link error with ORC 2.0.0 or later #41023

GH-40855: [C++][ORC] Fix `std::filesystem` related link error with ORC 2.0.0 or later #41023

kou commented Apr 5, 2024 •

edited

Loading

kou commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

kou commented Apr 5, 2024

wgtmac Apr 5, 2024

kou Apr 5, 2024

wgtmac Apr 5, 2024

kou Apr 5, 2024

wgtmac Apr 5, 2024

wgtmac commented Apr 5, 2024

kou commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

kou commented Apr 5, 2024

wgtmac commented Apr 5, 2024

kou commented Apr 5, 2024

wgtmac commented Apr 5, 2024

kou commented Apr 5, 2024

kou commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

conbench-apache-arrow bot commented Apr 6, 2024

GH-40855: [C++][ORC] Fix std::filesystem related link error with ORC 2.0.0 or later #41023

GH-40855: [C++][ORC] Fix std::filesystem related link error with ORC 2.0.0 or later #41023

Conversation

kou commented Apr 5, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

kou commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

kou commented Apr 5, 2024

wgtmac Apr 5, 2024

Choose a reason for hiding this comment

kou Apr 5, 2024

Choose a reason for hiding this comment

wgtmac Apr 5, 2024

Choose a reason for hiding this comment

kou Apr 5, 2024

Choose a reason for hiding this comment

wgtmac Apr 5, 2024

Choose a reason for hiding this comment

wgtmac commented Apr 5, 2024

kou commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

kou commented Apr 5, 2024

wgtmac commented Apr 5, 2024

kou commented Apr 5, 2024

wgtmac commented Apr 5, 2024

kou commented Apr 5, 2024

kou commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

conbench-apache-arrow bot commented Apr 6, 2024

GH-40855: [C++][ORC] Fix `std::filesystem` related link error with ORC 2.0.0 or later #41023

GH-40855: [C++][ORC] Fix `std::filesystem` related link error with ORC 2.0.0 or later #41023

kou commented Apr 5, 2024 •

edited

Loading