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 2021/12/22 21:37:53 UTC
[GitHub] [arrow] lafiona commented on a change in pull request #12004: ARROW-13185: [MATLAB] Create a single MEX gateway function which delegates to specific C++ functions
lafiona commented on a change in pull request #12004:
URL: https://github.com/apache/arrow/pull/12004#discussion_r774191805
##########
File path: matlab/CMakeLists.txt
##########
@@ -212,56 +233,150 @@ if(MATLAB_BUILD_TESTS)
# C++ libraries that are built from source.
build_arrow(BUILD_GTEST)
else()
+ # GTest is found, on Windows, IMPORTED_LOCATION needs to be set to indicate where the shared
+ # libraries live
+ if(WIN32)
+ set(GTEST_SHARED_LIB_DIR "${GTEST_ROOT}/bin")
+ set(GTEST_SHARED_LIBRARY_FILENAME
+ "${CMAKE_SHARED_LIBRARY_PREFIX}gtest${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ set(GTEST_SHARED_LIBRARY_LIB
+ "${GTEST_SHARED_LIB_DIR}/${GTEST_SHARED_LIBRARY_FILENAME}")
+
+ set(GTEST_MAIN_SHARED_LIB_DIR "${GTEST_ROOT}/bin")
+ set(GTEST_MAIN_SHARED_LIBRARY_FILENAME
+ "${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ set(GTEST_MAIN_SHARED_LIBRARY_LIB
+ "${GTEST_MAIN_SHARED_LIB_DIR}/${GTEST_MAIN_SHARED_LIBRARY_FILENAME}")
+
+ set_target_properties(GTest::gtest PROPERTIES IMPORTED_LOCATION
+ "${GTEST_SHARED_LIBRARY_LIB}")
+
+ set_target_properties(GTest::gtest_main
+ PROPERTIES IMPORTED_LOCATION
+ "${GTEST_MAIN_SHARED_LIBRARY_LIB}")
+ endif()
+
find_package(Arrow)
if(NOT Arrow_FOUND)
# Trigger an automatic build of the Arrow C++ libraries.
build_arrow()
endif()
endif()
+
+ # On Windows: copy the gtest and gtest_main runtime DLLs into the directory where the
+ # MATLAB C++ tests reside, since Windows require runtime DLLs that are depended on by
+ # executables to be in the same directory (or on the %PATH%).
+ if(WIN32)
+ set(MATLAB_TESTS_DIR "${MATLAB_BUILD_OUTPUT_DIR}")
+
+ get_property(GTEST_SHARED_LIB
+ TARGET GTest::gtest
+ PROPERTY IMPORTED_LOCATION)
+
+ get_property(GTEST_MAIN_SHARED_LIB
+ TARGET GTest::gtest_main
+ PROPERTY IMPORTED_LOCATION)
+
+ add_custom_target(copy_gtest_to_tests_dir ALL
Review comment:
This piece of code is not necessary anymore after adding the path of the shared libraries to the test environment path.
##########
File path: matlab/CMakeLists.txt
##########
@@ -212,56 +233,150 @@ if(MATLAB_BUILD_TESTS)
# C++ libraries that are built from source.
build_arrow(BUILD_GTEST)
else()
+ # GTest is found, on Windows, IMPORTED_LOCATION needs to be set to indicate where the shared
+ # libraries live
+ if(WIN32)
+ set(GTEST_SHARED_LIB_DIR "${GTEST_ROOT}/bin")
+ set(GTEST_SHARED_LIBRARY_FILENAME
+ "${CMAKE_SHARED_LIBRARY_PREFIX}gtest${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ set(GTEST_SHARED_LIBRARY_LIB
+ "${GTEST_SHARED_LIB_DIR}/${GTEST_SHARED_LIBRARY_FILENAME}")
+
+ set(GTEST_MAIN_SHARED_LIB_DIR "${GTEST_ROOT}/bin")
+ set(GTEST_MAIN_SHARED_LIBRARY_FILENAME
+ "${CMAKE_SHARED_LIBRARY_PREFIX}gtest_main${CMAKE_SHARED_LIBRARY_SUFFIX}")
+ set(GTEST_MAIN_SHARED_LIBRARY_LIB
+ "${GTEST_MAIN_SHARED_LIB_DIR}/${GTEST_MAIN_SHARED_LIBRARY_FILENAME}")
+
+ set_target_properties(GTest::gtest PROPERTIES IMPORTED_LOCATION
+ "${GTEST_SHARED_LIBRARY_LIB}")
+
+ set_target_properties(GTest::gtest_main
+ PROPERTIES IMPORTED_LOCATION
+ "${GTEST_MAIN_SHARED_LIBRARY_LIB}")
+ endif()
+
find_package(Arrow)
if(NOT Arrow_FOUND)
# Trigger an automatic build of the Arrow C++ libraries.
build_arrow()
endif()
endif()
+
+ # On Windows: copy the gtest and gtest_main runtime DLLs into the directory where the
+ # MATLAB C++ tests reside, since Windows require runtime DLLs that are depended on by
+ # executables to be in the same directory (or on the %PATH%).
+ if(WIN32)
+ set(MATLAB_TESTS_DIR "${MATLAB_BUILD_OUTPUT_DIR}")
+
+ get_property(GTEST_SHARED_LIB
+ TARGET GTest::gtest
+ PROPERTY IMPORTED_LOCATION)
+
+ get_property(GTEST_MAIN_SHARED_LIB
+ TARGET GTest::gtest_main
+ PROPERTY IMPORTED_LOCATION)
+
+ add_custom_target(copy_gtest_to_tests_dir ALL
+ COMMAND ${CMAKE_COMMAND} -E make_directory ${MATLAB_TESTS_DIR}
+ COMMAND ${CMAKE_COMMAND} -E copy_if_different ${GTEST_SHARED_LIB}
+ ${MATLAB_TESTS_DIR}
+ COMMAND ${CMAKE_COMMAND} -E copy_if_different
+ ${GTEST_MAIN_SHARED_LIB} ${MATLAB_TESTS_DIR}
+ COMMENT "Start copying gtest dlls from ${GTEST_SHARED_LIB} to ${MATLAB_TESTS_DIR} and ${GTEST_MAIN_SHARED_LIB} to ${MATLAB_TESTS_DIR}"
+ )
+ add_dependencies(copy_gtest_to_tests_dir GTest::gtest)
+ add_dependencies(copy_gtest_to_tests_dir GTest::gtest_main)
+ endif()
+
else()
find_package(Arrow)
if(NOT Arrow_FOUND)
build_arrow()
endif()
endif()
+# On Windows: copy arrow.dll into the directory where the MATLAB C++ tests reside,
+# since Windows require runtime DLLs that are depended on by executables to be in
+# the same directory (or on the %PATH%).
+if(WIN32)
+ get_property(ARROW_SHARED_LIB
+ TARGET arrow_shared
+ PROPERTY IMPORTED_LOCATION)
+
+ add_custom_target(copy_arrow_to_build_output_dir ALL
+ COMMAND ${CMAKE_COMMAND} -E make_directory ${MATLAB_BUILD_OUTPUT_DIR}
+ COMMAND ${CMAKE_COMMAND} -E copy_if_different ${ARROW_SHARED_LIB}
+ ${MATLAB_BUILD_OUTPUT_DIR}
+ COMMENT "Start copying arrow.dll from ${ARROW_SHARED_LIB} to ${MATLAB_BUILD_OUTPUT_DIR}"
+ )
+ add_dependencies(copy_arrow_to_build_output_dir arrow_shared)
+endif()
+
# MATLAB is Required
find_package(Matlab REQUIRED)
-# Construct the absolute path to featherread's source files
-set(featherread_sources featherreadmex.cc feather_reader.cc util/handle_status.cc
- util/unicode_conversion.cc)
-list(TRANSFORM featherread_sources PREPEND ${CMAKE_SOURCE_DIR}/src/)
-
-# Build featherreadmex MEX binary
-matlab_add_mex(R2018a
- NAME featherreadmex
- SRC ${featherread_sources}
- LINK_TO arrow_shared)
+message(STATUS "Mex Library: ${Matlab_MEX_LIBRARY}")
+message(STATUS "Mex Include Folder: ${Matlab_INCLUDE_DIRS}")
+
+set(CPP_SOURCE_DIR ${CMAKE_SOURCE_DIR}/src/cpp)
+set(MATLAB_SOURCE_DIR ${CMAKE_SOURCE_DIR}/src/cpp/arrow/matlab)
+
+set(arrow_matlab_sources
+ mex/mex_util.cc
+ feather/feather_reader.cc
+ feather/feather_writer.cc
+ feather/feather_functions.cc
+ feather/util/handle_status.cc
+ feather/util/unicode_conversion.cc)
+list(TRANSFORM arrow_matlab_sources PREPEND ${CPP_SOURCE_DIR}/arrow/matlab/)
+
+add_library(arrow_matlab SHARED ${arrow_matlab_sources})
+
+# Declare a dependency on arrow_shared (libarrow.so/dylib/dll).
+target_link_libraries(arrow_matlab arrow_shared)
+
+# On macOS, rpaths for linking imported targets are not automatically added to linked libraries
+if(APPLE)
+ get_property(ARROW_SHARED_IMPORTED_LOCATION
+ TARGET arrow_shared
+ PROPERTY IMPORTED_LOCATION)
+ get_filename_component(ARROW_SHARED_LIBRARY_DIR ${ARROW_SHARED_IMPORTED_LOCATION}
+ DIRECTORY)
+ target_link_options(arrow_matlab PUBLIC -Wl,-rpath,${ARROW_SHARED_LIBRARY_DIR})
+endif()
-# Construct the absolute path to featherwrite's source files
-set(featherwrite_sources featherwritemex.cc feather_writer.cc util/handle_status.cc
- util/unicode_conversion.cc)
-list(TRANSFORM featherwrite_sources PREPEND ${CMAKE_SOURCE_DIR}/src/)
+# Declare a dependency on the MEX shared library (libmex.so/dylib/dll).
+target_link_libraries(arrow_matlab ${Matlab_MX_LIBRARY})
+target_link_libraries(arrow_matlab ${Matlab_MEX_LIBRARY})
+
+# Include the MATLAB MEX headers.
+target_include_directories(arrow_matlab PRIVATE ${Matlab_INCLUDE_DIRS})
+target_include_directories(arrow_matlab PRIVATE ${CPP_SOURCE_DIR})
+target_include_directories(arrow_matlab PRIVATE ${ARROW_INCLUDE_DIR})
+target_compile_definitions(arrow_matlab PRIVATE ARROW_MATLAB_EXPORTING)
+
+set(mexfcn_sources mex/mexfcn.cc)
+list(TRANSFORM mexfcn_sources PREPEND ${CPP_SOURCE_DIR}/arrow/matlab/)
+
+# Store the CMake Configuration Type for multi-configuration generators so
+# that we can reference it later to automatically add the correct build
+# folder to the MATLAB Search Path
+get_property(is_multi_config GLOBAL PROPERTY GENERATOR_IS_MULTI_CONFIG)
+if(is_multi_config)
+ add_custom_command(TARGET arrow_matlab
+ POST_BUILD
+ COMMAND "${CMAKE_COMMAND}" -E echo $<CONFIG> >
+ ${CMAKE_BINARY_DIR}/is_multi_config.txt)
+endif()
Review comment:
This makes sense to me! I have removed the code to create `is_multi_config.txt`. And I will add it in a future pull request for [ARROW-15182](https://issues.apache.org/jira/browse/ARROW-15182).
##########
File path: matlab/CMakeLists.txt
##########
@@ -273,10 +388,35 @@ 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)
+ # 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 a test target.
add_test(PlaceholderTestTarget placeholder_test)
+ add_test(CheckNumArgsTestTarget mex_util_test)
+
+ # For Windows: add the directory of libmx.dll and libmex.dll to the %PATH% for
+ # running CheckNumArgsTestTarget
+ # Note: When appending to the path using set_test_properties' ENVIRONNENT property,
+ # make sure that we escape ';' to avoid cmake from interpreting the input as
+ # a list of strings.
+ if(WIN32)
+ set(MATLAB_DLL_DEPENDENCIES_DIR "${Matlab_ROOT_DIR}/bin/win64")
+ set_tests_properties(CheckNumArgsTestTarget
+ PROPERTIES ENVIRONMENT
+ "PATH=${MATLAB_DLL_DEPENDENCIES_DIR}\;$ENV{PATH}")
Review comment:
Thanks for this suggestion, it'll be more efficient to skip copying the GTest dlls. I will make this change.
--
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