ORC-1846: [C++] Fix imported libraries in the Conan build #2121
Conversation
|
Thanks for the review @ffacs ! |
### What changes were proposed in this pull request? Fix all imported libraries of thirdparty dependencies to build in Conan. ### Why are the changes needed? Upgrading the C++ ORC 2.1.0 in Conan will fail due to the CMake refactoring. See conan-io/conan-center-index#26426 ### How was this patch tested? See CIs from Conan: https://github.com/conan-io/conan-center-index/pull/26426/checks?check_run_id=35916226382 ### Was this patch authored or co-authored using generative AI tooling? No. Closes #2121 from wgtmac/fix_conan. Authored-by: Gang Wu <[email protected]> Signed-off-by: Gang Wu <[email protected]> (cherry picked from commit 09b0650) Signed-off-by: Gang Wu <[email protected]>
| @@ -87,7 +87,7 @@ option(ORC_PACKAGE_KIND | |||
|
|
|||
| option(ORC_ENABLE_CLANG_TOOLS | |||
| "Enable Clang tools" | |||
| ON) | |||
| OFF) | |||
There was a problem hiding this comment.
Hi, @wgtmac .
May I ask why we need to disable this for all build?
I guess ORC_PACKAGE_KIND has the information of conan.
Can we narrow down our change into the specific conan build?
There was a problem hiding this comment.
ORC_ENABLE_CLANG_TOOLS was required by the previous GitHub workflow for clang-tidy and clang-format enforcement. Now it is replaced by the cpp-linter-action and is no longer needed. At the same time, it defines custom targets like check-clang-tidy and check-format which may have name conflict with other projects (like Apache Arrow). In many build environments (like Conan or Github CIs) usually these tools are missing and result in annoying warning messages. So I think it makes sense to disable it by default.
There was a problem hiding this comment.
It sounds valid but independent from this PR. Maybe, could you handle them seperately next time please?
There was a problem hiding this comment.
BTW, do you think we need Apache ORC 2.1.1 for downstreams to deliver this? I'm wondering if this is the last one to help them.
There was a problem hiding this comment.
I'll keep it in mind next time :)
BTW, do you think we need Apache ORC 2.1.1 for downstreams to deliver this? I'm wondering if this is the last one to help them.
I don't think we need it yet and we can still keep the release cadence for now. Let's wait for feedback from the Conan community. But releasing 2.1.1 is helpful anyway.
|
Thank you for making a PR and backporting it. I left a comment in the above to remove any side-effects on |
What changes were proposed in this pull request?
Fix all imported libraries of thirdparty dependencies to build in Conan.
Why are the changes needed?
Upgrading the C++ ORC 2.1.0 in Conan will fail due to the CMake refactoring. See conan-io/conan-center-index#26426
How was this patch tested?
See CIs from Conan: https://github.com/conan-io/conan-center-index/pull/26426/checks?check_run_id=35916226382
Was this patch authored or co-authored using generative AI tooling?
No.