-
Notifications
You must be signed in to change notification settings - Fork 485
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
ORC-1833: [C++] Fix CMake script to be used inside another project #2108
Conversation
@@ -85,6 +85,10 @@ option(ORC_PACKAGE_KIND | |||
"Arbitrary string that identifies the kind of package" | |||
"") | |||
|
|||
option(ORC_ENABLE_CLANG_TOOLS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should check CMAKE_CXX_COMPILER_ID MATCHES "Clang"
when the option is ON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are unrelated. Sometimes I want to use gcc to build but Clang is available on my machine to do the checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are unrelated. Sometimes I want to use gcc to build but Clang is available on my machine to do the checking.
Fine, but there are some compile options used by GCC that clang did not understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think we can disable it by default because we have switched to use cpp-linter-action
already. But this PR is required to backport to 2.1.1 so I decided to keep it to make the CI happy on branch-2.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
cc @kou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
### What changes were proposed in this pull request? The change to add support for exporting CMake config and target has introduced some minor issues when the ORC C++ library is used inside a larger CMake project. Mainly there are three issues: - We should prepend (not append) our CMake modules so we always use our own modules when there are naming conflict. - Do not use CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR because they are tied to the root project and it is no longer ORC project when we are inside another project. - Add orc_ prefix to our CMake functions to avoid potential conflict. See: apache/arrow#45226 ### Why are the changes needed? We had a problem with upgrading Apache ORC to 2.1.0 in the Apache Arrow. See: apache/arrow#45226 ### How was this patch tested? - Pass all CIs. - Manually integrated it with Apache Arrow. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #2108 from wgtmac/fix_cmake. Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Gang Wu <ustcwg@gmail.com> (cherry picked from commit 3eb423a) Signed-off-by: Gang Wu <ustcwg@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Thank you, @wgtmac and @ffacs .
cc @williamhyun
What changes were proposed in this pull request?
The change to add support for exporting CMake config and target has introduced some minor issues when the ORC C++ library is used inside a larger CMake project. Mainly there are three issues:
See: apache/arrow#45226
Why are the changes needed?
We had a problem with upgrading Apache ORC to 2.1.0 in the Apache Arrow. See: apache/arrow#45226
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No.