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: 7e2e6128a51dc7db57be96aab1b5fb2d044130ad

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

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 <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### 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 <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 695e0f3) Signed-off-by: Dongjoon Hyun <[email protected]>

…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 <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

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

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

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

Uh oh!

Conversation

kou commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

kou commented Apr 5, 2024

Uh oh!

github-actions bot commented Apr 5, 2024

Uh oh!

kou commented Apr 5, 2024

Uh oh!

wgtmac Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

kou Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

wgtmac Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

kou Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

wgtmac Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Apr 5, 2024

Uh oh!

kou commented Apr 5, 2024

Uh oh!

github-actions bot commented Apr 5, 2024

Uh oh!

kou commented Apr 5, 2024

Uh oh!

wgtmac commented Apr 5, 2024

Uh oh!

kou commented Apr 5, 2024

Uh oh!

wgtmac commented Apr 5, 2024

Uh oh!

kou commented Apr 5, 2024

Uh oh!

kou commented Apr 5, 2024

Uh oh!

github-actions bot commented Apr 5, 2024

Uh oh!

github-actions bot commented Apr 5, 2024

Uh oh!

conbench-apache-arrow bot commented Apr 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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