Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

wgtmac
Copy link
Member

@wgtmac wgtmac commented Jan 13, 2025

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.

@@ -85,6 +85,10 @@ option(ORC_PACKAGE_KIND
"Arbitrary string that identifies the kind of package"
"")

option(ORC_ENABLE_CLANG_TOOLS
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

@ffacs ffacs left a comment

Choose a reason for hiding this comment

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

LGTM.

@wgtmac
Copy link
Member Author

wgtmac commented Jan 13, 2025

cc @kou

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@wgtmac
Copy link
Member Author

wgtmac commented Jan 13, 2025

Thanks for reviewing it! @ffacs @kou

@wgtmac wgtmac closed this in 3eb423a Jan 13, 2025
wgtmac added a commit that referenced this pull request Jan 13, 2025
### 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>
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.

+1, LGTM. Thank you, @wgtmac and @ffacs .

cc @williamhyun

@dongjoon-hyun dongjoon-hyun added this to the 2.1.1 milestone Feb 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants