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-1751: [C++] fix syntax error in ThirdpartyToolchain #1994

Closed
wants to merge 1 commit into from

Conversation

luffy-zh
Copy link
Contributor

@luffy-zh luffy-zh commented Aug 5, 2024

What changes were proposed in this pull request?
fix syntax error in ThirdpartyToolchain

Why are the changes needed?
Handle the issue discussed here

How was this patch tested?
Test it locally

Was this patch authored or co-authored using generative AI tooling?
NO

@github-actions github-actions bot added the BUILD label Aug 5, 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.

Is this all we need, @luffy-zh ?

Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

+1

Thanks @luffy-zh!

cc @kou @raulcd if there is anything missing

Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think the underlying issue which I had to use this patch:

diff --git a/cmake_modules/ThirdpartyToolchain.cmake b/cmake_modules/ThirdpartyToolchain.cmake
index aa520ff..7bd75d1 100644
--- a/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cmake_modules/ThirdpartyToolchain.cmake
@@ -416,6 +416,18 @@ endif ()
 if (NOT "${PROTOBUF_HOME}" STREQUAL "" OR ORC_PACKAGE_KIND STREQUAL "conan")
   find_package (Protobuf REQUIRED)
   set(PROTOBUF_VENDORED FALSE)
+  if (Protobuf_FOUND AND NOT PROTOBUF_STATIC_LIB)
+    get_target_property (target_type protobuf::libprotobuf TYPE)
+    if (target_type STREQUAL "STATIC_LIBRARY")
+        set(PROTOBUF_STATIC_LIB protobuf::libprotobuf)
+    endif ()
+  endif()
+  if (Protobuf_FOUND AND NOT PROTOC_STATIC_LIB)
+    get_target_property (target_type protobuf::libprotoc TYPE)
+    if (target_type STREQUAL "STATIC_LIBRARY")
+        set (PROTOC_STATIC_LIB protobuf::libprotoc)
+    endif ()
+  endif()
 else ()
   set(PROTOBUF_PREFIX "${THIRDPARTY_DIR}/protobuf_ep-install")
   set(PROTOBUF_INCLUDE_DIR "${PROTOBUF_PREFIX}/include")

might have already been fixed as suggested by @kou at: #1963

@dongjoon-hyun dongjoon-hyun added this to the 2.0.2 milestone Aug 6, 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.

+1, LGTM. Thank you, @luffy-zh , @wgtmac , @raulcd .
Merged to main/2.0 for Apache ORC 2.0.2.

@dongjoon-hyun
Copy link
Member

Merged to main.

There exists a conflict on branch-2.0. Could you make a backporting PR to branch-2.0, @luffy-zh ?

@dongjoon-hyun dongjoon-hyun modified the milestones: 2.0.2, 2.1.0 Aug 6, 2024
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

Sorry for late review.

@luffy-zh
Copy link
Contributor Author

luffy-zh commented Aug 7, 2024

Merged to main.

There exists a conflict on branch-2.0. Could you make a backporting PR to branch-2.0, @luffy-zh ?

OK, I will make a backporting PR to branch-2.0 later.

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.

5 participants