Copilot commented on code in PR #12359:
URL: https://github.com/apache/gluten/pull/12359#discussion_r3467512621
##########
cpp/velox/CMakeLists.txt:
##########
@@ -388,6 +382,12 @@ else()
endif()
target_link_libraries(velox PUBLIC external::veloxthrift)
+# If folly is not installed in system lib paths, please add
+# `-DCMAKE_PREFIX_PATH="${folly lib path}" to cmake arguments. It is also
+# applicable to other dependencies.
+find_package(Folly REQUIRED CONFIG)
+target_link_libraries(velox PUBLIC Folly::folly)
Review Comment:
`Folly::folly` is linked before the static thrift libraries
(`libthriftcpp2.a`/`libthriftprotocol.a`), but fbthrift/thrift typically
depends on Folly symbols. With vcpkg triplets defaulting to static linkage,
this ordering can still produce undefined Folly symbols at link time (static
archives are only searched once, left-to-right). Consider moving the Folly link
to *after* the thrift libraries block, or using a link group/rescan strategy to
break the circular static dependency chain.
##########
cpp/velox/CMakeLists.txt:
##########
@@ -388,6 +382,12 @@ else()
endif()
target_link_libraries(velox PUBLIC external::veloxthrift)
+# If folly is not installed in system lib paths, please add
+# `-DCMAKE_PREFIX_PATH="${folly lib path}" to cmake arguments. It is also
+# applicable to other dependencies.
Review Comment:
The inline CMake option in this comment has an opening backtick but no
closing backtick, and "folly lib path" is misleading (CMake looks for package
config under the install prefix, not the library directory). Consider fixing
the formatting and wording to avoid confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
