[GitHub] [arrow] kou commented on a diff in pull request #12881: ARROW-16183: [C++][FlightRPC] Support bundled UCX

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org