kou commented on a change in pull request #10614:
URL: https://github.com/apache/arrow/pull/10614#discussion_r665671192
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
Review comment:
Lowercase is better because we use lowercase for function call:
```suggestion
function(build_arrow)
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
Review comment:
Could you add `option(MATLAB_BUILD_TESTS ...)` somewhere?
https://cmake.org/cmake/help/latest/command/option.html
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep_build")
+
+ externalproject_add(arrow_ep
+ SOURCE_DIR "${CMAKE_SOURCE_DIR}/../cpp"
+ BINARY_DIR "${ARROW_BINARY_DIR}"
+ CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}
Review comment:
We need to specify `CMAKE_INSTALL_LIBDIR=lib` explicitly because some
platforms such as CentOS uses `lib64` for the default `CMAKE_INSTALL_LIBDIR`:
```suggestion
CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${ARROW_PREFIX}
-DCMAKE_INSTALL_LIBDIR=lib
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
Review comment:
`-DGTest_ROOT` should work with only `find_package(GTest)`. We don't
need additional code for it:
https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
Review comment:
```suggestion
set(ARROW_SHARED_LIB
"${ARROW_LIBRARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow${CMAKE_SHARED_LIBRARY_SUFFIX}")
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
+ include(ExternalProject)
+
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+ set(ARROW_INCLUDE_DIR "${ARROW_PREFIX}/include")
+ set(ARROW_LIBRARY_DIR "${ARROW_PREFIX}/lib")
+ set(ARROW_SHARED_LIB "${ARROW_LIBRARY_DIR}/libarrow.so")
+ set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep_build")
Review comment:
```suggestion
set(ARROW_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-build")
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -17,6 +17,84 @@
cmake_minimum_required(VERSION 3.20)
+# Build the Arrow C++ libraries WITHOUT bundled GoogleTest binaries.
+function(BUILD_ARROW)
Review comment:
Can we unify `build_arrow` and `build_arrow_and_gtest` by using function
arguments?
We can use `cmake_parse_arguments()` for processing arguments:
https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html
See also `resolve_dependency` macro that uses `cmake_parse_arguments()`:
https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L200-L204
##########
File path: matlab/CMakeLists.txt
##########
@@ -32,8 +110,29 @@ endif()
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${CMAKE_SOURCE_DIR}/cmake_modules)
-# Arrow is Required
-find_package(Arrow REQUIRED)
+# Only build the MATLAB interface C++ tests if MATLAB_BUILD_TESTS=ON.
+if(MATLAB_BUILD_TESTS)
+ # Should support custom GTEST_ROOT as well as package managers.
+ find_package(GTest)
+ if(NOT GTest_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries and bundled
GoogleTest binaries.
+ build_arrow_and_gtest()
+ else()
+ # Should support custom ARROW_HOME as well as package managers.
+ find_package(Arrow)
+ if(NOT Arrow_FOUND)
+ # Trigger an automatic build of the Arrow C++ libraries.
+ build_arrow()
+ endif()
+ endif()
+else()
+ # Should support custom ARROW_HOME as well as package managers.
Review comment:
`ARROW_HOME` is processed by `FindArrow.cmake`. So we don't need
additional code here.
--
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]
