kou commented on code in PR #12881:
URL: https://github.com/apache/arrow/pull/12881#discussion_r851006778
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -4513,6 +4523,88 @@ if(ARROW_S3)
endif()
endif()
+# ----------------------------------------------------------------------
+# ucx - communication framework for modern, high-bandwidth and low-latency
networks
+
+macro(build_ucx)
+ message(STATUS "Building UCX from source")
+
+ set(UCX_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/ucx_ep-install")
+
+ # link with static ucx libraries leads to test failures, use shared libs
instead
Review Comment:
Could you add UCX to `LICENSE.txt`?
If we may bundle it to our binary distribution, we need to refer it. It
doesn't depend on static/shared link.
See also:
* https://infra.apache.org/licensing-howto.html#bundled-vs-non-bundled
* https://infra.apache.org/licensing-howto.html#binary
We'll not bundle UCX for `.deb` package because Debian provides a package
for UCX. But we may bundle UCX for `.rpm` package (and `.wheel`?).
##########
cpp/cmake_modules/FindUcx.cmake:
##########
@@ -0,0 +1,25 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+find_package(PkgConfig QUIET)
+pkg_check_modules(Ucx IMPORTED_TARGET ucx)
Review Comment:
We can just use `find_package(ucx ...)`. And we don't need to add this file.
##########
cpp/src/arrow/flight/transport/ucx/CMakeLists.txt:
##########
@@ -18,19 +18,15 @@
add_custom_target(arrow_flight_transport_ucx)
arrow_install_all_headers("arrow/flight/transport/ucx")
-find_package(PkgConfig REQUIRED)
-pkg_check_modules(UCX REQUIRED IMPORTED_TARGET ucx)
-
set(ARROW_FLIGHT_TRANSPORT_UCX_SRCS
ucx_client.cc
ucx_server.cc
ucx.cc
ucx_internal.cc
util_internal.cc)
-set(ARROW_FLIGHT_TRANSPORT_UCX_LINK_LIBS)
-include_directories(SYSTEM ${UCX_INCLUDE_DIRS})
-list(APPEND ARROW_FLIGHT_TRANSPORT_UCX_LINK_LIBS PkgConfig::UCX)
+get_target_property(UCX_INCLUDE_DIR ucx::ucx INTERFACE_INCLUDE_DIRECTORIES)
+include_directories(SYSTEM ${UCX_INCLUDE_DIR})
Review Comment:
We can remove this once https://github.com/apache/arrow/pull/12861 is
completed. :-)
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -72,6 +72,7 @@ set(ARROW_THIRDPARTY_DEPENDENCIES
Snappy
Substrait
Thrift
+ Ucx
Review Comment:
How about using `ucx` because UCX uses `ucx` for CMake package name
https://github.com/openucx/ucx/blob/master/cmake/ucx-targets.cmake.in ?
```suggestion
ucx
```
--
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]
