Skip to content

Conversation

@wgtmac
Copy link
Member

@wgtmac wgtmac commented Apr 10, 2024

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.

@github-actions github-actions bot added the CPP label Apr 10, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

According to the PR description, is this a regression? Do we need this at Apache ORC 2.0.1, @wgtmac ?

@dongjoon-hyun dongjoon-hyun added this to the 2.0.1 milestone Apr 10, 2024
@dongjoon-hyun
Copy link
Member

I added 2.0.1 as the milestone. Thank you for doing this follow-up, @wgtmac .

dongjoon-hyun pushed a commit that referenced this pull request Apr 10, 2024
### 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>
@dongjoon-hyun
Copy link
Member

Merged to main/2.0.

@wgtmac
Copy link
Member Author

wgtmac commented Apr 11, 2024

Thanks @dongjoon-hyun! Yes, it should be ported to 2.0.1 as well.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants