You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/01/29 02:28:20 UTC
[GitHub] [arrow] kou commented on a change in pull request #12004: ARROW-13185: [MATLAB] Create a single MEX gateway function which delegates to specific C++ functions
kou commented on a change in pull request #12004:
URL: https://github.com/apache/arrow/pull/12004#discussion_r790106478
##########
File path: matlab/CMakeLists.txt
##########
@@ -273,10 +328,209 @@ if(MATLAB_BUILD_TESTS)
# Define a test executable target. TODO: Remove the placeholder test. This is
# just for testing GoogleTest integration.
add_executable(placeholder_test ${CMAKE_SOURCE_DIR}/src/placeholder_test.cc)
+ add_executable(mex_util_test ${CPP_SOURCE_DIR}/arrow/matlab/mex/mex_util_test.cc)
+
# Declare a dependency on the GTest::gtest and GTest::gtest_main IMPORTED
# targets.
target_link_libraries(placeholder_test GTest::gtest GTest::gtest_main)
- # Add a test target.
+ # Declare a dependency on the GTest::gtest and GTest::gtest_main IMPORTED
+ # targets.
+ target_link_libraries(mex_util_test GTest::gtest GTest::gtest_main)
+ target_link_libraries(mex_util_test arrow_matlab)
+
+ # Include the MATLAB MEX headers.
+ target_include_directories(mex_util_test PRIVATE ${Matlab_INCLUDE_DIRS})
+ # Include the C++ source headers.
+ target_include_directories(mex_util_test PRIVATE ${CPP_SOURCE_DIR})
+
+ # Add test targets for C++ tests.
add_test(PlaceholderTestTarget placeholder_test)
+ add_test(CheckNumArgsTestTarget mex_util_test)
+
+ # On macOS, add the directory of libarrow.dylib to the $DYLD_LIBRARY_PATH for
+ # running CheckNumArgsTestTarget.
+ if(APPLE)
+ get_target_property(ARROW_SHARED_LIB arrow_shared IMPORTED_LOCATION)
+ get_filename_component(ARROW_SHARED_LIB_DIR ${ARROW_SHARED_LIB} DIRECTORY)
+
+ set_tests_properties(CheckNumArgsTestTarget
+ PROPERTIES ENVIRONMENT
+ "DYLD_LIBRARY_PATH=${ARROW_SHARED_LIB_DIR}")
+ endif()
+
+ # On Windows:
+ # Add the directory of gtest.dll and gtest_main.dll to the %PATH% for running
+ # all tests.
+ # Add the directory of libmx.dll, libmex.dll, and libarrow.dll to the %PATH% for running
+ # CheckNumArgsTestTarget.
+ # Note: When appending to the path using set_test_properties' ENVIRONMENT property,
+ # make sure that we escape ';' to prevent CMake from interpreting the input as
+ # a list of strings.
+ if(WIN32)
+ get_target_property(GTEST_SHARED_LIB GTest::gtest IMPORTED_LOCATION)
+ get_filename_component(GTEST_SHARED_LIB_DIR ${GTEST_SHARED_LIB} DIRECTORY)
+
+ get_target_property(GTEST_MAIN_SHARED_LIB GTest::gtest_main IMPORTED_LOCATION)
+ get_filename_component(GTEST_MAIN_SHARED_LIB_DIR ${GTEST_MAIN_SHARED_LIB} DIRECTORY)
+
+ set_tests_properties(PlaceholderTestTarget
+ PROPERTIES ENVIRONMENT
+ "PATH=${GTEST_SHARED_LIB_DIR}\;${GTEST_MAIN_SHARED_LIB_DIR}\;$ENV{PATH}"
+ )
+
+ get_target_property(ARROW_SHARED_LIB arrow_shared IMPORTED_LOCATION)
+ get_filename_component(ARROW_SHARED_LIB_DIR ${ARROW_SHARED_LIB} DIRECTORY)
+
+ set(MATLAB_DLL_DEPENDENCIES_DIR "${Matlab_ROOT_DIR}/bin/win64")
+
+ set_tests_properties(CheckNumArgsTestTarget
+ PROPERTIES ENVIRONMENT
+ "PATH=${ARROW_SHARED_LIB_DIR}\;${MATLAB_DLL_DEPENDENCIES_DIR}\;${GTEST_SHARED_LIB_DIR}\;${GTEST_MAIN_SHARED_LIB_DIR}\;$ENV{PATH}"
+ )
+ endif()
+endif()
+
+# ##############################################################################
+# Install
+# ##############################################################################
+# Create a subdirectory at CMAKE_INSTALL_PREFIX to install the interface.
+set(CMAKE_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/arrow_matlab")
+
+# Install MATLAB source files.
+if(APPLE)
+ # On macOS, exclude '.DS_Store' files in the source tree from installation.
+ install(DIRECTORY "${CMAKE_SOURCE_DIR}/src/matlab/"
+ DESTINATION ${CMAKE_INSTALL_DIR}
+ PATTERN ".DS_Store" EXCLUDE)
+else()
+ install(DIRECTORY "${CMAKE_SOURCE_DIR}/src/matlab/" DESTINATION ${CMAKE_INSTALL_DIR})
+endif()
Review comment:
How about always using `APPLE` clause?
```suggestion
# On macOS, exclude '.DS_Store' files in the source tree from installation.
install(DIRECTORY "${CMAKE_SOURCE_DIR}/src/matlab/"
DESTINATION ${CMAKE_INSTALL_DIR}
PATTERN ".DS_Store" EXCLUDE)
```
Generally, `.DS_Store` doesn't exist on non macOS environments. It just does nothing.
##########
File path: matlab/CMakeLists.txt
##########
@@ -76,108 +96,96 @@ function(build_arrow)
file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
add_library(${ARROW_LIBRARY_TARGET} SHARED IMPORTED)
set_target_properties(${ARROW_LIBRARY_TARGET}
- PROPERTIES ${ARROW_IMPORTED_TYPE} ${ARROW_SHARED_LIB}
- INTERFACE_INCLUDE_DIRECTORIES ${ARROW_INCLUDE_DIR})
+ PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${ARROW_INCLUDE_DIR}
+ IMPORTED_LOCATION ${ARROW_SHARED_LIB})
+ if(WIN32)
+ set_target_properties(${ARROW_LIBRARY_TARGET} PROPERTIES IMPORTED_IMPLIB
+ ${ARROW_LINK_LIB})
+ endif()
add_dependencies(${ARROW_LIBRARY_TARGET} arrow_ep)
if(ARG_BUILD_GTEST)
build_gtest()
endif()
-
endfunction()
macro(enable_gtest)
- if(WIN32)
- set(ARROW_GTEST_IMPORTED_TYPE IMPORTED_IMPLIB)
- set(ARROW_GTEST_MAIN_IMPORTED_TYPE IMPORTED_IMPLIB)
+ set(ARROW_GTEST_PREFIX "${ARROW_BINARY_DIR}/googletest_ep-prefix")
+ set(ARROW_GTEST_MAIN_PREFIX "${ARROW_BINARY_DIR}/googletest_ep-prefix")
- set(ARROW_GTEST_LIBRARY_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX})
- set(ARROW_GTEST_MAIN_LIBRARY_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX})
+ if(WIN32)
+ set(ARROW_GTEST_LINK_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX})
+ set(ARROW_GTEST_SHARED_LIB_DIR "${ARROW_GTEST_PREFIX}/bin")
+ set(ARROW_GTEST_MAIN_LINK_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX})
+ set(ARROW_GTEST_MAIN_SHARED_LIB_DIR "${ARROW_GTEST_MAIN_PREFIX}/bin")
else()
- set(ARROW_GTEST_IMPORTED_TYPE IMPORTED_LOCATION)
- set(ARROW_GTEST_MAIN_IMPORTED_TYPE IMPORTED_LOCATION)
-
- set(ARROW_GTEST_LIBRARY_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
- set(ARROW_GTEST_MAIN_LIBRARY_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
+ set(ARROW_GTEST_LINK_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
+ set(ARROW_GTEST_SHARED_LIB_DIR "${ARROW_GTEST_PREFIX}/lib")
+ set(ARROW_GTEST_MAIN_LINK_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
+ set(ARROW_GTEST_MAIN_SHARED_LIB_DIR "${ARROW_GTEST_MAIN_PREFIX}/lib")
endif()
- set(ARROW_GTEST_PREFIX "${ARROW_BINARY_DIR}/googletest_ep-prefix")
set(ARROW_GTEST_INCLUDE_DIR "${ARROW_GTEST_PREFIX}/include")
- set(ARROW_GTEST_LIBRARY_DIR "${ARROW_GTEST_PREFIX}/lib")
+ set(ARROW_GTEST_LINK_LIB_DIR "${ARROW_GTEST_PREFIX}/lib")
+ set(ARROW_GTEST_LINK_LIB
+ "${ARROW_GTEST_LINK_LIB_DIR}/${CMAKE_IMPORT_LIBRARY_PREFIX}gtest${ARROW_GTEST_LINK_SUFFIX}"
Review comment:
It seems that we can always use `CMAKE_IMPORT_LIBRARY_SUFFIX` here.
##########
File path: matlab/CMakeLists.txt
##########
@@ -273,10 +328,209 @@ if(MATLAB_BUILD_TESTS)
# Define a test executable target. TODO: Remove the placeholder test. This is
# just for testing GoogleTest integration.
add_executable(placeholder_test ${CMAKE_SOURCE_DIR}/src/placeholder_test.cc)
+ add_executable(mex_util_test ${CPP_SOURCE_DIR}/arrow/matlab/mex/mex_util_test.cc)
+
# Declare a dependency on the GTest::gtest and GTest::gtest_main IMPORTED
# targets.
target_link_libraries(placeholder_test GTest::gtest GTest::gtest_main)
- # Add a test target.
+ # Declare a dependency on the GTest::gtest and GTest::gtest_main IMPORTED
+ # targets.
+ target_link_libraries(mex_util_test GTest::gtest GTest::gtest_main)
+ target_link_libraries(mex_util_test arrow_matlab)
+
+ # Include the MATLAB MEX headers.
+ target_include_directories(mex_util_test PRIVATE ${Matlab_INCLUDE_DIRS})
+ # Include the C++ source headers.
+ target_include_directories(mex_util_test PRIVATE ${CPP_SOURCE_DIR})
+
+ # Add test targets for C++ tests.
add_test(PlaceholderTestTarget placeholder_test)
+ add_test(CheckNumArgsTestTarget mex_util_test)
+
+ # On macOS, add the directory of libarrow.dylib to the $DYLD_LIBRARY_PATH for
+ # running CheckNumArgsTestTarget.
+ if(APPLE)
+ get_target_property(ARROW_SHARED_LIB arrow_shared IMPORTED_LOCATION)
+ get_filename_component(ARROW_SHARED_LIB_DIR ${ARROW_SHARED_LIB} DIRECTORY)
+
+ set_tests_properties(CheckNumArgsTestTarget
+ PROPERTIES ENVIRONMENT
+ "DYLD_LIBRARY_PATH=${ARROW_SHARED_LIB_DIR}")
+ endif()
+
+ # On Windows:
+ # Add the directory of gtest.dll and gtest_main.dll to the %PATH% for running
+ # all tests.
+ # Add the directory of libmx.dll, libmex.dll, and libarrow.dll to the %PATH% for running
+ # CheckNumArgsTestTarget.
+ # Note: When appending to the path using set_test_properties' ENVIRONMENT property,
+ # make sure that we escape ';' to prevent CMake from interpreting the input as
+ # a list of strings.
+ if(WIN32)
+ get_target_property(GTEST_SHARED_LIB GTest::gtest IMPORTED_LOCATION)
+ get_filename_component(GTEST_SHARED_LIB_DIR ${GTEST_SHARED_LIB} DIRECTORY)
+
+ get_target_property(GTEST_MAIN_SHARED_LIB GTest::gtest_main IMPORTED_LOCATION)
+ get_filename_component(GTEST_MAIN_SHARED_LIB_DIR ${GTEST_MAIN_SHARED_LIB} DIRECTORY)
+
+ set_tests_properties(PlaceholderTestTarget
+ PROPERTIES ENVIRONMENT
+ "PATH=${GTEST_SHARED_LIB_DIR}\;${GTEST_MAIN_SHARED_LIB_DIR}\;$ENV{PATH}"
+ )
+
+ get_target_property(ARROW_SHARED_LIB arrow_shared IMPORTED_LOCATION)
+ get_filename_component(ARROW_SHARED_LIB_DIR ${ARROW_SHARED_LIB} DIRECTORY)
+
+ set(MATLAB_DLL_DEPENDENCIES_DIR "${Matlab_ROOT_DIR}/bin/win64")
+
+ set_tests_properties(CheckNumArgsTestTarget
+ PROPERTIES ENVIRONMENT
+ "PATH=${ARROW_SHARED_LIB_DIR}\;${MATLAB_DLL_DEPENDENCIES_DIR}\;${GTEST_SHARED_LIB_DIR}\;${GTEST_MAIN_SHARED_LIB_DIR}\;$ENV{PATH}"
+ )
+ endif()
+endif()
+
+# ##############################################################################
+# Install
+# ##############################################################################
+# Create a subdirectory at CMAKE_INSTALL_PREFIX to install the interface.
+set(CMAKE_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/arrow_matlab")
+
+# Install MATLAB source files.
+if(APPLE)
+ # On macOS, exclude '.DS_Store' files in the source tree from installation.
+ install(DIRECTORY "${CMAKE_SOURCE_DIR}/src/matlab/"
+ DESTINATION ${CMAKE_INSTALL_DIR}
+ PATTERN ".DS_Store" EXCLUDE)
+else()
+ install(DIRECTORY "${CMAKE_SOURCE_DIR}/src/matlab/" DESTINATION ${CMAKE_INSTALL_DIR})
+endif()
+
+# Install arrow_matlab and mexfcn.
+if(WIN32)
+ install(TARGETS arrow_matlab mexfcn RUNTIME DESTINATION ${CMAKE_INSTALL_DIR})
+else()
+ # On Linux install arrow_matlab.so and on macOS install arrow_matlab.dylib
+ # using the LIBRARY keyword.
+ install(TARGETS arrow_matlab mexfcn LIBRARY DESTINATION ${CMAKE_INSTALL_DIR})
+endif()
Review comment:
We can simplify this like the following:
```suggestion
install(TARGETS arrow_matlab mexfcn
RUNTIME DESTINATION ${CMAKE_INSTALL_DIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_DIR})
```
##########
File path: matlab/CMakeLists.txt
##########
@@ -76,108 +96,96 @@ function(build_arrow)
file(MAKE_DIRECTORY "${ARROW_INCLUDE_DIR}")
add_library(${ARROW_LIBRARY_TARGET} SHARED IMPORTED)
set_target_properties(${ARROW_LIBRARY_TARGET}
- PROPERTIES ${ARROW_IMPORTED_TYPE} ${ARROW_SHARED_LIB}
- INTERFACE_INCLUDE_DIRECTORIES ${ARROW_INCLUDE_DIR})
+ PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${ARROW_INCLUDE_DIR}
+ IMPORTED_LOCATION ${ARROW_SHARED_LIB})
+ if(WIN32)
+ set_target_properties(${ARROW_LIBRARY_TARGET} PROPERTIES IMPORTED_IMPLIB
+ ${ARROW_LINK_LIB})
Review comment:
Can we always set `IMPORTED_IMPLIB` here?
I expect it's only used on Windows. (It's ignored on non Windows environments such as Linux and macOS.)
##########
File path: matlab/CMakeLists.txt
##########
@@ -31,23 +32,42 @@ function(build_arrow)
message(SEND_ERROR "Error: unrecognized arguments: ${ARG_UNPARSED_ARGUMENTS}")
endif()
+ # If Arrow needs to be built, the default location will be within the build tree.
+ set(ARROW_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/arrow_ep-prefix")
+
if(WIN32)
- set(ARROW_IMPORTED_TYPE IMPORTED_IMPLIB)
- set(ARROW_LIBRARY_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX})
+ # On Windows, the link time suffix (.lib) and runtime library suffixes (.dll) are different.
+ set(ARROW_LINK_SUFFIX ${CMAKE_IMPORT_LIBRARY_SUFFIX})
+ # The shared library is located in the "bin" directory.
+ set(ARROW_SHARED_LIBRARY_DIR "${ARROW_PREFIX}/bin")
else()
- set(ARROW_IMPORTED_TYPE IMPORTED_LOCATION)
- set(ARROW_LIBRARY_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
+ # On Linux and macOS, the link time suffix and runtime suffixes are consistent, (.so and .dylib).
+ set(ARROW_LINK_SUFFIX ${CMAKE_SHARED_LIBRARY_SUFFIX})
+ # The shared library is located in the "lib" directory.
+ set(ARROW_SHARED_LIBRARY_DIR "${ARROW_PREFIX}/lib")
endif()
- 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}/${CMAKE_SHARED_LIBRARY_PREFIX}arrow${ARROW_LIBRARY_SUFFIX}")
+ set(ARROW_LINK_LIB_FILENAME "${CMAKE_IMPORT_LIBRARY_PREFIX}arrow${ARROW_LINK_SUFFIX}")
Review comment:
It seems that we can always use `CMAKE_IMPORT_LIBRARY_SUFFIX` 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org