-
Notifications
You must be signed in to change notification settings - Fork 483
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
Conversation
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.
Is this all we need, @luffy-zh ?
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.
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.
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
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.
Merged to main. There exists a conflict 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.
+1
Sorry for late review.
OK, I will make a backporting PR to branch-2.0 later. |
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